chore(core): enhance error messages for access controls check#1278
chore(core): enhance error messages for access controls check#1278douenergy merged 2 commits intoCanner:mainfrom
Conversation
WalkthroughThis change updates error messages in row-level access control (RLAC) logic to include the rule name, improving clarity and traceability. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AccessControl
participant ModelGen
participant Plan
Caller->>AccessControl: validate_rule(name, required_properties, headers)
AccessControl-->>Caller: Result (error message includes rule name)
ModelGen->>AccessControl: validate_rule(&rule.name, &rule.required_properties, self.properties)
AccessControl-->>ModelGen: Result (error message includes rule name)
Plan->>AccessControl: validate_rule(&rule.name, &rule.required_properties, &self.properties)
AccessControl-->>Plan: Result (error message includes rule name)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
wren-core/core/src/mdl/mod.rs (2)
1936-1939: Prefer consistent wording for all rule-name messagesThe new wording is fine, but to avoid awkward sentences when the rule name itself already ends with “rule” (see below) consider the more neutral pattern
session property <prop> is required by rule \<rule_name>` but not found in headers`This keeps the grammar uniform and eliminates a hard-coded “rule” suffix that may duplicate the word later.
2025-2028: Apply the same wording guideline hereSame comment as above – switching to “…required by rule `name`…” makes the error copy consistent across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wren-core/core/src/logical_plan/analyze/access_control.rs(19 hunks)wren-core/core/src/logical_plan/analyze/model_generation.rs(1 hunks)wren-core/core/src/logical_plan/analyze/plan.rs(1 hunks)wren-core/core/src/mdl/mod.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: goldmedal
PR: Canner/wren-engine#1161
File: wren-core/core/src/logical_plan/analyze/access_control.rs:0-0
Timestamp: 2025-04-30T01:15:15.009Z
Learning: In the row-level access control implementation, separate error checks are maintained for different failure modes (missing property vs null vs empty) to provide more precise and actionable error messages, even if it means slightly more verbose code with multiple Option checks.
Learnt from: goldmedal
PR: Canner/wren-engine#1250
File: wren-core/core/src/mdl/mod.rs:391-417
Timestamp: 2025-07-11T02:36:24.323Z
Learning: In Wren Engine's column-level access control (CLAC) implementation, the `permission_analyze` function is used to provide better error messages when SQL planning fails. If permission_analyze succeeds (Ok), the original planning error is returned. If permission_analyze fails (Err), the permission error is returned instead, providing more specific error messages about access control violations.
Learnt from: goldmedal
PR: Canner/wren-engine#1161
File: wren-core/core/src/logical_plan/analyze/access_control.rs:0-0
Timestamp: 2025-04-30T01:18:21.776Z
Learning: In the `collect_condition` function of the row-level access control implementation, compound identifiers are intentionally ignored rather than causing failures when processing expressions. This is by design as confirmed by the team.
📚 Learning: the row-level access control implementation in wren engine filters headers with the prefix `x_wren_v...
Learnt from: goldmedal
PR: Canner/wren-engine#1161
File: ibis-server/app/routers/v3/connector.py:78-83
Timestamp: 2025-05-05T02:27:29.829Z
Learning: The row-level access control implementation in Wren Engine filters headers with the prefix `X_WREN_VARIABLE_PREFIX` in `EmbeddedEngineRewriter.get_session_properties` and validates session property expressions in `access_control.rs` to ensure they only contain literal values, preventing SQL injection.
Applied to files:
wren-core/core/src/logical_plan/analyze/model_generation.rswren-core/core/src/logical_plan/analyze/plan.rswren-core/core/src/logical_plan/analyze/access_control.rswren-core/core/src/mdl/mod.rs
📚 Learning: in the row-level access control implementation, separate error checks are maintained for different f...
Learnt from: goldmedal
PR: Canner/wren-engine#1161
File: wren-core/core/src/logical_plan/analyze/access_control.rs:0-0
Timestamp: 2025-04-30T01:15:15.009Z
Learning: In the row-level access control implementation, separate error checks are maintained for different failure modes (missing property vs null vs empty) to provide more precise and actionable error messages, even if it means slightly more verbose code with multiple Option checks.
Applied to files:
wren-core/core/src/logical_plan/analyze/model_generation.rswren-core/core/src/logical_plan/analyze/plan.rswren-core/core/src/logical_plan/analyze/access_control.rswren-core/core/src/mdl/mod.rs
📚 Learning: in wren engine's column-level access control (clac) implementation, the `permission_analyze` functio...
Learnt from: goldmedal
PR: Canner/wren-engine#1250
File: wren-core/core/src/mdl/mod.rs:391-417
Timestamp: 2025-07-11T02:36:24.323Z
Learning: In Wren Engine's column-level access control (CLAC) implementation, the `permission_analyze` function is used to provide better error messages when SQL planning fails. If permission_analyze succeeds (Ok), the original planning error is returned. If permission_analyze fails (Err), the permission error is returned instead, providing more specific error messages about access control violations.
Applied to files:
wren-core/core/src/logical_plan/analyze/model_generation.rswren-core/core/src/logical_plan/analyze/access_control.rswren-core/core/src/mdl/mod.rs
📚 Learning: in the `collect_condition` function of the row-level access control implementation, compound identif...
Learnt from: goldmedal
PR: Canner/wren-engine#1161
File: wren-core/core/src/logical_plan/analyze/access_control.rs:0-0
Timestamp: 2025-04-30T01:18:21.776Z
Learning: In the `collect_condition` function of the row-level access control implementation, compound identifiers are intentionally ignored rather than causing failures when processing expressions. This is by design as confirmed by the team.
Applied to files:
wren-core/core/src/logical_plan/analyze/model_generation.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: cargo test (win64)
- GitHub Check: cargo test (macos)
- GitHub Check: cargo check
- GitHub Check: cargo test (macos-aarch64)
- GitHub Check: test
- GitHub Check: ci
🔇 Additional comments (7)
wren-core/core/src/logical_plan/analyze/plan.rs (1)
409-409: LGTM: Enhanced error reporting with rule nameThe addition of
&rule.nameas the first parameter tovalidate_rulecorrectly implements the enhanced error messaging described in the PR objectives. This will provide more informative error messages when access control validation fails by including the specific rule name in the error output.wren-core/core/src/logical_plan/analyze/model_generation.rs (1)
274-274: LGTM: Consistent implementation of enhanced error reportingThe addition of
&rule.nameas the first parameter tovalidate_ruleis consistent with the changes inplan.rsand correctly implements the enhanced error messaging for row-level access control validation. This ensures that error messages will include the specific rule name, making debugging access control issues more straightforward.wren-core/core/src/logical_plan/analyze/access_control.rs (5)
86-86: LGTM! Enhanced error messages with rule name.The extraction of the rule name and its inclusion in error messages provides better context for debugging access control issues, which aligns perfectly with the PR objectives.
Also applies to: 101-108
147-150: LGTM! Consistent error message enhancement across all validation scenarios.All session property validation error messages now consistently include the rule name using the same formatting pattern, providing clear context for each type of validation failure.
Also applies to: 156-159, 165-168, 178-181
288-288: LGTM! Correctly updated for new validate_rule signature.The call now properly passes the CLAC rule name to maintain consistency with the enhanced error messaging pattern.
395-496: LGTM! Comprehensive test updates for enhanced error messages.All test cases have been systematically updated to:
- Pass the rule name parameter to
validate_rulecalls- Update assertion snapshots to expect rule names in error messages
- Maintain test coverage while validating the enhanced error messaging functionality
The test updates are thorough and consistent with the implementation changes.
Also applies to: 610-610, 630-630, 789-789
245-245: Allvalidate_rulecall sites updated withnameparameterVerified that every invocation of
validate_rulenow includes the rule name as its first argument:
- wren-core/core/src/logical_plan/analyze/access_control.rs: lines 288, 400–404, 411–415, 422–426, 541–553
- wren-core/core/src/logical_plan/analyze/plan.rs: line 409
- wren-core/core/src/logical_plan/analyze/model_generation.rs: line 274
No further changes required.
|
Thanks @goldmedal |
2 participants
Description
Show the rule name in the error messages
Summary by CodeRabbit
Bug Fixes
Tests