Skip to content

Commit b994d60

Browse files
committed
fix(ingest): isolate tests from a variable the CLI actually reads
The isolation helper cleared HOTDATA_WORKSPACE_ID, which the CLI does not read -- the variable that locks a workspace is HOTDATA_WORKSPACE. So half the isolation did nothing, and a developer with that set got different results from CI, which is the failure the helper was added to prevent. Also pins the multi-line --sql fix, which nothing asserted: every parse_select test was single-line, so a refactor moving the keyword search ahead of the whitespace collapse would have stayed green while indented SQL broke again. And `datasource fields` rendered a blank section for a oneOf whose branches are $refs -- which is sql's selector, the one family whose selector most needs explaining. The branch is resolved against $defs before rendering now.
1 parent 4a64349 commit b994d60

5 files changed

Lines changed: 250 additions & 9 deletions

File tree

‎skills/hotdata/SKILL.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,12 +236,14 @@ hotdata jobs <job_id> [--workspace-id <workspace_id>] [--output table|json|yaml]
236236

237237
### Ingest external data (`datasource`, `ingest`, `run`)
238238

239-
Pull data from external sources (SQL databases, APIs, S3/GCS/Azure buckets, Iceberg catalogs, Kafka) into managed databases. **Three nouns, three ids, and the id is always the argument** — display names are shown but never resolved against:
239+
Pull data from external sources (SQL databases, APIs, S3/GCS/Azure buckets, Iceberg catalogs, Kafka) into managed databases. **Three nouns, three ids, and an id is always what goes on the wire** — the service has no name lookup, because a display name is a label and nothing stops two rows sharing one:
240240

241241
- **datasource** (`ds_…`) — what a credential opens: a server, a bucket root, a catalog, a cluster. Holds config + credentials, loads no data.
242242
- **ingest** (`ing_…`) — a saved load definition: `datasource + selector + destination + type/schedule`. One datasource can back many ingests.
243243
- **run** (`run_…`) — one execution attempt, with snapshots of the config version, selector, and destination it used.
244244

245+
One flag softens that for typing, and only for typing: `ingest create --source` accepts a display name and resolves it to an id **client-side, before the request**, erroring with both ids if the name matches two datasources rather than picking one. Every other argument, and every request the CLI sends, takes ids only.
246+
245247
Read commands (`datasource list|show|types|fields`, `ingest list|show|runs`, `run show`) work with a login session JWT. Commands that persist a credential (`datasource create`, `datasource update-config`, `ingest create`) **require a workspace API key** (`HOTDATA_API_KEY` / `--api-key`, `hd_...`) — the run outlives the 5-minute JWT.
246248

247249
```bash

‎src/client/ingest.rs‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1141,6 +1141,67 @@ pub const FAMILY_REFERENCE_BODY: &str = r#"{
11411141
}
11421142
}"#;
11431143

1144+
/// The same family, with its selector encoded the other legal way: the `oneOf`
1145+
/// branches hoisted into `$defs` and referenced instead of written inline.
1146+
///
1147+
/// Both encodings describe the identical pair of forms, and which one arrives
1148+
/// is the schema generator's choice — a discriminated union is hoisted by
1149+
/// default and only inlined when something asks it to be. The body above is
1150+
/// inlined, so on its own it cannot tell "the service inlines" apart from "the
1151+
/// renderer only handles inline", and a renderer that reads `properties`
1152+
/// straight off a branch passes it while printing the `sql` family — the one
1153+
/// whose two selector forms are the reason the field reference exists — as a
1154+
/// family that takes no fields at all.
1155+
///
1156+
/// Trimmed to the selector: the other schemas are exercised above, and what is
1157+
/// being pinned here is the indirection, including the discriminator `const`
1158+
/// living a hop away from the `discriminator` that names it.
1159+
///
1160+
/// `r##` rather than the `r#` the other bodies use: a JSON pointer is written
1161+
/// `"#/$defs/…"`, and that `"#` would close an `r#` string mid-literal.
1162+
#[cfg(test)]
1163+
pub const FAMILY_REFERENCE_REF_SELECTOR_BODY: &str = r##"{
1164+
"family": "sql",
1165+
"config_schema": {"properties": {}, "type": "object"},
1166+
"credentials_schema": {"properties": {}, "type": "object"},
1167+
"selector_schema": {
1168+
"$defs": {
1169+
"SqlTableSelector": {
1170+
"additionalProperties": false,
1171+
"properties": {
1172+
"mode": {"const": "tables", "default": "tables", "title": "Mode",
1173+
"type": "string"},
1174+
"schema": {"anyOf": [{"type": "string"}, {"type": "null"}],
1175+
"default": null, "title": "Schema"},
1176+
"tables": {"items": {"type": "string"}, "minItems": 1,
1177+
"title": "Tables", "type": "array",
1178+
"description": "Source tables to read. Each lands in a destination table of the same name."}
1179+
},
1180+
"required": ["tables"], "title": "SqlTableSelector", "type": "object"},
1181+
"SqlQuerySelector": {
1182+
"additionalProperties": false,
1183+
"properties": {
1184+
"mode": {"const": "query", "title": "Mode", "type": "string"},
1185+
"sql": {"minLength": 1, "title": "Sql", "type": "string"}
1186+
},
1187+
"required": ["mode", "sql"], "title": "SqlQuerySelector", "type": "object"}
1188+
},
1189+
"discriminator": {"propertyName": "mode",
1190+
"mapping": {"tables": "#/$defs/SqlTableSelector",
1191+
"query": "#/$defs/SqlQuerySelector"}},
1192+
"oneOf": [{"$ref": "#/$defs/SqlTableSelector"},
1193+
{"$ref": "#/$defs/SqlQuerySelector"}]
1194+
},
1195+
"capabilities": {
1196+
"write_modes": ["replace", "append"],
1197+
"continuous": false,
1198+
"recoverable": true,
1199+
"supports_where": true,
1200+
"multi_table": true,
1201+
"immutable_config_fields": []
1202+
}
1203+
}"##;
1204+
11441205
/// A `GET /families` body: the same shape, several families, trimmed to the
11451206
/// parts the index table reads.
11461207
#[cfg(test)]

‎src/commands/datasource.rs‎

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1034,20 +1034,60 @@ fn print_schema(schema: &serde_json::Value) {
10341034
/// a complete alternative with its own required set, so rendering one of them
10351035
/// (or merging them into a single table) would describe a request the service
10361036
/// does not accept.
1037+
///
1038+
/// The root and every branch are followed through a local `$ref` first. A
1039+
/// schema generator may write a form inline or hoist it into `$defs` and point
1040+
/// at it, and the two say the same thing — but everything downstream reads
1041+
/// `properties`, which a `$ref` object does not have. An unfollowed branch
1042+
/// therefore renders as a form with no fields and no label: the field
1043+
/// reference answering "what does this family take?" with nothing, for exactly
1044+
/// the multi-form families whose fields are hardest to guess.
1045+
///
1046+
/// Branches resolve against the ORIGINAL root, not the resolved one, because
1047+
/// `$defs` sits on the document — a root that was itself a `$ref` resolves to
1048+
/// a definition that does not carry it.
10371049
fn schema_variants(schema: &serde_json::Value) -> Vec<(Option<String>, &serde_json::Value)> {
1038-
let Some(branches) = schema.get("oneOf").and_then(|b| b.as_array()) else {
1039-
return vec![(None, schema)];
1050+
let root = resolve_local_ref(schema, schema);
1051+
let Some(branches) = root.get("oneOf").and_then(|b| b.as_array()) else {
1052+
return vec![(None, root)];
10401053
};
1041-
let discriminator = schema
1054+
let discriminator = root
10421055
.get("discriminator")
10431056
.and_then(|d| d.get("propertyName"))
10441057
.and_then(|p| p.as_str());
10451058
branches
10461059
.iter()
1060+
.map(|b| resolve_local_ref(schema, b))
10471061
.map(|b| (variant_label(b, discriminator), b))
10481062
.collect()
10491063
}
10501064

1065+
/// Follow a `$ref` pointing inside this document. Anything else is returned as
1066+
/// it stands.
1067+
///
1068+
/// Resolved with `Value::pointer`, so `#/$defs/X` and the older
1069+
/// `#/definitions/X` both work without either spelling being written here —
1070+
/// which of the two arrives is the generator's business, not this command's.
1071+
///
1072+
/// A `$ref` to another document is left unresolved rather than fetched: this
1073+
/// renders one response, and reaching the network mid-render would make
1074+
/// `datasource fields` fail for a reason that has nothing to do with the
1075+
/// family being asked about. A pointer that resolves to nothing falls back the
1076+
/// same way, so an unfamiliar schema degrades to the previous rendering
1077+
/// instead of panicking on a family the CLI has never seen.
1078+
fn resolve_local_ref<'a>(
1079+
root: &'a serde_json::Value,
1080+
node: &'a serde_json::Value,
1081+
) -> &'a serde_json::Value {
1082+
let Some(reference) = node.get("$ref").and_then(|r| r.as_str()) else {
1083+
return node;
1084+
};
1085+
let Some(pointer) = reference.strip_prefix('#') else {
1086+
return node;
1087+
};
1088+
root.pointer(pointer).unwrap_or(node)
1089+
}
1090+
10511091
/// How to ask for one variant: the discriminator field and the value that
10521092
/// selects it (`mode = query`), since that key is what the caller must send.
10531093
fn variant_label(branch: &serde_json::Value, discriminator: Option<&str>) -> Option<String> {
@@ -1666,6 +1706,77 @@ mod tests {
16661706
assert_eq!(row(&rows, "mode"), ["mode", "query", "yes", "-", "-"]);
16671707
}
16681708

1709+
/// The same two forms, hoisted into `$defs` and referenced — the encoding
1710+
/// a discriminated union gets by default, and the one the test above
1711+
/// cannot distinguish from an inlined response because its body has no
1712+
/// `$defs` at all.
1713+
///
1714+
/// Left unfollowed, a `$ref` branch has no `properties`: `field_rows`
1715+
/// returns nothing, `variant_label` finds neither a `const` nor a title,
1716+
/// and the section prints as two unlabelled empty forms while the family
1717+
/// index shows a dash for required — the field reference reporting that
1718+
/// the family with the most to explain takes no fields.
1719+
#[test]
1720+
fn a_selector_whose_forms_are_refs_renders_like_one_whose_forms_are_inline() {
1721+
let reference: FamilyReference =
1722+
serde_json::from_str(crate::client::ingest::FAMILY_REFERENCE_REF_SELECTOR_BODY)
1723+
.expect("the pinned family reference decodes");
1724+
let variants = schema_variants(&reference.selector_schema);
1725+
assert_eq!(variants.len(), 2, "both forms must be described");
1726+
1727+
// The `discriminator` is on the root and the `const` answering it is a
1728+
// hop away in `$defs`, so a label at all means the branch was followed.
1729+
let (tables_label, tables) = &variants[0];
1730+
assert_eq!(tables_label.as_deref(), Some("mode = tables"));
1731+
let rows = field_rows(tables);
1732+
assert_eq!(
1733+
row(&rows, "tables"),
1734+
[
1735+
"tables",
1736+
"string[]",
1737+
"yes",
1738+
"-",
1739+
"Source tables to read. Each lands in a destination table of the same name."
1740+
]
1741+
);
1742+
assert_eq!(row(&rows, "mode"), ["mode", "tables", "no", "tables", "-"]);
1743+
1744+
let (query_label, query) = &variants[1];
1745+
assert_eq!(query_label.as_deref(), Some("mode = query"));
1746+
assert_eq!(
1747+
row(&field_rows(query), "sql"),
1748+
["sql", "string", "yes", "-", "-"]
1749+
);
1750+
1751+
// Each form keeps its own required set through the indirection, which
1752+
// is the cell the family index prints for the whole family.
1753+
assert_eq!(
1754+
required_cell(&reference.selector_schema),
1755+
"tables, mode, sql"
1756+
);
1757+
}
1758+
1759+
#[test]
1760+
fn a_ref_this_document_cannot_answer_leaves_the_branch_alone() {
1761+
// Resolution is best effort by design: a pointer into a `$defs` that
1762+
// is not there, and a `$ref` to another document, both render the way
1763+
// they did before any ref was followed rather than failing the whole
1764+
// command. An unfamiliar schema costs the section, not the process.
1765+
for unresolvable in [
1766+
serde_json::json!({"oneOf": [{"$ref": "#/$defs/Absent"}]}),
1767+
serde_json::json!({"oneOf": [{"$ref": "https://example.invalid/s.json"}]}),
1768+
] {
1769+
let variants = schema_variants(&unresolvable);
1770+
assert_eq!(variants.len(), 1);
1771+
assert!(variants[0].0.is_none());
1772+
assert!(field_rows(variants[0].1).is_empty());
1773+
}
1774+
// A schema carrying no `$ref` is returned untouched, so the ordinary
1775+
// path is not paying for any of this.
1776+
let plain = serde_json::json!({"properties": {"a": {"type": "string"}}});
1777+
assert_eq!(field_rows(schema_variants(&plain)[0].1).len(), 1);
1778+
}
1779+
16691780
/// The destination is per-family, and a family that describes one is
16701781
/// carried through the passthrough map rather than a field of the CLI's:
16711782
/// which of `table` / `table_prefix` a family takes is the service's

‎src/commands/ingest.rs‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1631,6 +1631,58 @@ mod tests {
16311631
assert_eq!(p.limit, Some(5));
16321632
}
16331633

1634+
#[test]
1635+
fn select_reads_the_same_query_laid_out_across_lines() {
1636+
// SQL pasted out of a script or a heredoc arrives across lines, and
1637+
// every keyword match in `parse_select` looks for a literal
1638+
// single-space form — "SELECT ", " FROM ", " LIMIT ". They see any of
1639+
// this only because whitespace is collapsed BEFORE the first of them
1640+
// runs.
1641+
//
1642+
// The two layouts below are the ones that pin that, and the obvious
1643+
// pretty-printed query is not among them: an INDENTED continuation
1644+
// parses even uncollapsed, because the indent happens to supply the
1645+
// space the search wanted — "\n FROM " contains " FROM ". A test
1646+
// written the natural way would stay green with the collapse deleted.
1647+
// What actually breaks is a line break with nothing after it: a
1648+
// keyword at column zero, and a break straight after SELECT.
1649+
//
1650+
// Each assertion is equality with the single-line form rather than a
1651+
// bare `is_ok`, so it also pins that layout moves no clause boundary —
1652+
// a `WHERE` predicate silently gaining or losing its last word would
1653+
// pass an `is_ok` and reach the source engine.
1654+
let across_lines =
1655+
parse_select("SELECT id, status\nFROM public.orders\nWHERE status = 'open'\nLIMIT 5")
1656+
.unwrap();
1657+
assert_eq!(
1658+
across_lines,
1659+
parse_select("SELECT id, status FROM public.orders WHERE status = 'open' LIMIT 5")
1660+
.unwrap()
1661+
);
1662+
// Uncollapsed this one fails the `starts_with("SELECT ")` prefix
1663+
// rather than the FROM search, so it covers the other keyword match
1664+
// and the other error message.
1665+
assert_eq!(
1666+
parse_select("SELECT\n\t*\n\tFROM orders").unwrap(),
1667+
parse_select("SELECT * FROM orders").unwrap()
1668+
);
1669+
// Tabs and ragged indentation land on the same parse, which is the
1670+
// form someone actually pastes.
1671+
assert_eq!(
1672+
parse_select(
1673+
"SELECT id,\n\tstatus\n FROM public.orders\n WHERE status = 'open'\n LIMIT 5"
1674+
)
1675+
.unwrap(),
1676+
across_lines
1677+
);
1678+
// Collapsing is not a licence to accept more: a query genuinely
1679+
// missing a clause is still rejected, and for the right reason.
1680+
assert!(
1681+
parse_select("SELECT\n 1\n").unwrap_err().contains("FROM"),
1682+
"a multi-line SELECT with no FROM must still be rejected as such"
1683+
);
1684+
}
1685+
16341686
#[test]
16351687
fn select_does_not_mistake_the_word_limit_inside_a_predicate() {
16361688
// "LIMIT" in a string literal is not a LIMIT clause: only a trailing

‎tests/ingest_cli.rs‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,32 @@ use std::process::Command;
88
fn hotdata() -> Command {
99
let mut cmd = Command::new(env!("CARGO_BIN_EXE_hotdata"));
1010
// ISOLATED FROM THE MACHINE RUNNING THE TEST. A spawned process inherits
11-
// this one's environment, so without these two the assertions below run
12-
// against whatever profile the developer happens to be logged into — and
13-
// the failure that produces is invisible locally and only shows up in CI,
14-
// which is the one place nobody is watching a test they just wrote.
11+
// this one's environment, so without the four lines below the assertions
12+
// run against whatever profile the developer happens to be logged into —
13+
// and the failure that produces is invisible locally and only shows up in
14+
// CI, which is the one place nobody is watching a test they just wrote.
1515
//
1616
// That is not hypothetical: three tests here asserted a retired verb's
1717
// explanation and passed on a laptop with a default workspace, while CI
1818
// had none and got an auth error instead.
19+
//
20+
// HOTDATA_WORKSPACE is the variable that locks a workspace, and it locks
21+
// it hard: it outranks an explicit --workspace-id rather than merely
22+
// supplying a default, so a developer with it exported would drive every
23+
// command here past the resolution these tests exist to observe. A name
24+
// that is nearly it clears nothing while looking like it clears
25+
// everything, which is the worst of the three outcomes.
26+
//
27+
// The working directory is isolation too, not housekeeping. The binary
28+
// loads a .env from its own cwd and the directories above it at startup,
29+
// which is after this env_remove — so under `cargo test`, where the cwd is
30+
// the crate root, a .env there would hand the key straight back. The empty
31+
// config dir is a directory no .env is in, so pointing the child at it
32+
// closes that and any stray file further up.
1933
cmd.env("HOTDATA_CONFIG_DIR", config_dir());
2034
cmd.env_remove("HOTDATA_API_KEY");
21-
cmd.env_remove("HOTDATA_WORKSPACE_ID");
35+
cmd.env_remove("HOTDATA_WORKSPACE");
36+
cmd.current_dir(config_dir());
2237
cmd
2338
}
2439

0 commit comments

Comments
 (0)