feat(catalog/op_bridge): D-AR-6.2 — bridge 7 new ast::Kind variants (String/Bool/Float/Decimal/Datetime/Bytes/Uuid)#38
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…String/Bool/Float/Decimal/Datetime/Bytes/Uuid) Catches up the C16c bridge to the AST surface PR #29 added on the op-surreal-ast side. Without this, any `field_type`-derived `ast::Kind::String` (etc.) in a Schema would fail to convert to `catalog::Kind` once #29 merges. # What this adds `From<ast::Kind> for CatalogKind` gains 7 arms: ast::Kind::String → CatalogKind::String ast::Kind::Bool → CatalogKind::Bool ast::Kind::Float → CatalogKind::Float ast::Kind::Decimal → CatalogKind::Decimal ast::Kind::Datetime → CatalogKind::Datetime ast::Kind::Bytes → CatalogKind::Bytes ast::Kind::Uuid → CatalogKind::Uuid All seven are 1:1 mirrors of existing `surrealdb_core::expr::kind::Kind` variants. Option-wrapping (Rails-nullable, per codex P1 fix in PR #29) flows through the pre-existing `ast::Kind::Option(inner)` arm: `ast::Kind::String.optional()` → `catalog::Kind::Either([None, String])`. # Dep pin `op-surreal-ast` git branch ref updated from `claude/op-surreal-ast-from-triples` (PR #26 era, pre-D-AR-5.2) to `claude/op-surreal-ast-field-types-stacked` (PR #29 with the new variants). Flip to `branch = "main"` after #29 merges. # Tests `+2` new under `--features op-bridge`: - `d_ar_6_2_scalar_variants_map_one_to_one` — every new ast variant maps to the expected catalog variant. - `d_ar_6_2_option_wrapped_scalar_bridges_via_either` — the Rails-nullable Option<String> → catalog Either(None, String) chain still works for the new variants. # Forward-compat status (D-AR-6.3 deferred) The `ast::FieldDefinition.assert: Option<String>` slot added in PR #27 is still ignored by the bridge (D-AR-6.3 follow-up). Lowering a SurrealQL expression string to `catalog::Expr` requires either the surrealdb-core SurrealQL parser (heavy) or a constrained mini-parser for the few expressions the AR-shape extractor emits (`$value != NONE` is the only one today). Punted to a focused PR. # Iron-rule lock Zero new types. Zero new traits. Seven additive match arms, all 1:1 with existing catalog variants. No behavioural drift on the pre-existing 4 arms (Any/Int/Record/Option).
AdaWorldAPI
force-pushed
the
claude/op-bridge-kind-variants
branch
from
14c4b10 to
308f2e8
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14c4b10d34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # variants (String/Bool/Float/Decimal/Datetime/Bytes/Uuid) + the | ||
| # `FieldDefinition.assert` slot. Flip to `branch = "main"` after PR #29 | ||
| # merges. The crate is zero-async-deps so the pull stays cheap. | ||
| op-surreal-ast = { git = "https://github.com/AdaWorldAPI/openproject-nexgen-rs", branch = "claude/op-surreal-ast-field-types-stacked" } |
There was a problem hiding this comment.
This dependency bump selects the AST branch that now carries FieldDefinition.assert and populates it from validates_constraint, but From<ast::FieldDefinition> still only transfers the name/table/kind via .with_kind(Some(kind)). For schemas containing validations, the AST can render ASSERT $value != NONE while the bridged catalog field silently drops that clause, so the catalog accepts values that the generated schema intended to reject; please map the new assertion slot into the catalog with_assert path or avoid this branch until that is supported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in follow-up PR #39 (04fd090). From<ast::FieldDefinition> for catalog::FieldDefinition now calls .with_assert(rails_assert_to_expr(s)) so the catalog carries the same $value != NONE constraint the AST renders. Lowering is structural (Binary(Param "value", NotEqual, Literal::None)) rather than parser-based — one known AR-shape expression today, one explicit arm; unknown expressions fall back to None (safe no-op preserving the pre-PR behaviour).
PR #28's normalize: annotation that renders as $value != NONE /* normalized */ is handled by stripping the block comment + canonicalising whitespace before matching, so both forms lower to the same Expr.
Regression tests:
d_ar_6_3_field_definition_bridges_assert_claused_ar_6_3_assert_normalized_comment_is_strippedd_ar_6_3_no_assert_when_ast_field_has_noned_ar_6_3_unknown_assert_string_lowers_to_none
Thanks for the catch — declared-vs-enforced drift is exactly what the bridge exists to prevent.
… 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.
feat(catalog/op_bridge): D-AR-6.3 — lower ast::FieldDefinition.assert to catalog Expr (codex P2 from #38)
2 participants
Summary
Catches up the C16c bridge to the AST surface
AdaWorldAPI/openproject-nexgen-rs#29added on the op-surreal-astside. Without this, any
field_type-derivedast::Kind::String(etc.) in a Schema would fail to convert to
catalog::Kindonce#29 merges (exhaustive-match compile error).
What this adds
From<ast::Kind> for CatalogKindgains 7 arms:ast::Kindcatalog::KindStringStringBoolBoolFloatFloatDecimalDecimalDatetimeDatetimeBytesBytesUuidUuidAll seven are 1:1 mirrors of existing
surrealdb_core::expr::kind::Kindvariants. Option-wrapping(Rails-nullable, per codex P1 fix in nexgen-rs#29) flows through the
pre-existing
ast::Kind::Option(inner)arm:ast::Kind::String.optional()→catalog::Kind::Either([None, String]).Dep pin
op-surreal-astgit branch ref updated fromclaude/op-surreal-ast-from-triples(PR #26 era, pre-D-AR-5.2) toclaude/op-surreal-ast-field-types-stacked(nexgen-rs#29 with thenew variants). Flip to
branch = "main"after nexgen-rs#29 merges.Tests
+2 new under
--features op-bridge:d_ar_6_2_scalar_variants_map_one_to_one— every new ast variantmaps to the expected catalog variant.
d_ar_6_2_option_wrapped_scalar_bridges_via_either— theRails-nullable
Option<String>→ catalogEither(None, String)chain works for the new variants.
Forward-compat status (D-AR-6.3 deferred)
The
ast::FieldDefinition.assert: Option<String>slot added innexgen-rs#27 is still ignored by the bridge — D-AR-6.3 follow-up.
Lowering a SurrealQL expression string to
catalog::Exprrequireseither the surrealdb-core SurrealQL parser (heavy) or a constrained
mini-parser for the few expressions the AR-shape extractor emits
(
$value != NONEis the only one today). Punted to a focused PR.Iron-rule lock
Zero new types. Zero new traits. Seven additive match arms, all
1:1 with existing catalog variants. No behavioural drift on the
pre-existing 4 arms (
Any/Int/Record/Option).Test plan
cargo check -p surrealdb-core --features op-bridge→ cleancargo test -p surrealdb-core --features op-bridge -- catalog::op_bridge(test link disk-bound in current env; lib check verified the type-checker accepts the new arms against the actual op-surreal-ast deps)branch = "..."→branch = "main"