Skip to content

Commit 8fcc836

Browse files
committed
fix: enforce boundary contracts for API response shape and multi-spec resolution
unwrap_response now returns Result — a missing wrapper key is a typed error instead of a silent fallback to the entire body. json_response_to_batch rejects unexpected JSON shapes (null, bool, number, string) instead of returning empty results. find_table renamed to find_tables, returns all matches across catalogs so bare names in multi-spec mode surface every matching table with its spec prefix. MCP instructions and get_schema advertise qualified names in multi-spec mode. Example clippy lint suppression added to fix CI --all-targets.
1 parent 713d717 commit 8fcc836

5 files changed

Lines changed: 143 additions & 35 deletions

File tree

crates/sqlize-core/examples/github_schema.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
#![expect(
2+
clippy::print_stdout,
3+
clippy::expect_used,
4+
reason = "example binary, stdout is the interface"
5+
)]
6+
17
use std::path::Path;
28

39
use sqlize_core::catalog::ddl::catalog_ddl;

crates/sqlize-core/src/datafusion/arrow_convert.rs

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,20 @@ pub fn json_response_to_batch(
9393
let items = match json {
9494
serde_json::Value::Array(arr) => arr.as_slice(),
9595
serde_json::Value::Object(_) => std::slice::from_ref(json),
96-
_ => &[],
96+
other => {
97+
return Err(DataFusionError::External(Box::new(std::io::Error::other(
98+
format!(
99+
"expected JSON array or object from API, got {}",
100+
match other {
101+
serde_json::Value::Null => "null",
102+
serde_json::Value::Bool(_) => "boolean",
103+
serde_json::Value::Number(_) => "number",
104+
serde_json::Value::String(_) => "string",
105+
_ => unreachable!(),
106+
}
107+
),
108+
))));
109+
}
97110
};
98111

99112
// Build key map once from the first item (O(keys) instead of O(rows * cols * keys))
@@ -496,4 +509,27 @@ mod tests {
496509
let batch = json_response_to_batch(&json, &cols, &params, &schema).unwrap();
497510
assert!(batch.column(0).is_null(0));
498511
}
512+
513+
#[test]
514+
fn unexpected_json_shape_is_error() {
515+
let cols = vec![response_col("title", ColumnType::String)];
516+
let table = test_table(cols.clone());
517+
let schema = virtual_table_to_schema(&table);
518+
let params = HashMap::new();
519+
520+
for (json, expected_label) in [
521+
(serde_json::Value::Null, "null"),
522+
(serde_json::Value::Bool(true), "boolean"),
523+
(serde_json::json!(42), "number"),
524+
(serde_json::json!("a string"), "string"),
525+
] {
526+
let err = json_response_to_batch(&json, &cols, &params, &schema)
527+
.expect_err(&format!("expected error for JSON {expected_label}"));
528+
let msg = err.to_string();
529+
assert!(
530+
msg.contains(expected_label),
531+
"error for {expected_label} should mention the type, got: {msg}"
532+
);
533+
}
534+
}
499535
}

crates/sqlize-core/src/datafusion/exec.rs

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,10 @@ impl ExecutionPlan for ApiTableExec {
201201
Err(e) => return Some((Err(e), state)),
202202
};
203203

204-
let data = unwrap_response(&body, &state.table.endpoint.response_wrapper_key);
204+
let data = match unwrap_response(&body, &state.table.endpoint.response_wrapper_key) {
205+
Ok(d) => d,
206+
Err(e) => return Some((Err(e), state)),
207+
};
205208

206209
let batch = match json_response_to_batch(
207210
data,
@@ -327,9 +330,44 @@ async fn fetch_page(
327330
fn unwrap_response<'a>(
328331
body: &'a serde_json::Value,
329332
wrapper_key: &Option<String>,
330-
) -> &'a serde_json::Value {
333+
) -> Result<&'a serde_json::Value, DataFusionError> {
331334
match wrapper_key {
332-
Some(field) => body.get(field.as_str()).unwrap_or(body),
333-
None => body,
335+
Some(field) => body.get(field.as_str()).ok_or_else(|| {
336+
DataFusionError::External(Box::new(std::io::Error::other(format!(
337+
"response missing expected wrapper field '{field}'"
338+
))))
339+
}),
340+
None => Ok(body),
341+
}
342+
}
343+
344+
#[cfg(test)]
345+
mod tests {
346+
use super::*;
347+
348+
#[test]
349+
fn unwrap_response_no_key_returns_body() {
350+
let body = serde_json::json!({"items": [1, 2, 3]});
351+
let result = unwrap_response(&body, &None).unwrap();
352+
assert_eq!(result, &body);
353+
}
354+
355+
#[test]
356+
fn unwrap_response_valid_key_extracts_inner() {
357+
let body = serde_json::json!({"items": [1, 2, 3], "total": 3});
358+
let result = unwrap_response(&body, &Some("items".to_owned())).unwrap();
359+
assert_eq!(result, &serde_json::json!([1, 2, 3]));
360+
}
361+
362+
#[test]
363+
fn unwrap_response_missing_key_is_error() {
364+
let body = serde_json::json!({"data": [1, 2, 3]});
365+
let err = unwrap_response(&body, &Some("items".to_owned()))
366+
.expect_err("should fail when wrapper key is absent");
367+
let msg = err.to_string();
368+
assert!(
369+
msg.contains("items"),
370+
"error should name the missing field, got: {msg}"
371+
);
334372
}
335373
}

crates/sqlize/src/mcp.rs

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,17 @@ pub struct SqlizeServer {
2121

2222
impl SqlizeServer {
2323
pub fn new(catalog_set: Arc<CatalogSet>, ctx: Arc<QueryEngine>, api_title: &str) -> Self {
24+
let multi = catalog_set.is_multi();
2425
let table_names: Vec<String> = catalog_set
2526
.all_tables()
2627
.iter()
27-
.map(|(_, t)| t.name.as_str().to_owned())
28+
.map(|(schema, t)| {
29+
if multi {
30+
format!("{schema}.{}", t.name)
31+
} else {
32+
t.name.as_str().to_owned()
33+
}
34+
})
2835
.collect();
2936
let instructions = format!(
3037
"SQLize: Query the {api_title} using SQL.\n\
@@ -76,31 +83,43 @@ impl SqlizeServer {
7683
if let Some(ddl) = self.catalog_set.describe(name) {
7784
ddl
7885
} else {
86+
let multi = self.catalog_set.is_multi();
7987
let available: Vec<String> = self
8088
.catalog_set
8189
.all_tables()
8290
.iter()
83-
.map(|(_, t)| t.name.as_str().to_owned())
91+
.map(|(schema, t)| {
92+
if multi {
93+
format!("{schema}.{}", t.name)
94+
} else {
95+
t.name.as_str().to_owned()
96+
}
97+
})
8498
.collect();
8599
format!(
86100
"Table '{name}' not found. Available tables:\n{}",
87101
available.join(", ")
88102
)
89103
}
90104
} else {
105+
let multi = self.catalog_set.is_multi();
91106
let mut out = String::from(
92107
"Available tables (use get_schema with a table name for full DDL):\n\n",
93108
);
94-
for (_, table) in self.catalog_set.all_tables() {
109+
for (schema, table) in self.catalog_set.all_tables() {
110+
let display_name = if multi {
111+
format!("{schema}.{}", table.name)
112+
} else {
113+
table.name.as_str().to_owned()
114+
};
95115
let required: Vec<_> = table.required_params().map(|c| c.name.as_str()).collect();
96116
let req = if required.is_empty() {
97117
String::new()
98118
} else {
99119
format!(" required: {}", required.join(", "))
100120
};
101121
out.push_str(&format!(
102-
" {:<30} -- {}{}\n",
103-
table.name,
122+
" {display_name:<30} -- {}{}\n",
104123
table
105124
.description
106125
.as_ref()

crates/sqlize/src/repl.rs

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -59,42 +59,51 @@ impl CatalogSet {
5959
tables
6060
}
6161

62-
/// Look up a table by name. Supports "schema.table" or bare "table" (searches all).
63-
pub fn find_table(&self, name: &str) -> Option<(&str, &VirtualTable)> {
62+
/// Look up tables by name. Supports "schema.table" (exact) or bare "table" (all catalogs).
63+
pub fn find_tables(&self, name: &str) -> Vec<(&str, &VirtualTable)> {
6464
if let Some((schema, table)) = name.split_once('.') {
65-
// Qualified: schema.table
66-
for (cat_name, catalog) in &self.entries {
67-
if cat_name == schema {
68-
if let Ok(tn) = TableName::new(table) {
69-
if let Some(t) = catalog.get(&tn) {
70-
return Some((cat_name, t));
71-
}
72-
}
73-
}
74-
}
75-
None
65+
// Qualified: schema.table — at most one match
66+
let Ok(tn) = TableName::new(table) else {
67+
return Vec::new();
68+
};
69+
self.entries
70+
.iter()
71+
.filter(|(cat_name, _)| cat_name == schema)
72+
.filter_map(|(cat_name, catalog)| catalog.get(&tn).map(|t| (cat_name.as_str(), t)))
73+
.collect()
7674
} else {
77-
// Bare: search all catalogs
78-
if let Ok(tn) = TableName::new(name) {
79-
for (cat_name, catalog) in &self.entries {
80-
if let Some(t) = catalog.get(&tn) {
81-
return Some((cat_name, t));
82-
}
83-
}
84-
}
85-
None
75+
// Bare: return all matches across catalogs
76+
let Ok(tn) = TableName::new(name) else {
77+
return Vec::new();
78+
};
79+
self.entries
80+
.iter()
81+
.filter_map(|(cat_name, catalog)| catalog.get(&tn).map(|t| (cat_name.as_str(), t)))
82+
.collect()
8683
}
8784
}
8885

8986
pub fn describe(&self, name: &str) -> Option<String> {
90-
self.find_table(name).map(|(_, t)| table_ddl(t))
87+
let matches = self.find_tables(name);
88+
if matches.is_empty() {
89+
return None;
90+
}
91+
if matches.len() == 1 && !self.is_multi() {
92+
return Some(table_ddl(matches[0].1));
93+
}
94+
// Multiple matches or multi-spec mode: prefix each with spec name
95+
let ddls: Vec<String> = matches
96+
.iter()
97+
.map(|(schema, t)| format!("-- Spec: {schema}\n{}", table_ddl(t)))
98+
.collect();
99+
Some(ddls.join("\n\n"))
91100
}
92101

93102
pub fn full_ddl(&self) -> String {
94103
let mut out = String::new();
95104
for (name, catalog) in &self.entries {
96105
if self.is_multi() {
97-
out.push_str(&format!("-- Schema: {name}\n\n"));
106+
out.push_str(&format!("-- Spec: {name}\n\n"));
98107
}
99108
out.push_str(&catalog_ddl(catalog));
100109
out.push('\n');
@@ -539,7 +548,7 @@ fn handle_show_tables(catalog_set: &CatalogSet) {
539548
let mut builder = Builder::default();
540549

541550
if catalog_set.is_multi() {
542-
builder.push_record(["schema", "table", "columns", "required", "description"]);
551+
builder.push_record(["spec", "table", "columns", "required", "description"]);
543552
} else {
544553
builder.push_record(["table", "columns", "required", "description"]);
545554
}

0 commit comments

Comments
 (0)