test(eql-v3): pin ->/->> bare-literal domain-flattening; fix misleading comment#318
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Re-verify the eql_v3 implementation audit against the current tree and correct drift introduced by the 3.0.0 overhaul (single self-contained installer; eql_v2 removed): - §1/§6: eql_v2 is removed, not 'the documented public API, unchanged'; reframe the ORE-comparator note as historical fork provenance. - §5: replace the 4-variant build table with the single-artifact build; re-cite build.sh and rename pin_search_path.sql -> pin_search_path_v3.sql. - §3.3: drop stale 'Supabase variant' exclusion; explain the functional- index rationale that made the subset build redundant. - §4.3: v3_ste_vec.sql is now generated/gitignored, not a committed fixture. - §2.4: fix drifted scalar_domains.rs / spec.rs line citations. De-enumerate snapshot baseline counts so they can't rot: CLAUDE.md and audit §4.2 now point at tests/sqlx/snapshots/README.md as the source of truth, and that README now documents matrix_jsonb_entry_tests.txt.
tobyhede
force-pushed
the
test/v3-jsonb-bare-operand-fallthrough
branch
from
9e069ab to
4843600
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/sqlx/tests/v3_jsonb_tests.rs`:
- Around line 879-924: The test function
v3_jsonb_bare_operand_flattens_to_native is using a synthetic hand-curated JSON
payload (NN_DOC) which violates SQLx testing guidelines requiring real encrypted
data. Replace the NN_DOC constant with actual encrypted test data sourced from
fixtures generated via mise run test:sqlx:prep, ensuring the document is sourced
from real crypto-generated terms rather than manual JSON blobs. Apply this same
fixture-based approach to the other test function mentioned in the comment range
(around lines 941-1008).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 641d3477-3244-4454-bc43-929ddae6f199
📒 Files selected for processing (5)
CLAUDE.mdsrc/v3/jsonb/operators.sqltests/sqlx/snapshots/README.mdtests/sqlx/snapshots/v3_jsonb_tests.txttests/sqlx/tests/v3_jsonb_tests.rs
Comment on lines
+879
to
+924
| async fn v3_jsonb_bare_operand_flattens_to_native(pool: PgPool) -> anyhow::Result<()> { | ||
| let doc = format!("'{}'::eql_v3.json", NN_DOC); | ||
|
|
||
| // `?` is blocked with a typed RHS in D7 (`question`). Bare `'sv'` is unknown | ||
| // -> native `jsonb ? text` -> top-level key present -> TRUE, no raise. | ||
| let bare_question: bool = sqlx::query_scalar(&format!("SELECT {doc} ? 'sv'")) | ||
| .fetch_one(&pool) | ||
| .await?; | ||
| assert!( | ||
| bare_question, | ||
| "bare `?` must resolve to native `jsonb ? text` (top-level key 'sv' is \ | ||
| present in NN_DOC -> true); a raise would mean it reached our blocker, \ | ||
| breaking the documented domain-flattening contract" | ||
| ); | ||
|
|
||
| // Same operator, TYPED RHS -> our blocker raises. Proves the divergence is | ||
| // real (this is the D7 `question` case, re-asserted here to keep the | ||
| // bare/typed contrast in one place). | ||
| eql_tests::assert_raises( | ||
| &pool, | ||
| &format!("SELECT {doc} ? 'sv'::text"), | ||
| &[], | ||
| "is not supported", | ||
| ) | ||
| .await?; | ||
|
|
||
| // `||` is blocked with a typed RHS in D7 (`concat`). Bare `'{}'` is unknown | ||
| // -> native `jsonb || jsonb` -> merged object, no raise. | ||
| let bare_concat: String = sqlx::query_scalar(&format!("SELECT ({doc} || '{{}}')::text")) | ||
| .fetch_one(&pool) | ||
| .await?; | ||
| assert!( | ||
| bare_concat.contains("\"sv\""), | ||
| "bare `||` must resolve to native `jsonb || jsonb` and return the merged \ | ||
| document, got {bare_concat:?}" | ||
| ); | ||
| eql_tests::assert_raises( | ||
| &pool, | ||
| &format!("SELECT {doc} || '{{}}'::jsonb"), | ||
| &[], | ||
| "is not supported", | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use generated encrypted fixtures instead of synthetic NN_DOC in these new SQLx tests.
These tests are built around a hand-curated JSON payload, which conflicts with the SQLx test-data requirement and weakens confidence that behavior is validated against real crypto-generated terms. Please source the test document from generated fixtures prepared via mise run test:sqlx:prep.
As per coding guidelines, tests/sqlx/**/*.rs tests must use real encrypted data from actual crypto and must not use synthetic blobs; and fixtures should be generated via mise run test:sqlx:prep. Based on learnings, use mise run test:sqlx:prep before running the SQLx suite.
Also applies to: 941-1008
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/sqlx/tests/v3_jsonb_tests.rs` around lines 879 - 924, The test function
v3_jsonb_bare_operand_flattens_to_native is using a synthetic hand-curated JSON
payload (NN_DOC) which violates SQLx testing guidelines requiring real encrypted
data. Replace the NN_DOC constant with actual encrypted test data sourced from
fixtures generated via mise run test:sqlx:prep, ensuring the document is sourced
from real crypto-generated terms rather than manual JSON blobs. Apply this same
fixture-based approach to the other test function mentioned in the comment range
(around lines 941-1008).
Sources: Coding guidelines, Learnings
…ng comment The `eql_v3.json` domain flattens to native `jsonb` when an operator's RHS is an unknown-typed literal, so a bare `col -> 'sel'` binds the NATIVE `jsonb -> text` (a root-key lookup on the envelope) instead of the v3 selector-lookup operator — a silent wrong answer for direct-SQL callers (the Proxy is unaffected because it always sends typed `$n`). This is intrinsic to the domain type-kind and cannot be closed by an extra operator/blocker; it can only be pinned. - Re-land v3_jsonb_bare_operand_flattens_to_native (blocker face: `?` / `||` succeed as native on a bare RHS, raise on a typed RHS), recovered from the dropped commit 817a9660. - Add v3_jsonb_arrow_bare_operand_flattens_to_native (supported-operator face): assert via pg_typeof which operator binds AND the user-visible value divergence, so a resolution change in either direction goes red. Verified empirically against PG 17 (bare `-> 'sv'` -> jsonb `[]`; typed `-> 'sv'::text` -> eql_v3.ste_vec_entry NULL). - Regenerate snapshots/v3_jsonb_tests.txt (74 -> 76) for test:v3-jsonb:inventory. - Fix the inline comment in operators.sql (integer `->` overload): it claimed a bare `e -> 'sv'` would bind the v3 operator — empirically false and contrary to the file's own @warning. Bare binds native; the custom operator does not capture an untyped literal. Comment-only, no behaviour change.
tobyhede
force-pushed
the
test/v3-jsonb-bare-operand-fallthrough
branch
from
4843600 to
678e80f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Pins the
eql_v3.jsondomain-flattening behaviour for the->/->>operators and corrects a misleading source comment that stated the opposite of the real behaviour.eql_v3.jsonis a DOMAIN overjsonb. When an operator's RHS is an unknown-typed literal, PostgreSQL reduces the domain to its base type and the native operator wins the exact-match tiebreak. So a barecol -> 'sel'binds nativejsonb -> text(a root-key lookup on the envelope) instead of the v3 selector-lookup operator — a silent wrong answer for direct-SQL callers. The CipherStash Proxy is unaffected because it always sends typed$nparameters.This is intrinsic to the domain type-kind and cannot be closed by an extra operator/blocker — it can only be pinned so a future resolution change is caught.
Changes
v3_jsonb_bare_operand_flattens_to_native(blocker face:?/||succeed as native on a bare RHS, raise on a typed RHS) — recovered from dropped commit817a9660.v3_jsonb_arrow_bare_operand_flattens_to_native(supported-operator face): asserts viapg_typeofwhich operator binds and the user-visible value divergence, so a resolution change in either direction goes red.snapshots/v3_jsonb_tests.txt(74 → 76) fortest:v3-jsonb:inventory.->overload (src/v3/jsonb/operators.sql): it claimed a baree -> 'sv'would bind the v3 operator — empirically false and contrary to the file's own@warning. Comment-only, no behaviour change.Verification (PG 17)
bare -> 'sv'→pg_typeof=jsonb, value[](native root-key lookup)typed -> 'sv'::text→pg_typeof=eql_v3.ste_vec_entry, valueNULLtest:self_contained_v3andtest:v3-jsonb:inventorygates green.Note
This pins the behaviour — it does not (and cannot) close the underlying finding-#1 footgun. Direct-SQL callers writing the bare form still get native semantics with no error; the mitigation remains "Proxy always sends typed
$n".Summary by CodeRabbit
Tests
Documentation