feat(proxy): support OPE index type#390
Open
freshtonic wants to merge 3 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces OPE (Order-Preserving Encryption) index support alongside ORE, adds new index type documentation, refactors ORE test infrastructure to use parameterized fixtures, implements comprehensive OPE ordering and WHERE clause tests, and creates corresponding database schema fixtures. ChangesOPE Index Type Support
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/sql/schema.sql (1)
172-307: Factor the ORE/OPE fixture setup into one generator.The two DO blocks duplicate the schema and search-config matrix. The next column or config change now has to stay in sync across
encrypted,encrypted_elixir, and both fixture generators, which is easy to drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sql/schema.sql` around lines 172 - 307, The two DO $$ blocks duplicate table creation and eql_v2.add_search_config calls for ORE and OPE; refactor by extracting a single generator function or DO block that accepts an index_kind parameter ('ore' or 'ope') and a list of table name suffixes, then loops once to CREATE TABLE and call eql_v2.add_search_config and eql_v2.add_encrypted_constraint accordingly; update calls that currently use the separate test_tables arrays to pass the appropriate names and index_kind to the single generator and remove the duplicated second DO block (refer to the existing identifiers: the two DO $$ blocks, the test_tables arrays, and functions eql_v2.add_search_config / eql_v2.add_encrypted_constraint to locate where to consolidate).packages/cipherstash-proxy-integration/src/map_ope_index_order.rs (1)
14-56: Add text ordering test with non-ASCII or mixed-case data to verify collation semantics.The current text ordering tests (
map_ope_order_text_ascandmap_ope_order_text_desc) use only lowercase ASCII strings (["aardvark", "aplomb", "chimera", "chrysalis", "zephyr"]). Without explicitCOLLATE "C"in the query or test data containing mixed-case or non-ASCII characters, these tests won't detect if OPE byte-order semantics diverge from PostgreSQL's default text collation. For transparent zero-change SQL semantics, consider adding a case with non-ASCII characters (e.g., accented letters) or explicitly specifying collation to make the ordering contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy-integration/src/map_ope_index_order.rs` around lines 14 - 56, Update the tests map_ope_order_text_asc and map_ope_order_text_desc to validate collation semantics by either (a) changing the values array to include mixed-case and non-ASCII strings (e.g., accented characters) so ordering compares against PostgreSQL's text ordering for those characters, or (b) explicitly specifying a deterministic byte-wise collation in the SELECT (e.g., add COLLATE "C" to the ORDER BY clause) so the OPE byte-order behavior is asserted; adjust the expected vectors accordingly (use values.iter().map(...) or values.iter().rev().map(...) as already done) and keep the insert/select logic (insert variable, SELECT encrypted_text FROM {table} ORDER BY encrypted_text ...) the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 46-50: The Cargo.toml currently uses local sibling path
dependencies cipherstash-client and cts-common which break clean CI/contributor
builds; either revert these entries to the published crates.io versions by
replacing the path entries for cipherstash-client and cts-common with the
appropriate crate version specifications, or add explicit provisioning in
CI/bootstrap to checkout the sibling cipherstash-suite repository before
building (ensuring the workspace path "../cipherstash-suite/packages/..."
exists), and remove or update the TODO comment accordingly so builds no longer
rely on an unchecked-out sibling repo.
In `@CHANGELOG.md`:
- Around line 9-11: The changelog entry includes implementation-level details;
revise the "Added" OPE entry to be release-facing by keeping only the
user-visible summary (e.g., "Added OPE (Order-Preserving Encryption) index for
range and ORDER BY queries") and remove the SQL example and internal note about
IndexType::Ope and CipherStash build requirements; move the SQL usage example
(SELECT eql_v2.add_search_config...) and the IndexType::Ope/CipherStash build
details into the docs or PR description instead so the CHANGELOG remains
high-level and user-oriented.
In `@packages/cipherstash-proxy-integration/src/common.rs`:
- Around line 94-97: The helper clear_table_with_client is interpolating the
table name directly into a TRUNCATE SQL string; instead validate/whitelist the
table identifier before formatting to prevent injection or accidental SQL
execution. Update clear_table_with_client to first check the table argument
(e.g. match against a whitelist of allowed fixture names or validate with a
strict regex like only ASCII letters/numbers/underscores) and return/raise an
error if it fails validation, and avoid using unwrap() on client.simple_query
(propagate or handle the Result); only format and execute the TRUNCATE when the
table name passes the guard.
In `@packages/cipherstash-proxy-integration/src/map_ope_index_where.rs`:
- Around line 199-206: The loop that zips actual and expected and asserts
equality per element should be replaced with a direct assert_eq!(actual,
expected, "value mismatch for {col_name} via {sql}") after the length check;
remove the for (a, e) in actual.iter().zip(expected.iter()) { assert!(a == e,
...) } block and use assert_eq! on the whole Vecs (actual and expected) to get
clearer diffs and simpler code while keeping the existing message placeholders
{col_name} and {sql}.
---
Nitpick comments:
In `@packages/cipherstash-proxy-integration/src/map_ope_index_order.rs`:
- Around line 14-56: Update the tests map_ope_order_text_asc and
map_ope_order_text_desc to validate collation semantics by either (a) changing
the values array to include mixed-case and non-ASCII strings (e.g., accented
characters) so ordering compares against PostgreSQL's text ordering for those
characters, or (b) explicitly specifying a deterministic byte-wise collation in
the SELECT (e.g., add COLLATE "C" to the ORDER BY clause) so the OPE byte-order
behavior is asserted; adjust the expected vectors accordingly (use
values.iter().map(...) or values.iter().rev().map(...) as already done) and keep
the insert/select logic (insert variable, SELECT encrypted_text FROM {table}
ORDER BY encrypted_text ...) the same.
In `@tests/sql/schema.sql`:
- Around line 172-307: The two DO $$ blocks duplicate table creation and
eql_v2.add_search_config calls for ORE and OPE; refactor by extracting a single
generator function or DO block that accepts an index_kind parameter ('ore' or
'ope') and a list of table name suffixes, then loops once to CREATE TABLE and
call eql_v2.add_search_config and eql_v2.add_encrypted_constraint accordingly;
update calls that currently use the separate test_tables arrays to pass the
appropriate names and index_kind to the single generator and remove the
duplicated second DO block (refer to the existing identifiers: the two DO $$
blocks, the test_tables arrays, and functions eql_v2.add_search_config /
eql_v2.add_encrypted_constraint to locate where to consolidate).
🪄 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: bce3ce58-efa8-4f1a-b428-54f5d387c63e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
CHANGELOG.mdCargo.tomlpackages/cipherstash-proxy-integration/src/common.rspackages/cipherstash-proxy-integration/src/lib.rspackages/cipherstash-proxy-integration/src/map_ope_index_order.rspackages/cipherstash-proxy-integration/src/map_ope_index_where.rspackages/cipherstash-proxy-integration/src/map_ore_index_order.rspackages/cipherstash-proxy-integration/src/map_ore_index_where.rspackages/cipherstash-proxy-integration/src/multitenant/ore_order.rspackages/cipherstash-proxy-integration/src/ore_order_helpers.rspackages/cipherstash-proxy/src/postgresql/context/column.rspackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/cipherstash-proxy/src/postgresql/data/to_sql.rspackages/cipherstash-proxy/src/proxy/encrypt_config/config.rstests/sql/schema.sql
Comment on lines
+9
to
+11
| ### Added | ||
|
|
||
| - **OPE (Order-Preserving Encryption) index**: New `ope` index type alongside the existing `ore` for range and ordering queries. OPE ciphertexts compare under standard lexicographic byte ordering, so they're a drop-in alternative to ORE for range and `ORDER BY`. Configure with `SELECT eql_v2.add_search_config('table', 'column', 'ope', 'int')` (any cast type that `ore` supports). Requires EQL with OPE support and a CipherStash client/config build that includes `IndexType::Ope`. |
There was a problem hiding this comment.
Make the changelog entry release-facing.
The first sentence is changelog material; the SQL example and IndexType::Ope note read like implementation details. I'd move those details into docs or the PR description so [Unreleased] stays user-facing.
As per coding guidelines, "Write changelog entries from the user's perspective, not implementation details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 9 - 11, The changelog entry includes
implementation-level details; revise the "Added" OPE entry to be release-facing
by keeping only the user-visible summary (e.g., "Added OPE (Order-Preserving
Encryption) index for range and ORDER BY queries") and remove the SQL example
and internal note about IndexType::Ope and CipherStash build requirements; move
the SQL usage example (SELECT eql_v2.add_search_config...) and the
IndexType::Ope/CipherStash build details into the docs or PR description instead
so the CHANGELOG remains high-level and user-oriented.
Comment on lines
+94
to
+97
| pub async fn clear_table_with_client(client: &Client, table: &str) { | ||
| let sql = format!("TRUNCATE {}", table); | ||
| client.simple_query(&sql).await.unwrap(); | ||
| } |
There was a problem hiding this comment.
Guard table names before building TRUNCATE SQL.
Line 95 interpolates table directly into SQL. Even in tests, this helper is pub and can execute unintended SQL if passed unexpected input. Constrain fixture table names before formatting.
Suggested hardening
pub async fn clear_table_with_client(client: &Client, table: &str) {
+ assert!(
+ table
+ .chars()
+ .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_'),
+ "invalid fixture table name: {table}"
+ );
let sql = format!("TRUNCATE {}", table);
client.simple_query(&sql).await.unwrap();
}📝 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.
Suggested change
| pub async fn clear_table_with_client(client: &Client, table: &str) { | |
| let sql = format!("TRUNCATE {}", table); | |
| client.simple_query(&sql).await.unwrap(); | |
| } | |
| pub async fn clear_table_with_client(client: &Client, table: &str) { | |
| assert!( | |
| table | |
| .chars() | |
| .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_'), | |
| "invalid fixture table name: {table}" | |
| ); | |
| let sql = format!("TRUNCATE {}", table); | |
| client.simple_query(&sql).await.unwrap(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cipherstash-proxy-integration/src/common.rs` around lines 94 - 97,
The helper clear_table_with_client is interpolating the table name directly into
a TRUNCATE SQL string; instead validate/whitelist the table identifier before
formatting to prevent injection or accidental SQL execution. Update
clear_table_with_client to first check the table argument (e.g. match against a
whitelist of allowed fixture names or validate with a strict regex like only
ASCII letters/numbers/underscores) and return/raise an error if it fails
validation, and avoid using unwrap() on client.simple_query (propagate or handle
the Result); only format and execute the TRUNCATE when the table name passes the
guard.
Comment on lines
+199
to
+206
| assert_eq!( | ||
| actual.len(), | ||
| expected.len(), | ||
| "wrong row count for {col_name} via {sql}" | ||
| ); | ||
| for (a, e) in actual.iter().zip(expected.iter()) { | ||
| assert!(a == e, "value mismatch for {col_name} via {sql}"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "map_ope_index_where.rs" | head -5Repository: cipherstash/proxy
Length of output: 129
🏁 Script executed:
if [ -f "packages/cipherstash-proxy-integration/src/map_ope_index_where.rs" ]; then
wc -l "packages/cipherstash-proxy-integration/src/map_ope_index_where.rs"
fiRepository: cipherstash/proxy
Length of output: 131
🏁 Script executed:
if [ -f "packages/cipherstash-proxy-integration/src/map_ope_index_where.rs" ]; then
sed -n '190,215p' "packages/cipherstash-proxy-integration/src/map_ope_index_where.rs" | cat -n
fiRepository: cipherstash/proxy
Length of output: 903
Replace manual equality loop with assert_eq!.
Line 205 uses assert!(a == e, ...) in a loop; prefer assert_eq! for guideline compliance and clearer failure diffs. Since both actual and expected are sorted Vec<T> with T: PartialEq, this can be simplified to a single direct comparison.
Suggested fix
assert_eq!(
actual.len(),
expected.len(),
"wrong row count for {col_name} via {sql}"
);
- for (a, e) in actual.iter().zip(expected.iter()) {
- assert!(a == e, "value mismatch for {col_name} via {sql}");
- }
+ assert_eq!(actual, expected, "value mismatch for {col_name} via {sql}");📝 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.
Suggested change
| assert_eq!( | |
| actual.len(), | |
| expected.len(), | |
| "wrong row count for {col_name} via {sql}" | |
| ); | |
| for (a, e) in actual.iter().zip(expected.iter()) { | |
| assert!(a == e, "value mismatch for {col_name} via {sql}"); | |
| } | |
| assert_eq!( | |
| actual.len(), | |
| expected.len(), | |
| "wrong row count for {col_name} via {sql}" | |
| ); | |
| assert_eq!(actual, expected, "value mismatch for {col_name} via {sql}"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cipherstash-proxy-integration/src/map_ope_index_where.rs` around
lines 199 - 206, The loop that zips actual and expected and asserts equality per
element should be replaced with a direct assert_eq!(actual, expected, "value
mismatch for {col_name} via {sql}") after the length check; remove the for (a,
e) in actual.iter().zip(expected.iter()) { assert!(a == e, ...) } block and use
assert_eq! on the whole Vecs (actual and expected) to get clearer diffs and
simpler code while keeping the existing message placeholders {col_name} and
{sql}.
coderdan
reviewed
May 4, 2026
Comment on lines
+59
to
+68
| #[tokio::test] | ||
| async fn map_ope_where_generic_text() { | ||
| map_ope_where_generic( | ||
| "encrypted_ope_where_text", | ||
| "encrypted_text", | ||
| "ABC".to_string(), | ||
| "BCD".to_string(), | ||
| ) | ||
| .await; | ||
| } |
Contributor
There was a problem hiding this comment.
We should test text with differing length strings as well.
coderdan
reviewed
May 4, 2026
Comment on lines
+64
to
+65
| "ABC".to_string(), | ||
| "BCD".to_string(), |
Contributor
There was a problem hiding this comment.
As above, we should test strings of differing lengths as well.
coderdan
reviewed
May 4, 2026
Comment on lines
+221
to
+226
| encrypted_bool eql_v2_encrypted, | ||
| encrypted_int2 eql_v2_encrypted, | ||
| encrypted_int4 eql_v2_encrypted, | ||
| encrypted_int8 eql_v2_encrypted, | ||
| encrypted_float8 eql_v2_encrypted, | ||
| encrypted_date eql_v2_encrypted, |
Contributor
There was a problem hiding this comment.
Is this adding all columns for all tables? Might be a bit redundant if so. Not a major problem.
coderdan
reviewed
May 4, 2026
Comment on lines
+49
to
+50
| cipherstash-client = { path = "../cipherstash-suite/packages/cipherstash-client" } | ||
| cts-common = { path = "../cipherstash-suite/packages/cts-common" } |
Contributor
There was a problem hiding this comment.
I'm assuming these will be updated before merge.
coderdan
approved these changes
May 4, 2026
Contributor
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Some minor suggestions.
Only blocker is to update the peer-dependencies for cipherstash-suite.
Recognises the new 'ope' index in encrypt config (alongside ore, match, unique, ste_vec) and routes it through cipherstash-client's `IndexType::Ope` so that ORE-style range/order operators work against OPE-indexed columns. - Adds `OpeIndexOpts` and `ope` field to the proxy's `Indexes` config. - Wires `Index::new_ope()` into `Column::into_column_config()`. - Adds an `encrypted_ope` integration-test table mirroring `encrypted` but with `ope`+`unique` indexes per column, and extends `clear()` to truncate it. - Adds 7 WHERE tests (int2/4/8, float8, date, text, bool) and 6 ORDER BY tests (asc/desc, NULLs first/last) targeting the new table. - Adds a `can_parse_ope_index` unit test mirroring `can_parse_ore_index`. CHANGELOG entry under [Unreleased].
Eliminates the parallel-test races that plagued the ORE WHERE/ORDER tests when run alongside each other (and the OPE tests against the shared `encrypted_ope` table). Each test now owns a dedicated table generated up front by a DO block in `tests/sql/schema.sql`, with the same shape as `encrypted` (minus jsonb) and the matching `add_search_config` calls. Other changes: - Drop `#[serial]` from `map_ore_index_order` — per-table isolation removes the need. - Drop the shared `encrypted_ope` table and the hand-added `clear_ope` helper. Tests use the new generic `clear_table(name)` to truncate their dedicated fixture. - Parameterise `ore_order_helpers` on the table name so the same helpers serve `map_ore_index_order` (per-test tables) and multitenant tests (shared `encrypted` table, isolated via keyset + `#[serial]`). End-to-end (parallel, default thread count): 38/38 ORE+OPE tests pass in ~1.6s, repeatedly. Was previously 14s serial / flaky in parallel.
Fills in two follow-up gaps from 832049a1 ("feat(ope): add ope index
type alongside ore"):
- Add `can_parse_ope_index` unit test in the encrypt-config manager,
mirroring `can_parse_ore_index`. The earlier commit's message claimed
this test was added but it wasn't.
- Document `ope` as a drop-in alternative to `ore`:
- `docs/how-to/index.md` lists `ope` alongside `match` and `ore` and
notes the operator parity ("pick ope or ore per column, not both").
- `docs/reference/searchable-json.md` updates the `MIN()/MAX()` line
to mention `ope` as well.
- `docs/sql/schema-example.sql` adds a comment near the `ore` entry
pointing to `ope` as the alternative.
freshtonic
force-pushed
the
feat/ope-index-type
branch
from
2e3c62b to
17d9de7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/cipherstash-proxy-integration/src/map_ope_index_order.rs (1)
8-106: ⚡ Quick winExtract shared OPE ordering test flow to a helper to reduce drift.
These four tests duplicate the same setup/insert/query/assert structure with only column/type/order differences. Pulling that into a small helper will make future OPE coverage additions safer and less repetitive.
Refactor sketch
+ async fn insert_interleaved<T: tokio_postgres::types::ToSql + Sync>( + client: &tokio_postgres::Client, + table: &str, + column: &str, + values: &[T], + ) { + let insert = format!("INSERT INTO {table} (id, {column}) VALUES ($1, $2)"); + for idx in interleaved_indices(values.len()) { + client + .query(&insert, &[&random_id(), &values[idx]]) + .await + .unwrap(); + } + }🤖 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 `@packages/cipherstash-proxy-integration/src/map_ope_index_order.rs` around lines 8 - 106, The four tests map_ope_order_text_asc, map_ope_order_text_desc, map_ope_order_int4_asc, and map_ope_order_int4_desc duplicate the same setup/insert/query/assert flow; extract that into a single async helper (eg. run_ope_order_test) that accepts parameters: table name (&str), column name (&str), a slice of values (generic T: ToSql + FromSql + Ord + Clone), and order flag (asc/desc) and reuses trace(), clear_table(table).await, connect_with_tls(PROXY).await, interleaved_indices, random_id, and the SELECT formatting/ordering; the helper should insert values in interleaved order, query "SELECT {column} FROM {table} ORDER BY {column} [DESC]" and build the expected vector (values or values.rev()) before asserting equality, then replace each specific test with a one-line call to that helper passing the proper table, column, values array, and order.packages/cipherstash-proxy-integration/src/map_ope_index_where.rs (2)
59-68: ⚡ Quick winText test only covers equal-length strings — add differing-length cases.
OPE text ordering for strings of different lengths (e.g.
"A"vs"ABCD", or"AB"vs"B") exercises different code paths than equal-length comparisons. This gap was noted in a previous review; the current values"ABC"/"BCD"still don't cover it.🤖 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 `@packages/cipherstash-proxy-integration/src/map_ope_index_where.rs` around lines 59 - 68, Test covers only equal-length strings; add cases with differing-length strings to exercise OPE text ordering code paths. Update the test map_ope_where_generic_text to call map_ope_where_generic additional times with mixed-length inputs such as ("A".to_string(), "ABCD".to_string()) and ("AB".to_string(), "B".to_string()) (and any other short-vs-long permutations you think relevant) so the resulting assertions in map_ope_where_generic run for differing-length comparisons.
99-99: ⚡ Quick winReplace
expect()withunwrap()per coding guidelines.
"insert failed"and"query failed"don't add context beyond what a panic backtrace already provides. The guideline reservesexpect()for messages that give meaningful diagnostic context.♻️ Proposed fix
- .expect("insert failed"); + .unwrap();- let rows = client.query(sql, params).await.expect("query failed"); + let rows = client.query(sql, params).await.unwrap();As per coding guidelines: "Use
unwrap()instead ofexpect()unless providing meaningful context in test code."Also applies to: 107-107, 193-193
🤖 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 `@packages/cipherstash-proxy-integration/src/map_ope_index_where.rs` at line 99, Replace non-informative expect() calls with unwrap() per guidelines: locate the .expect("insert failed") call (and the other expect occurrences noted at the same file, including the one around the query and the later occurrence) in map_ope_index_where.rs and change those .expect("...") invocations to .unwrap() so they no longer pass trivial strings; keep the surrounding insert/query call chains and error flow intact, only replace .expect(...) with .unwrap() for the three noted sites.packages/cipherstash-proxy-integration/src/ore_order_helpers.rs (1)
45-55: 💤 Low valueStatic analysis SQL-injection flags are false positives in this test context.
The static analysis tool flags lines 98–102 and 147–151 for
format!()-built SQL, but the same{table}interpolation pattern is used consistently across every helper in this file. Sincetableis always a string literal supplied by integration-test call sites (never derived from external/user input), there is no real injection vector here. The pattern is intentional and appropriate for parameterizing test fixture targeting.No action required, but if the project wants to silence the scanner across this entire module, a module-level
#[allow(...)]attribute or an.opengrepsuppression comment on each instance would be the minimal fix.Also applies to: 98-119, 147-158, 372-382, 414-425
🤖 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 `@packages/cipherstash-proxy-integration/src/ore_order_helpers.rs` around lines 45 - 55, Static-analysis SQL-injection warnings are false positives for the SQL built with format!() in this module (see insert_sql and sql variables and interleaved_indices usage); add a module-level suppression for the scanner (e.g., a crate/linter-specific allow attribute or a module-level comment suppression) at the top of this file that documents "table is always a test literal" and silences the SQL-injection lint for the whole module so the insert_sql/sql format!() usages are ignored by the scanner.
🤖 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 `@docs/how-to/index.md`:
- Line 231: The doc sentence advising "pick `ope` or `ore` per column, not both"
lacks guidance on how to choose; update the `ope`/`ore` section to add a short
selection guide that contrasts `ope` and `ore` by name (mention `ope` and
`ore`), summarizing performance differences (e.g., typical query/scan speed and
index size), high-level security tradeoffs (what plaintext order leakage each
reveals), and recommended use cases (when to prefer `ope` for performance vs
`ore` for stronger ordering/precision), and include a link to deeper
documentation for metrics/benchmarks and threat model details so readers can
make an informed choice.
---
Nitpick comments:
In `@packages/cipherstash-proxy-integration/src/map_ope_index_order.rs`:
- Around line 8-106: The four tests map_ope_order_text_asc,
map_ope_order_text_desc, map_ope_order_int4_asc, and map_ope_order_int4_desc
duplicate the same setup/insert/query/assert flow; extract that into a single
async helper (eg. run_ope_order_test) that accepts parameters: table name
(&str), column name (&str), a slice of values (generic T: ToSql + FromSql + Ord
+ Clone), and order flag (asc/desc) and reuses trace(),
clear_table(table).await, connect_with_tls(PROXY).await, interleaved_indices,
random_id, and the SELECT formatting/ordering; the helper should insert values
in interleaved order, query "SELECT {column} FROM {table} ORDER BY {column}
[DESC]" and build the expected vector (values or values.rev()) before asserting
equality, then replace each specific test with a one-line call to that helper
passing the proper table, column, values array, and order.
In `@packages/cipherstash-proxy-integration/src/map_ope_index_where.rs`:
- Around line 59-68: Test covers only equal-length strings; add cases with
differing-length strings to exercise OPE text ordering code paths. Update the
test map_ope_where_generic_text to call map_ope_where_generic additional times
with mixed-length inputs such as ("A".to_string(), "ABCD".to_string()) and
("AB".to_string(), "B".to_string()) (and any other short-vs-long permutations
you think relevant) so the resulting assertions in map_ope_where_generic run for
differing-length comparisons.
- Line 99: Replace non-informative expect() calls with unwrap() per guidelines:
locate the .expect("insert failed") call (and the other expect occurrences noted
at the same file, including the one around the query and the later occurrence)
in map_ope_index_where.rs and change those .expect("...") invocations to
.unwrap() so they no longer pass trivial strings; keep the surrounding
insert/query call chains and error flow intact, only replace .expect(...) with
.unwrap() for the three noted sites.
In `@packages/cipherstash-proxy-integration/src/ore_order_helpers.rs`:
- Around line 45-55: Static-analysis SQL-injection warnings are false positives
for the SQL built with format!() in this module (see insert_sql and sql
variables and interleaved_indices usage); add a module-level suppression for the
scanner (e.g., a crate/linter-specific allow attribute or a module-level comment
suppression) at the top of this file that documents "table is always a test
literal" and silences the SQL-injection lint for the whole module so the
insert_sql/sql format!() usages are ignored by the scanner.
🪄 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: 57f88174-24cd-48b4-99a5-88834edcb5b7
📒 Files selected for processing (14)
CHANGELOG.mddocs/how-to/index.mddocs/reference/searchable-json.mddocs/sql/schema-example.sqlpackages/cipherstash-proxy-integration/src/common.rspackages/cipherstash-proxy-integration/src/lib.rspackages/cipherstash-proxy-integration/src/map_ope_index_order.rspackages/cipherstash-proxy-integration/src/map_ope_index_where.rspackages/cipherstash-proxy-integration/src/map_ore_index_order.rspackages/cipherstash-proxy-integration/src/map_ore_index_where.rspackages/cipherstash-proxy-integration/src/multitenant/ore_order.rspackages/cipherstash-proxy-integration/src/ore_order_helpers.rspackages/cipherstash-proxy/src/proxy/encrypt_config/manager.rstests/sql/schema.sql
✅ Files skipped from review due to trivial changes (5)
- packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs
- packages/cipherstash-proxy-integration/src/lib.rs
- docs/reference/searchable-json.md
- CHANGELOG.md
- docs/sql/schema-example.sql
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/cipherstash-proxy-integration/src/common.rs
- packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs
- tests/sql/schema.sql
- packages/cipherstash-proxy-integration/src/map_ore_index_where.rs
- packages/cipherstash-proxy-integration/src/map_ore_index_order.rs
| The first SQL statement adds a `match` index, which is used for partial matches with `LIKE`. | ||
| The second SQL statement adds an `ore` index, which is used for ordering with `ORDER BY`. | ||
| The second SQL statement adds an `ore` index, which is used for ordering with `ORDER BY` and range comparisons (`<`, `<=`, `>`, `>=`). | ||
| The third SQL statement adds an `ope` index, which supports the same range and ordering operators as `ore` and is a drop-in alternative — pick `ope` or `ore` per column, not both. |
There was a problem hiding this comment.
Add selection criteria for choosing between ope and ore.
The documentation states that users should "pick ope or ore per column, not both" but doesn't explain when or why to choose one over the other. Users need guidance on the tradeoffs between these two encryption schemes to make an informed architectural decision.
Consider adding a brief explanation of the key differences or a link to more detailed documentation that covers:
- Performance characteristics
- Security properties
- Use case recommendations
🤖 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 `@docs/how-to/index.md` at line 231, The doc sentence advising "pick `ope` or
`ore` per column, not both" lacks guidance on how to choose; update the
`ope`/`ore` section to add a short selection guide that contrasts `ope` and
`ore` by name (mention `ope` and `ore`), summarizing performance differences
(e.g., typical query/scan speed and index size), high-level security tradeoffs
(what plaintext order leakage each reveals), and recommended use cases (when to
prefer `ope` for performance vs `ore` for stronger ordering/precision), and
include a link to deeper documentation for metrics/benchmarks and threat model
details so readers can make an informed choice.
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
2 participants
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.
This PR also includes changes to bring Proxy up to date with the refactored
cipherstash-config(which I will try to separate into its own PR).Acknowledgment
By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.
Summary by CodeRabbit
Release Notes
New Features
Documentation