Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a preferred Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application/Client
participant ConfigF as eql_v2.config()
participant Validator as eql_v2.config_check_cast()
participant DB as Database
App->>ConfigF: Request config for table/column
ConfigF->>DB: SELECT tbl_config FROM eql_v2_configuration
DB-->>ConfigF: Return config JSON
ConfigF->>ConfigF: resolved_type = COALESCE(tbl_config->>'plaintext_type', tbl_config->>'cast_as')
ConfigF->>Validator: Validate(resolved_type)
Validator->>Validator: Check against allowed types {text,int,small_int,big_int,real,double,boolean,date,jsonb,json,float,decimal,timestamp}
alt Type Valid
Validator-->>ConfigF: true
ConfigF-->>App: Return decrypts_as = resolved_type
else Type Invalid
Validator-->>ConfigF: RAISE exception
ConfigF-->>App: Return configuration error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
freshtonic
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/config/constraints.sql (1)
78-79: Centralize the cast-type allowlist to prevent validator drift.This list is duplicated here and in
src/config/functions.sql(Line 49). If they diverge,add_search_config()and table constraints can disagree on valid input.♻️ Suggested direction
- _valid_types text[] := '{text, int, small_int, big_int, real, double, boolean, date, jsonb, json, float, decimal, timestamp}'; + _valid_types text[] := eql_v2.valid_cast_types();Add a shared helper (for example in a common SQL file) and reuse it in both validators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/constraints.sql` around lines 78 - 79, The _valid_types array declared in constraints.sql is duplicated with the allowlist used by add_search_config() (in functions.sql), so centralize the list by extracting it into a single reusable helper (e.g., a SQL function or constant view like get_allowed_cast_types() or allowed_casts() in a common SQL file) and replace the local _valid_types declaration in constraints.sql and the hardcoded list in add_search_config() with calls to that helper; ensure the helper returns the same text[] or setof text shape required by both the table constraint logic and add_search_config() so both validators reference the single source of truth.tests/sqlx/tests/config_tests.rs (1)
503-545: Add a precedence test forcast_asvsplaintext_type.These tests validate alias support, but they don’t lock in the documented rule that
cast_aswins when both are present.🧪 Suggested test addition
+#[sqlx::test(fixtures(path = "../fixtures", scripts("config_tables")))] +async fn configuration_prefers_cast_as_over_plaintext_type(pool: PgPool) -> Result<()> { + sqlx::query("TRUNCATE TABLE eql_v2_configuration") + .execute(&pool) + .await?; + + let config = serde_json::json!({ + "v": 1, + "tables": { + "users": { + "email": { + "cast_as": "int", + "plaintext_type": "text", + "indexes": { "unique": {} } + } + } + } + }); + + sqlx::query("INSERT INTO eql_v2_configuration (data) VALUES ($1::jsonb)") + .bind(&config) + .execute(&pool) + .await?; + + let decrypts_as: Option<String> = sqlx::query_scalar( + "SELECT decrypts_as FROM eql_v2.config() WHERE relation = 'users' AND col_name = 'email'", + ) + .fetch_one(&pool) + .await?; + + assert_eq!(decrypts_as.as_deref(), Some("int")); + Ok(()) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sqlx/tests/config_tests.rs` around lines 503 - 545, Add a precedence test (or extend configuration_accepts_plaintext_type_field) to ensure when both "cast_as" and "plaintext_type" are provided the view returns "cast_as" (i.e., cast_as wins): create a config JSON that includes both fields for the same column (e.g., "email": { "cast_as": "varchar", "plaintext_type": "text", ... }), insert it the same way using sqlx::query(...).bind(&config).execute(&pool).await, then query eql_v2.config() like the existing test and assert decrypts_as equals the cast_as value ("varchar"); reference the existing test function configuration_accepts_plaintext_type_field and the query selecting decrypts_as to locate where to add this assertion/test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-04-01-accept-canonical-type-names.md`:
- Line 13: Update the machine-specific home-directory reference in the markdown
line that reads
`~/cipherstash/cipherstash-suite/docs/plans/2026-04-01-canonical-encryption-config-design.md`
to a repo-relative path so others/CI can resolve it (e.g., replace the
`~/cipherstash/cipherstash-suite/...` fragment with
`docs/plans/2026-04-01-canonical-encryption-config-design.md`); locate the
string in docs/plans/2026-04-01-accept-canonical-type-names.md and change it
accordingly.
- Around line 17-277: The markdown file uses inconsistent heading levels
(jumping from # to ###) which triggers markdownlint MD001; update each "Task 1",
"Task 2", etc. section headers currently written as "### Task X: ..." to use "##
Task X: ..." (and adjust any subheadings under them if necessary) so headings
increment properly; ensure all top-level task headings are exactly two hashes
and scan the document for any other heading level jumps to normalize the
hierarchy.
---
Nitpick comments:
In `@src/config/constraints.sql`:
- Around line 78-79: The _valid_types array declared in constraints.sql is
duplicated with the allowlist used by add_search_config() (in functions.sql), so
centralize the list by extracting it into a single reusable helper (e.g., a SQL
function or constant view like get_allowed_cast_types() or allowed_casts() in a
common SQL file) and replace the local _valid_types declaration in
constraints.sql and the hardcoded list in add_search_config() with calls to that
helper; ensure the helper returns the same text[] or setof text shape required
by both the table constraint logic and add_search_config() so both validators
reference the single source of truth.
In `@tests/sqlx/tests/config_tests.rs`:
- Around line 503-545: Add a precedence test (or extend
configuration_accepts_plaintext_type_field) to ensure when both "cast_as" and
"plaintext_type" are provided the view returns "cast_as" (i.e., cast_as wins):
create a config JSON that includes both fields for the same column (e.g.,
"email": { "cast_as": "varchar", "plaintext_type": "text", ... }), insert it the
same way using sqlx::query(...).bind(&config).execute(&pool).await, then query
eql_v2.config() like the existing test and assert decrypts_as equals the cast_as
value ("varchar"); reference the existing test function
configuration_accepts_plaintext_type_field and the query selecting decrypts_as
to locate where to add this assertion/test.
🪄 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: 3793fb43-4b1c-44a9-bee1-d36dc4680f02
📒 Files selected for processing (5)
docs/plans/2026-04-01-accept-canonical-type-names.mddocs/reference/index-config.mdsrc/config/constraints.sqlsrc/config/functions.sqltests/sqlx/tests/config_tests.rs
tobyhede
approved these changes
Apr 1, 2026
Add json, float, decimal, and timestamp as valid cast types. Support plaintext_type as a canonical alias for cast_as in configuration, with the config() view resolving either field via COALESCE. Fix inverted validation logic in config_check_cast and ambiguous alias in config().
The COALESCE argument order in eql_v2.config() was checking cast_as before plaintext_type, meaning the deprecated field would win when both were present. Swap the order so plaintext_type takes precedence. Add test verifying precedence behavior and update docs to clarify plaintext_type is preferred and cast_as is a deprecated alias.
Not needed in the final PR.
freshtonic
force-pushed
the
refactor/canonical-config
branch
3 times, most recently
from
df237ea to
11dba92
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-eql.yml:
- Around line 15-17: The env block contains an invalid YAML entry using
shell-style assignment for MISE_VERBOSE; update the env mapping so both entries
are valid YAML key: value pairs (e.g., keep FORCE_JAVASCRIPT_ACTIONS_TO_NODE24:
true and change MISE_VERBOSE to a YAML mapping like MISE_VERBOSE: "1" or
MISE_VERBOSE: 1) ensuring proper indentation and no shell-style equals sign.
🪄 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: f9d8fb2b-aec7-4a99-b3a3-72a6aba14b3c
📒 Files selected for processing (3)
.github/workflows/release-eql.ymlmise.tomltests/sqlx/tests/config_tests.rs
✅ Files skipped from review due to trivial changes (2)
- mise.toml
- tests/sqlx/tests/config_tests.rs
freshtonic
force-pushed
the
refactor/canonical-config
branch
3 times, most recently
from
dcb7228 to
d271eed
Compare
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Unit tests committed locally. Commit: |
freshtonic
force-pushed
the
refactor/canonical-config
branch
from
ac9c030 to
d271eed
Compare
Add test that configures one column with cast_as and another with plaintext_type to ensure both resolve correctly through eql_v2.config().
Python 3.13.12 installation was missing a lib directory, causing CI failures. Upgrading to 3.14 resolves the issue. Also ensure consistent mise GH Action version.
freshtonic
force-pushed
the
refactor/canonical-config
branch
from
d271eed to
c40537d
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
3 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.
feat(config): accept canonical type names and plaintext_type alias
Add json, float, decimal, and timestamp as valid cast types. Support plaintext_type as a canonical alias for cast_as in
configuration, with the config() view resolving either field via COALESCE. Fix inverted validation logic in
config_check_cast and ambiguous alias in config().
Summary by CodeRabbit
New Features
Documentation
Tests
Chores