Skip to content

Commit 1998d10

Browse files
committed
feat(catalog/op_bridge): D-AR-6.3 — lower ast::FieldDefinition.assert to catalog Expr (codex P2 PR #38)
P2 review comment on #38: the bridge transferred name/table/kind from `ast::FieldDefinition` but silently dropped `assert: Option<String>` (added in nexgen-rs#27). Result: the catalog accepts values that the rendered SurrealQL announces it would reject — exactly the kind of declared-vs-enforced drift the C16c bridge exists to prevent. # Fix `From<ast::FieldDefinition> for catalog::FieldDefinition` now calls `with_assert(rails_assert_to_expr(s))` so the catalog carries the same constraint the AST renders. # `rails_assert_to_expr` — structural lowering The OpenProject AR-shape extractor emits exactly one expression today: `$value != NONE` (the schema-level marker for any `validates_constraint` triple; see PR #28 fix for why it's not ASSERT-on-`normalizes_attribute`). We construct it structurally as Expr::Binary { left: Param("value"), op: NotEqual, right: Literal::None, } instead of going through surrealdb-core's SurrealQL parser (which is `async` + heavy). One known shape, one explicit arm. Comment stripping: PR #28's `normalize:` annotation renders as `$value != NONE /* normalized */`. The block-comment + whitespace canonicalisation drops the marker before matching so both inputs lower to the same Expr. The marker is metadata; the assertion is unchanged. Unknown strings (e.g. a future `$value > 100 AND $value < 1000`) return `None` — the catalog stays accept-any, matching the previous (pre-D-AR-6.3) silent-drop behaviour. A future PR swapping in a real mini-parser is the natural next step when the AR-shape vocab needs richer expressions (`validates :len, length: {minimum: 3}` → `string::len($value) >= 3`). # Tests +4 new under `--features op-bridge`: - `d_ar_6_3_field_definition_bridges_assert_clause` — matches the structural Binary($value, !=, NONE) shape. - `d_ar_6_3_assert_normalized_comment_is_stripped` — the `/* normalized */` marker is metadata; the lowered Expr is the same as the bare `$value != NONE` case. - `d_ar_6_3_no_assert_when_ast_field_has_none` — passthrough preserves None. - `d_ar_6_3_unknown_assert_string_lowers_to_none` — unknown expressions don't corrupt the catalog (safety net). # Iron-rule lock §0 ANTI-INVENTION GUARDRAIL honoured: one new free fn (`rails_assert_to_expr`) over the existing `Expr` / `Param` / `Literal` types. No new variant, no new builder, no new dep.
1 parent 735265c commit 1998d10

1 file changed

Lines changed: 156 additions & 2 deletions

File tree

surrealdb/core/src/catalog/op_bridge.rs

Lines changed: 156 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ use op_surreal_ast as ast;
5555

5656
use crate::catalog::TableType as CatalogTableType;
5757
use crate::catalog::{IndexDefinition as CatalogIndexDefinition, TableDefinition};
58-
use crate::expr::{Idiom, Kind as CatalogKind};
58+
use crate::expr::operator::BinaryOperator;
59+
use crate::expr::param::Param;
60+
use crate::expr::{Expr, Idiom, Kind as CatalogKind, Literal};
5961
use crate::val::TableName;
6062

6163
use super::FieldDefinition as CatalogFieldDefinition;
@@ -144,7 +146,70 @@ impl From<ast::FieldDefinition> for CatalogFieldDefinition {
144146
let idiom = Idiom::field(f.name);
145147
let table = TableName::from(f.table);
146148
let kind: CatalogKind = f.kind.into();
147-
CatalogFieldDefinition::new_for_ddl(idiom, table).with_kind(Some(kind))
149+
// D-AR-6.3 (codex P2 PR #38): lower `ast::FieldDefinition.assert`
150+
// (a SurrealQL expression string) to a real `catalog::Expr` so the
151+
// bridged catalog field carries the same `ASSERT` clause the AST
152+
// renders. Without this, validations land in the rendered SQL but
153+
// the in-memory catalog accepts values the rendered schema would
154+
// reject.
155+
let assert = f.assert.as_deref().and_then(rails_assert_to_expr);
156+
CatalogFieldDefinition::new_for_ddl(idiom, table)
157+
.with_kind(Some(kind))
158+
.with_assert(assert)
159+
}
160+
}
161+
162+
/// Lower a SurrealQL assertion-expression string emitted by
163+
/// `op_surreal_ast::from_triples` into a structural [`Expr`] the
164+
/// catalog can store.
165+
///
166+
/// The OpenProject AR-shape extractor today emits exactly one
167+
/// expression — `$value != NONE` — as the schema-level marker for a
168+
/// `validates_constraint` triple (codex P2 PR #38 → D-AR-6.3). We
169+
/// construct that case structurally rather than going through the
170+
/// surrealdb-core SurrealQL parser, which is `async` + heavy.
171+
///
172+
/// Returns `None` for any expression the lowering doesn't recognise.
173+
/// Returning `None` is the safe fallback: the catalog field accepts
174+
/// any value, which matches the previous (pre-D-AR-6.3) behaviour of
175+
/// silently dropping the assert. A future PR can swap this in for a
176+
/// real parser when the AR-shape needs richer expressions
177+
/// (`validates :len, length: {minimum: 3}` → `string::len($value) >= 3`).
178+
///
179+
/// **Stripping whitespace + `/* ... */` comments** is intentional —
180+
/// `normalizes_attribute` (PR #28 fix) layers a structured marker on
181+
/// top of the same `$value != NONE` core, e.g.
182+
/// `$value != NONE /* normalized */`. The marker is metadata only;
183+
/// the assertion semantic is unchanged.
184+
fn rails_assert_to_expr(s: &str) -> Option<Expr> {
185+
// Strip `/* ... */` block comments anywhere in the string.
186+
let mut cleaned = String::with_capacity(s.len());
187+
let mut i = 0;
188+
let bytes = s.as_bytes();
189+
while i < bytes.len() {
190+
if i + 1 < bytes.len() && bytes[i] == b'/' && bytes[i + 1] == b'*' {
191+
// Skip until `*/`.
192+
i += 2;
193+
while i + 1 < bytes.len() && !(bytes[i] == b'*' && bytes[i + 1] == b'/') {
194+
i += 1;
195+
}
196+
i += 2;
197+
continue;
198+
}
199+
cleaned.push(bytes[i] as char);
200+
i += 1;
201+
}
202+
// Canonicalise whitespace.
203+
let canonical: String = cleaned.split_whitespace().collect::<Vec<_>>().join(" ");
204+
match canonical.as_str() {
205+
"$value != NONE" => Some(Expr::Binary {
206+
left: Box::new(Expr::Param(Param::new("value".to_string()))),
207+
op: BinaryOperator::NotEqual,
208+
right: Box::new(Expr::Literal(Literal::None)),
209+
}),
210+
// Future Rails-mapped expressions land here (one explicit arm
211+
// each; no parser glob until the AR-shape vocab demands it).
212+
_ => None,
148213
}
149214
}
150215

@@ -302,6 +367,95 @@ mod tests {
302367
assert!(has_string, "Option<String> must include String arm");
303368
}
304369

370+
/// **D-AR-6.3 (codex P2 PR #38)** — the bridge now lowers
371+
/// `ast::FieldDefinition.assert` (a string the AST renders as
372+
/// `ASSERT $value != NONE`) to a structural [`Expr`] so the
373+
/// catalog field carries the same constraint the rendered SQL
374+
/// announces.
375+
#[test]
376+
fn d_ar_6_3_field_definition_bridges_assert_clause() {
377+
let ast_field = ast::FieldDefinition::new(
378+
"subject",
379+
"WorkPackage",
380+
ast::Kind::String,
381+
)
382+
.with_assert(Some("$value != NONE".to_string()));
383+
let cat: CatalogFieldDefinition = ast_field.into();
384+
let assert = cat
385+
.assert
386+
.as_ref()
387+
.expect("assert must be set on bridged field");
388+
// Verify it's the expected Binary(Param "value", NotEqual, Literal::None).
389+
match assert {
390+
Expr::Binary { left, op, right } => {
391+
assert!(
392+
matches!(left.as_ref(), Expr::Param(p) if &**p == "value"),
393+
"expected $value param on the left arm",
394+
);
395+
assert!(matches!(op, BinaryOperator::NotEqual));
396+
assert!(matches!(right.as_ref(), Expr::Literal(Literal::None)));
397+
}
398+
other => panic!("expected Binary(...) assert; got {other:?}"),
399+
}
400+
}
401+
402+
/// **D-AR-6.3** — the `normalize:` marker from PR #28 (which
403+
/// renders as `ASSERT $value != NONE /* normalized */`) lowers
404+
/// to the SAME `$value != NONE` Expr — the comment is metadata
405+
/// only and doesn't change the semantic.
406+
#[test]
407+
fn d_ar_6_3_assert_normalized_comment_is_stripped() {
408+
let ast_field = ast::FieldDefinition::new(
409+
"email",
410+
"User",
411+
ast::Kind::String.optional(),
412+
)
413+
.with_assert(Some(
414+
"$value != NONE /* normalized */".to_string(),
415+
));
416+
let cat: CatalogFieldDefinition = ast_field.into();
417+
assert!(
418+
cat.assert.as_ref().is_some(),
419+
"normalize-annotated assert must still lower",
420+
);
421+
}
422+
423+
/// **D-AR-6.3** — an `ast::FieldDefinition.assert == None`
424+
/// produces a catalog field with no assertion (the no-validation
425+
/// path; preserves the pre-PR behaviour).
426+
#[test]
427+
fn d_ar_6_3_no_assert_when_ast_field_has_none() {
428+
let ast_field =
429+
ast::FieldDefinition::new("subject", "WorkPackage", ast::Kind::Any);
430+
let cat: CatalogFieldDefinition = ast_field.into();
431+
assert!(
432+
cat.assert.as_ref().is_none(),
433+
"expected no assert when ast field carries None",
434+
);
435+
}
436+
437+
/// **D-AR-6.3** — an unrecognised assertion string lowers to
438+
/// `None` rather than corrupting the catalog. Conservative
439+
/// safety net for future AR-shape expressions the bridge
440+
/// doesn't yet know how to lower; matches the documented
441+
/// `rails_assert_to_expr` return contract.
442+
#[test]
443+
fn d_ar_6_3_unknown_assert_string_lowers_to_none() {
444+
let ast_field = ast::FieldDefinition::new(
445+
"score",
446+
"Test",
447+
ast::Kind::Int.optional(),
448+
)
449+
.with_assert(Some(
450+
"$value > 100 AND $value < 1000".to_string(),
451+
));
452+
let cat: CatalogFieldDefinition = ast_field.into();
453+
// The bridge doesn't yet know how to lower this; rather than
454+
// dropping us into mis-construction territory, the assert
455+
// becomes None and the catalog field stays accept-any.
456+
assert!(cat.assert.as_ref().is_none());
457+
}
458+
305459
#[test]
306460
fn kind_option_nests_correctly() {
307461
let k: CatalogKind = ast::Kind::Int.optional().into();

0 commit comments

Comments
 (0)