test(v3): property-based tests for the eql_v3 encrypted scalar domains (CIP-3141)#275
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:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@CHANGELOG.md`:
- Line 32: The changelog entry currently ends with an issue link "(CIP-3141)";
update the tail of that CHANGELOG.md paragraph to reference the PR link instead
of the issue link by replacing the CIP-3141 URL with the pull request URL for
this change (use the PR number/URL for the merged PR), preserving the existing
text and parentheses and keeping the "Why" explanation intact so the entry is
user-facing and follows the project's changelog guideline.
In `@crates/eql-scalars/src/proptest_invariants.rs`:
- Around line 95-108: The test every_catalog_domain_payload_keys_match_its_terms
currently only checks that every term's json_key is present in
Term::term_json_keys(dom.terms) but doesn't fail on extra keys; change it to
assert exact set equality by building the expected set from dom.terms (e.g.,
collect t.json_key() for each t into a HashSet or sorted Vec) and compare it to
the actual set returned by Term::term_json_keys(dom.terms), using equality (==)
in the assert with a clear message referencing spec.domain_name(dom) so the test
fails if there are any missing or extra keys.
In `@tests/sqlx/src/property.rs`:
- Around line 169-176: The error context currently interpolates the full
DATABASE_URL in connect_pool which can leak credentials; change connect_pool to
build a redacted URL before passing it to with_context by parsing the url string
(e.g., using url::Url::parse or equivalent), replacing or removing the password
component (set password to "<redacted>" or omit it) and then use that redacted
string in the with_context closure (keep PgPool::connect(&url).await as-is but
call with_context(|| format!("connecting property-test pool to {}",
redacted_url))).
In `@tests/sqlx/tests/encrypted_domain/property/live_oracle.rs`:
- Around line 22-37: The encrypt_rows function currently zips
values.iter().cloned().zip(payloads) which can silently drop entries if
encrypt_store returned a different number of payloads; before zipping, compare
values.len() with payloads.len() and return a failure (Err) when they differ to
fail fast and surface fixture/encryption mismatches; update encrypt_rows to use
the lengths check on values and payloads (from encrypt_store) and only proceed
to build Vec<Row<T>> when counts match, referencing encrypt_rows, encrypt_store,
payloads, values, and Row.
🪄 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: b92a070f-2eff-467e-900a-1ee49263c154
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
CHANGELOG.mdcrates/eql-scalars/Cargo.tomlcrates/eql-scalars/src/lib.rscrates/eql-scalars/src/proptest_invariants.rsmise.tomltests/sqlx/Cargo.tomltests/sqlx/README.mdtests/sqlx/src/lib.rstests/sqlx/src/property.rstests/sqlx/tests/encrypted_domain.rstests/sqlx/tests/encrypted_domain/property/edge_cases.rstests/sqlx/tests/encrypted_domain/property/fixture_oracle.rstests/sqlx/tests/encrypted_domain/property/live_oracle.rstests/sqlx/tests/encrypted_domain/property/mod.rs
| - **`eql_v3.timestamptz` encrypted-domain type family (equality-only).** Two jsonb-backed domains for encrypted `timestamptz` columns — `eql_v3.timestamptz` (storage-only) and `eql_v3.timestamptz_eq` (`=` / `<>` via HMAC) — generated from the `timestamptz` row in `eql-scalars::CATALOG` by the same materializer as the `eql_v3.date` family. Values are **UTC-normalized** (cipherstash has no timezone-preserving type): plaintexts encrypt under the `timestamp` cast. Index via a functional index on the `eql_v3.eq_term` extractor, not an operator class on the domain. **Ordering (`<` `<=` `>` `>=`, `MIN` / `MAX`) is deferred:** cipherstash encrypts `Plaintext::Timestamp` at native 12-block ORE width, but EQL's only ORE comparator (`eql_v2.compare_ore_block_u64_8_256_term`) is hardcoded to 8 blocks, so ordered timestamptz domains would silently mis-order. There are no `eql_v3.timestamptz_ord` / `_ord_ore` domains and no timestamptz `MIN` / `MAX` aggregates until a wide-ORE (12-block) term lands — tracked in [#241](https://github.com/cipherstash/encrypt-query-language/issues/241). Why: a type-safe, equality-searchable encrypted UTC-timestamp column, stacking on the `date` temporal-scalar foundation; ordering follows once the comparator supports the native ciphertext width. ([#257](https://github.com/cipherstash/encrypt-query-language/pull/257)) | ||
| - **Per-domain `MIN` / `MAX` aggregates for the encrypted-domain family.** `eql_v3.min(eql_v3.<T>_ord)` / `eql_v3.max(eql_v3.<T>_ord)` (and the `_ord_ore` twin) are generated for every ord-capable scalar variant, giving type-safe extrema on domain-typed columns — comparison routes through the variant's `<` / `>` operator (ORE block term, no decryption). The aggregates are declared `PARALLEL = SAFE` with a combine function (the state function itself — min/max are associative), so PostgreSQL can use partial/parallel aggregation on large `GROUP BY` workloads. Why: the new domain types previously had no equivalent of the composite-type aggregates. The existing `eql_v2.min(eql_v2_encrypted)` / `eql_v2.max(eql_v2_encrypted)` aggregates are **retained** and continue to work on `eql_v2_encrypted` columns; the per-domain aggregates are additive and coexist with them. ([#239](https://github.com/cipherstash/encrypt-query-language/pull/239)) | ||
| - **`eql_v3.text` encrypted-domain family (`text`, `text_eq`, `text_match`, `text_ord`, `text_ord_ore`).** Adds equality (`=` / `<>` via HMAC), match (`@>` / `<@` via a new self-contained `eql_v3.bloom_filter` SEM index term), and ORE ordering (`<` `<=` `>` `>=`, `min` / `max`) for encrypted text, at parity with EQL v2 text — generated from the `text` row in `eql-scalars::CATALOG` by the same materializer as the `eql_v3.int4` reference. `text` is the first scalar to add a new index `Term` (`Bloom`) and the first non-integer, unbounded ordered kind (lexicographic pivots, hand-written `impl ScalarType`). Index via a functional index on the `eql_v3.eq_term` / `eql_v3.ord_term` / `eql_v3.match_term` extractors, not an operator class on the domain. Why: brings searchable encrypted text to the namespaced, `eql_v2`-free `eql_v3` surface. Match is exposed as bloom-filter containment on the `text_match` domain — deliberately *not* SQL `LIKE` (no wildcard/anchoring; probabilistic ngram containment) — and never backs equality (which always routes through `Hm`). ([#260](https://github.com/cipherstash/encrypt-query-language/pull/260)) | ||
| - **Property-based tests for the `eql_v3` encrypted scalar domains.** A three-tier harness asserts SQL operator results agree with a plaintext oracle across a generated input space: a pure-Rust catalog-invariant tier (no database), a fixture-corpus tier that samples the live-encrypted fixtures and checks all ordered pairs in each sampled corpus, and a live-encryption tier (gated behind the `proptest-live` cargo feature) that batch-encrypts freshly generated plaintexts each run. Covers the equality (`=`/`<>`) and ordering (`<`/`<=`/`>`/`>=`, `ord_term` sort order) oracles plus NULL/blocker/CHECK edge cases. Why: the prior matrix exercised fixed pivots only; property tests catch operator/oracle disagreements across the whole value space. ([CIP-3141](https://linear.app/cipherstash/issue/CIP-3141)) |
There was a problem hiding this comment.
Use a PR link (not issue link) in the changelog entry tail.
Line 32 currently ends with a CIP issue link; the guideline requires a PR link in parentheses for user-facing entries.
Suggested edit
-... Why: the prior matrix exercised fixed pivots only; property tests catch operator/oracle disagreements across the whole value space. ([CIP-3141](https://linear.app/cipherstash/issue/CIP-3141))
+... Why: the prior matrix exercised fixed pivots only; property tests catch operator/oracle disagreements across the whole value space. ([`#275`](https://github.com/cipherstash/encrypt-query-language/pull/275))As per coding guidelines, "When making a user-facing change, write the CHANGELOG.md entry ... then a short 'Why' explanation, then a PR link in parentheses."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Property-based tests for the `eql_v3` encrypted scalar domains.** A three-tier harness asserts SQL operator results agree with a plaintext oracle across a generated input space: a pure-Rust catalog-invariant tier (no database), a fixture-corpus tier that samples the live-encrypted fixtures and checks all ordered pairs in each sampled corpus, and a live-encryption tier (gated behind the `proptest-live` cargo feature) that batch-encrypts freshly generated plaintexts each run. Covers the equality (`=`/`<>`) and ordering (`<`/`<=`/`>`/`>=`, `ord_term` sort order) oracles plus NULL/blocker/CHECK edge cases. Why: the prior matrix exercised fixed pivots only; property tests catch operator/oracle disagreements across the whole value space. ([CIP-3141](https://linear.app/cipherstash/issue/CIP-3141)) | |
| - **Property-based tests for the `eql_v3` encrypted scalar domains.** A three-tier harness asserts SQL operator results agree with a plaintext oracle across a generated input space: a pure-Rust catalog-invariant tier (no database), a fixture-corpus tier that samples the live-encrypted fixtures and checks all ordered pairs in each sampled corpus, and a live-encryption tier (gated behind the `proptest-live` cargo feature) that batch-encrypts freshly generated plaintexts each run. Covers the equality (`=`/`<>`) and ordering (`<`/`<=`/`>`/`>=`, `ord_term` sort order) oracles plus NULL/blocker/CHECK edge cases. Why: the prior matrix exercised fixed pivots only; property tests catch operator/oracle disagreements across the whole value space. ([`#275`](https://github.com/cipherstash/encrypt-query-language/pull/275)) |
🤖 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 `@CHANGELOG.md` at line 32, The changelog entry currently ends with an issue
link "(CIP-3141)"; update the tail of that CHANGELOG.md paragraph to reference
the PR link instead of the issue link by replacing the CIP-3141 URL with the
pull request URL for this change (use the PR number/URL for the merged PR),
preserving the existing text and parentheses and keeping the "Why" explanation
intact so the entry is user-facing and follows the project's changelog
guideline.
Source: Coding guidelines
- property.rs: redact userinfo from DATABASE_URL in connect_pool error context so a connection failure never logs the password (CodeRabbit Major). - proptest_invariants.rs: assert exact payload-key set equality (catches extra keys, not just missing ones). - live_oracle.rs: ensure! encrypt_store returns one payload per plaintext before zipping, so a count mismatch fails fast instead of silently truncating. - CHANGELOG.md: use the PR link (#275) instead of the CIP issue link.
CodeRabbit Autofix — Fixes AppliedApplied 4 validated fixes from CodeRabbit review feedback (commit Files modified:
Validation: |
- property.rs: redact userinfo from DATABASE_URL in connect_pool error context so a connection failure never logs the password (CodeRabbit Major). - proptest_invariants.rs: assert exact payload-key set equality (catches extra keys, not just missing ones). - live_oracle.rs: ensure! encrypt_store returns one payload per plaintext before zipping, so a count mismatch fails fast instead of silently truncating. - CHANGELOG.md: use the PR link (#275) instead of the CIP issue link.
ba5b73e to
b0e323b
Compare
- property.rs: redact userinfo from DATABASE_URL in connect_pool error context so a connection failure never logs the password (CodeRabbit Major). - proptest_invariants.rs: assert exact payload-key set equality (catches extra keys, not just missing ones). - live_oracle.rs: ensure! encrypt_store returns one payload per plaintext before zipping, so a count mismatch fails fast instead of silently truncating. - CHANGELOG.md: use the PR link (#275) instead of the CIP issue link.
a0602dd to
07dd0f6
Compare
…P-3141) Load each fixture corpus into the shared connect_pool DB on demand (ensure_fixture_loaded), since fixtures.eql_v2_<T> otherwise exist only in sqlx::test ephemeral databases, not the base DB the property tiers connect to.
Also commit Cargo.lock (proptest deps) and apply cargo fmt to the property-test modules.
…eferral, CHECK specificity) + clippy cleanup (CIP-3141)
- Add ->/@> blocker raises + not-STRICT tests on int4_eq (the documented
native-jsonb domain-fallback footgun); previously only < was covered.
- Assert timestamptz ordering deferral: < on timestamptz_eq raises.
- Tighten CHECK tests from is_err() to assert_raises("violates check
constraint"); add storage-envelope and _ord_ore CHECK cases.
- Clear clippy type_complexity (OrdRow alias) and needless_lifetimes;
derive Clone on Row<T> (drops the dead modulo in pick).
…t representatives (CIP-3141) The behavioural raise tests cover <, ->, @> on int4_eq; this adds a catalog-level test that asserts the non-STRICT + LANGUAGE plpgsql contract for all 977 generated blockers across every eql_v3 domain (->, ->>, @>, <@, ||, ?, ?|, ?&, @?, @@, #>, #>>, -, #-, comparisons), against the installed catalog. Closes the gap where a blocker regressing to STRICT/LANGUAGE sql on an operator other than < would pass the suite.
The repo is public and accepts fork PRs (approval policy only gates first-time contributors), so the prior comment claiming "this repository does not accept fork PRs" was false and the env block left the CipherStash credentials reachable from fork-PR runs if the fork-secrets toggle were ever enabled. Add a job-level `if:` that runs the secret-bearing test job only for push / workflow_dispatch / same-repo branch PRs, and correct the comment.
- property.rs: redact userinfo from DATABASE_URL in connect_pool error context so a connection failure never logs the password (CodeRabbit Major). - proptest_invariants.rs: assert exact payload-key set equality (catches extra keys, not just missing ones). - live_oracle.rs: ensure! encrypt_store returns one payload per plaintext before zipping, so a count mismatch fails fast instead of silently truncating. - CHANGELOG.md: use the PR link (#275) instead of the CIP issue link.
The eql_v3 rebase added a `token: &str` parameter to `Variant::supports_ord`, but `assert_ord_oracle` still called it tokenless, so the eql_tests lib did not compile. Pass `T::PG_TYPE`.
…find it The fixture-oracle property tests loaded their corpus at runtime via std::fs::read_to_string keyed off CARGO_MANIFEST_DIR/fixtures/eql_v2_<T>.sql. In CI's archive->shard split the test shards do a fresh checkout where those gitignored fixtures are absent, so int2/text (and every Tier A oracle test) failed with "No such file or directory". Only 3 surfaced because nextest fail-fast cancelled the shard after 93/518. Embed the fixture SQL into the test binary via include_str! at compile time, exactly like sqlx::test's scripts(...) does, so the corpus travels inside the prebuilt nextest archive. The embed lives in the fixture_oracle test target (not the eql_tests lib) so generate_all_fixtures still compiles before the fixtures exist. ensure_fixture_loaded now takes the script as a parameter. No CI or catalog changes.
Replace the abstract 'Tier A/B/C' labels with concrete names matching what each suite does and where it lives: - Tier C -> catalog (unit, pure Rust, no DB) - Tier A -> fixture (integration, committed ciphertext) - Tier B -> e2e (integration, fresh ZeroKMS->Postgres each run) Full-depth rename: live_oracle.rs -> e2e_oracle.rs, cargo feature proptest-live -> proptest-e2e (Cargo.toml, mise.toml, #[cfg]), run_live_property -> run_e2e_property, *_oracle_live -> *_oracle_e2e, DB tables proptest_live_* -> proptest_e2e_*, plus all prose in comments, README, CHANGELOG and the implementation plan. Unrelated 'Tier 1/2' usages (benchmarks, pg_stat_statements, jsonb builders) left untouched. Test-only; no behaviour change.
Add a README for the eql_v3 property tests describing the three suites, the shared all-pairs oracle engine, why ciphertext can't be Arbitrary-derived, and the conventions/footguns (not under scalars::, e2e gated behind proptest-e2e, shrinking disabled for e2e). Reference it from CLAUDE.md's Testing section.
…(CIP-3141)
The fixture/e2e property oracles connect via connect_pool() to the base
DATABASE_URL database, not through #[sqlx::test]'s migrated per-test scratch
DBs. Under the sharded build-once-archive CI, a shard's base DB is a stock
Postgres with no EQL installed (only build-archive ran `sqlx migrate run`,
against a different Postgres), so every `::eql_v3.<T>_eq` cast raised
`schema "eql_v3" does not exist`. The old per-version CI happened to install
EQL into the base DB via test:sqlx:prep's migrate step, which the suites
silently relied on; the rebase onto the new CI removed that, turning the
suites red. (The failures looked type-specific — int2/int8/text/date/
timestamptz — but that was just nextest fail-fast + shard distribution; the
error is type-agnostic, int4 included.)
Make the suites self-sufficient: ensure_eql_installed() applies the embedded
001_install_eql.sql installer to the connected DB on first use, guarded by a
once-per-process async mutex and an `eql_v3.int4_eq` presence check (so a
developer's pre-installed local DB is left untouched — the installer is not
idempotent). The installer is include_str!'d in the test target (mod.rs), the
same archive-travel mechanism the fixture corpus uses, kept out of the lib so
clippy/test:crates contexts don't need the generated file.
Also stop swallowing the real Postgres error: the proptest runners rendered
anyhow errors with {e}/to_string(), dropping the cause chain and leaving only
the "pair query: SELECT …" context line — which is what made this opaque in
CI. Render with {e:#} to keep the full chain.
Verified: against a fresh DB with no eql_v3, all 11 fixture_oracle tests
(including the ones that failed in CI) now pass and the surface is installed
on demand; against an already-installed DB the presence check skips the install.
…ller (CIP-3141) c012fdc made the property suites self-install eql_v3 with an embedded copy of the release SQL guarded by a hand-rolled advisory lock. That raced: nextest runs each test in its OWN process, several hit the empty shard DB at once, and concurrent `CREATE SCHEMA eql_v3` failed with `duplicate key … pg_namespace`. The lock attempt was reinventing — incorrectly — what sqlx already does. Use the real thing: `ensure_eql_installed` now runs `sqlx::migrate!("./migrations")` against the base pool — the SAME embedded migration set `#[sqlx::test]` applies to every other test in the suite. `Migrator::run` records applied versions in `_sqlx_migrations` and skips them (idempotent, so a developer's pre-migrated local DB is a no-op) and holds a database-level advisory lock for the run, so the per-test processes serialise: exactly one applies each migration, the rest observe it already applied. No bespoke installer, no bespoke lock. The migrator is built in the test target (property/mod.rs `migrator()`) so the lib never embeds the gitignored generated `001_install_eql.sql`. Keeps the `{e:#}` change from c012fdc that surfaces anyhow's cause chain (it is what revealed the duplicate-key error in the first place). Compiles clean; CI's sharded nextest run (process-per-test) is the verification that the migrator serialises the concurrent install.
b41e03b to
7725810
Compare
…olation (CIP-3141) The fixture property suite connected to one shared base DB via connect_pool(). nextest runs each test in its own process, so sibling tests raced on every shared write: first the eql_v3 install (CREATE SCHEMA eql_v3), then — once the migrator fixed that — the fixture load (CREATE SCHEMA IF NOT EXISTS fixtures, which is not concurrency-safe in Postgres), plus a load-vs-read hazard where a later test re-DROP/CREATEs a fixture table out from under an earlier test's in-flight oracle SELECTs. Locking each write was whack-a-mole on a shared-DB design every other test in the suite avoids. Run each fixture property under #[sqlx::test] instead, like the rest of the suite (incl. the sibling edge_cases.rs): each test gets its own migrated scratch DB (eql_v3 already installed) and loads the committed fixture corpus into that isolated DB. No shared state, no install-on-demand, no advisory locks, no load-vs-read hazard — the whole race class is gone. proptest's case loop is synchronous and can't take #[sqlx::test]'s injected async pool, which is why the suite used a raw pool originally. `drive_proptest` bridges them: the proptest runner lives on a dedicated OS thread that ships each generated case to the async side over a channel and blocks for the verdict; the async side runs the oracle on the injected pool and replies. The pool never crosses runtimes, it works under any runtime flavour, and shrinking is preserved. The e2e suite is unchanged: it batch-encrypts via ZeroKMS, runs single-process (gated behind proptest-e2e, not in the nextest shards), and keeps using connect_pool + the migrator (now gated to that feature so it isn't dead code in the default shard build). Removed the now-unused ensure_fixture_loaded + its per-process guard from the lib. Verified: all 11 fixture_oracle tests pass under `cargo nextest run` (process-per-test, the model that failed in CI).
- property.rs: redact userinfo from DATABASE_URL in connect_pool error context so a connection failure never logs the password (CodeRabbit Major). - proptest_invariants.rs: assert exact payload-key set equality (catches extra keys, not just missing ones). - live_oracle.rs: ensure! encrypt_store returns one payload per plaintext before zipping, so a count mismatch fails fast instead of silently truncating. - CHANGELOG.md: use the PR link (#275) instead of the CIP issue link.
test(v3): property-based tests for the eql_v3 encrypted scalar domains (CIP-3141)
- property.rs: redact userinfo from DATABASE_URL in connect_pool error context so a connection failure never logs the password (CodeRabbit Major). - proptest_invariants.rs: assert exact payload-key set equality (catches extra keys, not just missing ones). - live_oracle.rs: ensure! encrypt_store returns one payload per plaintext before zipping, so a count mismatch fails fast instead of silently truncating. - CHANGELOG.md: use the PR link (#275) instead of the CIP issue link.
What
Property-based tests for the
eql_v3encrypted scalar domains: SQL operator results are asserted to agree with a plaintext oracle over a generated input space — reaching operator/oracle disagreements the fixed-pivot matrix can't.Property coverage
The harness is three property suites plus an example-based edge-case suite — one unit-level catalog suite (no DB) and two integration suites, fixture (committed ciphertext) and e2e (fresh end-to-end encryption each run):
proptest_invariants.rsplpgsql; payload-key set == declared termsproperty/fixture_oracle.rs=<><<=>>=+ord_termsort, over every ordered pairproperty/e2e_oracle.rs(proptest-e2e)property/edge_cases.rs->/@>domain-fallback); timestamptz ordering deferral; CHECK rejects malformed payloadThe fixture, e2e and edge-case suites share one all-pairs oracle engine; the fixture suite generalises across the whole catalog, and the e2e suite is the only one that can hit "same plaintext, different ciphertext" (fixtures have no duplicates).
Notes
TestRunner—#[sqlx::test]'s injected pool isn't usable from a syncproptest!body. Evaluation is read-only, so no per-test schema isolation;connect_pool()+ a race-safe once-per-type fixture load into the shared DB.property::, notscalars::— the matrix-inventory gate reads everyscalars::<X>::prefix as a scalar type.Follow-ups
Origin: CIP-3141.
Summary by CodeRabbit
Tests