fix: update … --all-rows falls back to the DSL instead of misparsing (ADR-0033 Am4)
Advanced-mode `update T set x = 42 --all-rows` parsed the `--all-rows`
DSL flag as the arithmetic `42 - -all - rows` over phantom columns
`all`/`rows` (Amendment 3's counter-example), masked only by the engine's
`--` comment leniency. The playground supports no `--` line comment, so
this was a misparse (ADR-0027: flag input known to fail at runtime).
Fix: walk_punct refuses a `-` that begins an adjacent `--`. Only the SQL
expression uses Node::Punct('-'), so this is scoped to it. The SET
expression then stops, the SQL UPDATE shape fails, and dispatch falls
back to the DSL Update { AllRows } — symmetry with delete … --all-rows.
Behaviour: `42 --all-rows` → DSL Update{AllRows}; spaced `42 - -3` stays
SqlUpdate (= 45, preserved); adjacent `42--3` → parse error (contrived;
no `--` comment support).
Tests: inverted parse test (+ arithmetic-preserved + adjacent-error
assertions); new full-pipeline update_all_rows_flag_in_advanced_updates_every_row.
Suite 1963/0/1; clippy clean.
This commit is contained in:
@@ -356,6 +356,21 @@ fn walk_punct(
|
|||||||
) -> NodeWalkResult {
|
) -> NodeWalkResult {
|
||||||
let bytes = source.as_bytes();
|
let bytes = source.as_bytes();
|
||||||
if position < bytes.len() && bytes[position] == ch as u8 {
|
if position < bytes.len() && bytes[position] == ch as u8 {
|
||||||
|
// ADR-0033 Amendment 4: a `-` does not match when it begins an
|
||||||
|
// adjacent `--`. The playground supports no `--` line comment, and
|
||||||
|
// `--` is the DSL flag marker (e.g. `--all-rows`); treating `--`
|
||||||
|
// as two minus operators silently mis-parsed `set x = 42
|
||||||
|
// --all-rows` as arithmetic over phantom columns `all`/`rows`.
|
||||||
|
// Refusing here makes the SQL expression stop, so the SQL shape
|
||||||
|
// fails and dispatch falls back to the DSL flag. Spaced `- -3` is
|
||||||
|
// unaffected (the dashes are not adjacent). `Node::Punct('-')` is
|
||||||
|
// used only by the SQL expression grammar, so this is scoped to it.
|
||||||
|
if ch == '-' && bytes.get(position + 1) == Some(&b'-') {
|
||||||
|
return NodeWalkResult::NoMatch {
|
||||||
|
position,
|
||||||
|
expected: vec![Expectation::Punct(ch)],
|
||||||
|
};
|
||||||
|
}
|
||||||
path.push(MatchedItem {
|
path.push(MatchedItem {
|
||||||
kind: MatchedKind::Punct(ch),
|
kind: MatchedKind::Punct(ch),
|
||||||
text: ch.to_string(),
|
text: ch.to_string(),
|
||||||
|
|||||||
+28
-8
@@ -31,7 +31,9 @@ use rdbms_playground::app::App;
|
|||||||
use rdbms_playground::db::{Database, DbError, DeleteResult, InsertResult, UpdateResult};
|
use rdbms_playground::db::{Database, DbError, DeleteResult, InsertResult, UpdateResult};
|
||||||
use rdbms_playground::dsl::parser::parse_command_in_mode;
|
use rdbms_playground::dsl::parser::parse_command_in_mode;
|
||||||
use rdbms_playground::dsl::walker::Severity;
|
use rdbms_playground::dsl::walker::Severity;
|
||||||
use rdbms_playground::dsl::{ColumnSpec, Command, ReferentialAction, Type, parse_command};
|
use rdbms_playground::dsl::{
|
||||||
|
ColumnSpec, Command, ReferentialAction, RowFilter, Type, parse_command,
|
||||||
|
};
|
||||||
use rdbms_playground::event::AppEvent;
|
use rdbms_playground::event::AppEvent;
|
||||||
use rdbms_playground::mode::Mode;
|
use rdbms_playground::mode::Mode;
|
||||||
use rdbms_playground::persistence::Persistence;
|
use rdbms_playground::persistence::Persistence;
|
||||||
@@ -513,18 +515,36 @@ fn e2e_single_dml_statement_with_trailing_semicolon_parses() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn e2e_update_all_rows_in_advanced_does_not_fall_back_to_dsl() {
|
fn e2e_update_all_rows_in_advanced_falls_back_to_dsl() {
|
||||||
// ADR-0033 Amendment 3 counter-example: the SQL `UPDATE`'s
|
// ADR-0033 Amendment 4 reverses Amendment 3's counter-example: the
|
||||||
// `SET <expr>` absorbs `--all-rows` (as `42 - -all - rows`), so
|
// SQL `UPDATE`'s `SET` expression must NOT consume the DSL flag
|
||||||
// the SQL shape matches and there is no DSL fall-back. (Harmless
|
// `--all-rows`. An adjacent `--` is not two minus operators — the
|
||||||
// at execution — the engine treats `--all-rows` as a line
|
// playground has no `--` line comment — so the SQL shape fails and
|
||||||
// comment.)
|
// dispatch falls back to the DSL `Update { AllRows }`, mirroring
|
||||||
|
// `delete … --all-rows`.
|
||||||
assert!(
|
assert!(
|
||||||
matches!(
|
matches!(
|
||||||
parse_command_in_mode("update Orders set total = 42 --all-rows", Mode::Advanced),
|
parse_command_in_mode("update Orders set total = 42 --all-rows", Mode::Advanced),
|
||||||
|
Ok(Command::Update { filter: RowFilter::AllRows, .. })
|
||||||
|
),
|
||||||
|
"advanced `update … --all-rows` falls back to the DSL Update",
|
||||||
|
);
|
||||||
|
// Legitimate spaced arithmetic is unaffected — the dashes are not
|
||||||
|
// adjacent, so this stays a SQL UPDATE (total = 42 - (-3) = 45).
|
||||||
|
assert!(
|
||||||
|
matches!(
|
||||||
|
parse_command_in_mode("update Orders set total = 42 - -3", Mode::Advanced),
|
||||||
Ok(Command::SqlUpdate { .. })
|
Ok(Command::SqlUpdate { .. })
|
||||||
),
|
),
|
||||||
"advanced `update … --all-rows` stays SQL (no DSL fall-back)",
|
"spaced `42 - -3` stays a SQL UPDATE",
|
||||||
|
);
|
||||||
|
// An adjacent `--` before a number is no longer silently accepted as
|
||||||
|
// arithmetic; with no `--all-rows` flag to fall back to, it is a
|
||||||
|
// parse error (acceptable per Amendment 4 — contrived input, and the
|
||||||
|
// playground does not support `--` comments).
|
||||||
|
assert!(
|
||||||
|
parse_command_in_mode("update Orders set total = 42--3", Mode::Advanced).is_err(),
|
||||||
|
"adjacent `42--3` is a parse error (no `--` comment support)",
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -488,3 +488,38 @@ fn advanced_update_set_expression_still_parses_via_sql_expr() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// =================================================================
|
||||||
|
// ADR-0033 Amendment 4 — `update … --all-rows` falls back to the DSL
|
||||||
|
// =================================================================
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn update_all_rows_flag_in_advanced_updates_every_row() {
|
||||||
|
// `update … --all-rows` falls back to the DSL Update { AllRows } in
|
||||||
|
// advanced mode (run_replay parses each line in advanced mode) and
|
||||||
|
// updates every row — the full pipeline end to end, not just the
|
||||||
|
// parse-level dispatch (covered in tests/sql_dml_e2e.rs).
|
||||||
|
let (project, db, _dir) = open_project_db();
|
||||||
|
let rt = rt();
|
||||||
|
create_cols(&db, &rt, "t", &[("id", Type::Int), ("v", Type::Int)], &["id"]);
|
||||||
|
seed(&db, &rt, "insert into t (id, v) values (1, 1), (2, 2)", "t");
|
||||||
|
std::fs::write(
|
||||||
|
project.path().join("allrows.commands"),
|
||||||
|
"update t set v = 9 --all-rows\n",
|
||||||
|
)
|
||||||
|
.expect("write script");
|
||||||
|
let events = rt.block_on(run_replay(&db, project.path(), "allrows.commands"));
|
||||||
|
assert!(
|
||||||
|
matches!(events.last(), Some(AppEvent::ReplayCompleted { count: 1, .. })),
|
||||||
|
"the --all-rows update replays through the DSL fall-back; events: {events:?}"
|
||||||
|
);
|
||||||
|
let rows = rt
|
||||||
|
.block_on(db.query_data("t".to_string(), None, None, None))
|
||||||
|
.expect("query")
|
||||||
|
.rows;
|
||||||
|
assert_eq!(rows.len(), 2, "both rows present");
|
||||||
|
assert!(
|
||||||
|
rows.iter().all(|r| r[1].as_deref() == Some("9")),
|
||||||
|
"every row's v set to 9 (all-rows update); rows: {rows:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user