Skip to content

Commit 6351d7b

Browse files
fix(sql_validator): allow VALUES {rows} placeholder shape at load time (#118)
The runtime renderer in `substitute_sql_params` expands an array-of-arrays parameter into a multi-row tuple list (`(c1, c2), (c1, c2)`) so a pipeline like `INSERT INTO docs (...) VALUES {rows}` can batch-insert without hardcoding row count in YAML. But `preprocess_parameters` in the load-time SQL validator always substituted `{name}` with the quoted scalar literal `'__PARAM__'`, so the same template parsed as `VALUES '__PARAM__'` and sqlparser rejected it with `Expected: (, found: '__PARAM__'`. Result: skardi-server crashed during config load, the container restarted in a loop, and any pipeline using the new tuple-list shape was unloadable. Switch the placeholder to `(NULL)`, which parses both as a scalar expression (`WHERE x = (NULL)`) and as a single-row VALUES tuple (`VALUES (NULL)`), so the validator accepts every shape the runtime renderer can emit. The validator only checks DDL/access-mode restrictions, not types or arity, so the choice of literal is purely about parseability. Adds a regression test covering `VALUES {rows}` (with and without an `ON CONFLICT` tail) and confirming access-mode enforcement still fires on the tuple-list shape against a read-only table. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d548138 commit 6351d7b

1 file changed

Lines changed: 53 additions & 4 deletions

File tree

crates/skardi/src/sources/sql_validator.rs

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,21 +65,27 @@ pub fn validate_sql(sql: &str, config: &SqlValidatorConfig) -> Result<(), SqlVal
6565
}
6666

6767
fn preprocess_parameters(sql: &str) -> String {
68+
// `(NULL)` parses both as a scalar expression (e.g. `WHERE x = (NULL)`)
69+
// and as a single-row VALUES tuple (e.g. `INSERT … VALUES (NULL)`),
70+
// so the same substitution covers both `{scalar}` and `VALUES {rows}`
71+
// pipeline shapes. The runtime renderer is responsible for emitting
72+
// shape-correct SQL; this stand-in only needs to be parseable.
73+
const REPLACEMENT: &str = "(NULL)";
74+
6875
let mut result = sql.to_string();
6976
let mut start = 0;
7077

7178
while let Some(open) = result[start..].find('{') {
7279
let open = start + open;
7380
if let Some(close) = result[open..].find('}') {
7481
let close = open + close;
75-
// Replace {param_name} with a placeholder string
7682
result = format!(
77-
"{}'{}'{}",
83+
"{}{}{}",
7884
&result[..open],
79-
"__PARAM__",
85+
REPLACEMENT,
8086
&result[close + 1..]
8187
);
82-
start = open + "'__PARAM__'".len();
88+
start = open + REPLACEMENT.len();
8389
} else {
8490
break;
8591
}
@@ -387,4 +393,47 @@ mod tests {
387393
);
388394
assert!(result.is_ok());
389395
}
396+
397+
#[test]
398+
fn test_parameterized_values_tuple_list() {
399+
// The runtime renderer expands `{rows}` into a multi-row tuple list
400+
// (`(c1, c2), (c1, c2)`) for batched inserts. The validator must accept
401+
// this shape — replacing `{rows}` with a quoted scalar literal would
402+
// produce `VALUES '__PARAM__'`, which fails SQL parsing and previously
403+
// crashed config load.
404+
let config = test_config();
405+
406+
let result = validate_sql(
407+
"INSERT INTO orders (id, amount) VALUES {rows}",
408+
&config,
409+
);
410+
assert!(
411+
result.is_ok(),
412+
"VALUES {{rows}} (multi-row tuple list shape) should validate, got: {:?}",
413+
result
414+
);
415+
416+
let result = validate_sql(
417+
"INSERT INTO orders (id, embedding) VALUES {rows} ON CONFLICT (id) DO NOTHING",
418+
&config,
419+
);
420+
assert!(
421+
result.is_ok(),
422+
"VALUES {{rows}} with ON CONFLICT clause should validate, got: {:?}",
423+
result
424+
);
425+
426+
// Access-mode enforcement must still apply to the tuple-list shape.
427+
let result = validate_sql(
428+
"INSERT INTO users (id, name) VALUES {rows}",
429+
&config,
430+
);
431+
match result {
432+
Err(SqlValidationError::WriteNotAllowed { operation, table }) => {
433+
assert_eq!(operation, "INSERT");
434+
assert_eq!(table, "users");
435+
}
436+
other => panic!("Expected WriteNotAllowed for read-only table, got: {:?}", other),
437+
}
438+
}
390439
}

0 commit comments

Comments
 (0)