feat(core): enhance the error message for CLAC failure#1250
feat(core): enhance the error message for CLAC failure#1250douenergy merged 4 commits intoCanner:mainfrom
Conversation
WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PySessionContext
participant AnalyzedWrenMDL
participant Context
participant ModelPlanNode
User->>PySessionContext: Create new context (with manifest, properties)
PySessionContext->>AnalyzedWrenMDL: analyze(manifest, properties, Mode)
AnalyzedWrenMDL-->>PySessionContext: analyzed_mdl
PySessionContext->>Context: create_ctx_with_mdl(analyzed_mdl, properties, Mode)
Context->>ModelPlanNode: Build plan with permission checks (if Mode::PermissionAnalyze)
ModelPlanNode-->>Context: Success or WrenError::PermissionDenied
Context-->>PySessionContext: Context or error
PySessionContext-->>User: Context or permission denied error
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningswren-core/core/src/mdl/mod.rs (4)undefined <retrieved_learning> <retrieved_learning> <retrieved_learning> <retrieved_learning> ⏰ 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). (8)
🔇 Additional comments (8)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
core
rust
goldmedal
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
wren-core-py/src/context.rs(2 hunks)wren-core-py/src/errors.rs(2 hunks)wren-core-py/tests/test_modeling_core.py(1 hunks)wren-core/benchmarks/src/tpch/run.rs(2 hunks)wren-core/benchmarks/src/wren/run.rs(2 hunks)wren-core/core/src/lib.rs(1 hunks)wren-core/core/src/logical_plan/analyze/plan.rs(2 hunks)wren-core/core/src/logical_plan/error.rs(1 hunks)wren-core/core/src/logical_plan/mod.rs(1 hunks)wren-core/core/src/mdl/context.rs(5 hunks)wren-core/core/src/mdl/mod.rs(51 hunks)wren-core/core/src/mdl/utils.rs(5 hunks)wren-core/sqllogictest/src/test_context.rs(3 hunks)wren-core/wren-example/examples/plan-sql.rs(2 hunks)wren-core/wren-example/examples/to-many-calculation.rs(2 hunks)wren-core/wren-example/examples/view.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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#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.
wren-core/core/src/logical_plan/analyze/plan.rs (2)
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.
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.
wren-core-py/tests/test_modeling_core.py (1)
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.
wren-core/core/src/logical_plan/error.rs (2)
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#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.
wren-core/core/src/mdl/context.rs (1)
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.
wren-core/core/src/mdl/mod.rs (2)
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.
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.
🧬 Code Graph Analysis (2)
wren-core-py/tests/test_modeling_core.py (3)
ibis-server/tests/routers/v3/connector/local_file/test_query.py (1)
manifest_str(72-73)wren-core-py/src/context.rs (1)
transform_sql(192-208)wren-core/core/src/mdl/mod.rs (1)
transform_sql(359-373)
wren-core/core/src/mdl/utils.rs (3)
wren-core/core/src/mdl/mod.rs (5)
mdl(195-224)new(126-181)new(529-531)analyze(74-84)default(62-70)wren-core-py/src/context.rs (2)
new(79-188)default(61-68)wren-core/core/src/mdl/context.rs (1)
new(335-357)
⏰ 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 (macos)
- GitHub Check: cargo test (macos-aarch64)
- GitHub Check: test
- GitHub Check: cargo test (win64)
- GitHub Check: cargo check
- GitHub Check: ci
🔇 Additional comments (37)
wren-core/wren-example/examples/view.rs (2)
9-9: LGTM: Correct import addition.The import of
Modeenum is appropriately placed with otherwren_coreimports.
19-19: LGTM: Appropriate mode selection for SQL unparsing.Using
Mode::Unparseis correct for this example since it demonstrates SQL transformation functionality rather than permission analysis.wren-core/wren-example/examples/plan-sql.rs (2)
7-7: LGTM: Consistent import addition.The import is correctly placed and consistent with other example files.
17-17: LGTM: Appropriate mode selection.Using
Mode::Unparseis correct for this SQL planning example.wren-core/core/src/logical_plan/mod.rs (1)
2-2: LGTM: Proper module organization.Adding the
errormodule is appropriate for centralizing permission-related error handling within the logical plan module.wren-core/core/src/lib.rs (1)
9-9: LGTM: Appropriate public re-export.Re-exporting
WrenErrorat the crate level enables external usage of the permission-related error type and follows standard Rust practices.wren-core/benchmarks/src/wren/run.rs (2)
12-12: LGTM: Consistent import addition.The import is correctly placed and follows the same pattern as other files updated in this PR.
67-67: LGTM: Appropriate mode for benchmarking.Using
Mode::Unparseis correct for this benchmark scenario focused on SQL transformation performance.wren-core/benchmarks/src/tpch/run.rs (2)
11-11: LGTM: Mode enum import added correctly.The import follows the consistent pattern established across the codebase for the Mode enum refactoring.
54-58: LGTM: Mode::Unparse usage is appropriate for benchmarking.The explicit
Mode::Unparseparameter is correctly used in the benchmark context where SQL unparsing is the primary operation being measured.wren-core/wren-example/examples/to-many-calculation.rs (2)
10-10: LGTM: Mode enum import added correctly.The import is consistent with the Mode enum refactoring across the codebase.
79-85: LGTM: Mode::LocalRuntime usage is appropriate for local execution.The explicit
Mode::LocalRuntimeparameter correctly replaces the boolean flag for local execution contexts in this example.wren-core/sqllogictest/src/test_context.rs (3)
31-31: LGTM: Mode enum import added correctly.The import is consistent with the Mode enum refactoring pattern used throughout the codebase.
308-308: LGTM: Mode::LocalRuntime usage is appropriate for test contexts.The explicit
Mode::LocalRuntimeparameter correctly replaces the boolean flag for the ecommerce test context.
544-544: LGTM: Mode::LocalRuntime usage is appropriate for test contexts.The explicit
Mode::LocalRuntimeparameter correctly replaces the boolean flag for the TPC-H test context.wren-core-py/tests/test_modeling_core.py (1)
368-376: LGTM: Permission denial test case validates the improved CLAC error messaging.This test case correctly validates the PR's main objective by ensuring that accessing a restricted column (
c_name) raises a specific permission denied error message instead of a generic "column not found" error. The expected error message format matches the improved CLAC error handling described in the PR objectives.wren-core/core/src/mdl/utils.rs (5)
270-270: LGTM: Mode enum import added correctly.The import is consistent with the Mode enum refactoring pattern used throughout the codebase.
282-286: LGTM: Mode::Unparse usage is appropriate for expression analysis tests.The explicit
Mode::Unparseparameter is correctly used in the test context where expression analysis and unparsing operations are being tested.
314-318: LGTM: Mode::Unparse usage is appropriate for expression analysis tests.Consistent with the other test functions,
Mode::Unparseis correctly used for expression analysis testing.
367-371: LGTM: Mode::Unparse usage is appropriate for expression analysis tests.The
Mode::Unparseparameter is correctly applied for model expression testing.
391-395: LGTM: Mode::Unparse usage is appropriate for expression analysis tests.The final test function correctly uses
Mode::Unparsefor remote expression testing, maintaining consistency with the other test functions.wren-core/core/src/logical_plan/error.rs (1)
1-16: LGTM! Clean and focused error type implementation.The
WrenErrorenum provides a dedicated error type for permission-related failures. The implementation follows Rust conventions with proper trait implementations and a clear user-friendly error message format.wren-core/core/src/logical_plan/analyze/plan.rs (2)
22-25: LGTM! Proper imports added for permission checking.The imports for
validate_clac_ruleandWrenErrorare correctly added to support the new permission checking functionality.
151-162: LGTM! Well-implemented permission checking logic.The permission validation logic is correctly implemented:
- Only performs checks in
PermissionAnalyzemode as documented- Returns a clear
WrenError::PermissionDeniedwith model and column information- Comments explain the mode-specific behavior differences
- Error message format is consistent with PR objectives
wren-core-py/src/context.rs (2)
161-167: LGTM! Correctly updated to use Mode enum.The
AnalyzedWrenMDL::analyzecall has been properly updated to use the newMode::Unparseenum instead of a boolean flag, which aligns with the broader refactoring effort.
177-177: LGTM! Consistent Mode enum usage.The
create_ctx_with_mdlcall correctly usesMode::Unparse, maintaining consistency with the session context initialization pattern.wren-core-py/src/errors.rs (2)
7-8: LGTM! Proper imports added for enhanced error handling.The imports for
DataFusionErrorandWrenErrorare correctly added to support the enhanced error conversion functionality.
54-65: LGTM! Enhanced error conversion for better user experience.The improved
From<DataFusionError>implementation correctly handles the nested error structure:
- Safely extracts
WrenErrorfromDataFusionError::Context->Externalchain- Uses
downcast_reffor safe type checking- Preserves original error message as fallback
- Surfaces permission-specific messages to improve user experience
This aligns perfectly with the PR objective of providing clearer permission-related error messages.
wren-core/core/src/mdl/mod.rs (5)
1-5: LGTM!The new imports for
WrenErrorandModeare properly organized and necessary for the enhanced error handling functionality.
74-84: LGTM!The function signature update to accept
Modeparameter is well-integrated and properly propagated toinfer_and_register_remote_table.
189-229: LGTM!The column validation logic correctly bypasses access control checks when in
PermissionAnalyzemode, allowing the system to differentiate between permission errors and missing column errors.
439-490: LGTM!The
permission_analyzefunction correctly implements a two-pass approach to detect permission errors. The logic properly distinguishes between general errors and permission-specific errors, providing clearer error messages to users.
546-580: LGTM!The test updates correctly use the new
Modeparameter, and the new test case properly validates that permission denied errors are surfaced with user-friendly messages.Also applies to: 2591-2619
wren-core/core/src/mdl/context.rs (4)
49-106: LGTM!The refactoring from boolean parameter to
Modeenum improves code clarity and extensibility. The mode-based rule selection is well-structured.
108-161: LGTM!The
Modeenum is well-designed with clear documentation and appropriate helper methods. The encapsulation of mode-specific rule selection improves maintainability.
278-298: LGTM!The
analyze_rule_for_permissionfunction correctly includes only the essential analyzer rules needed for permission checking, avoiding unnecessary transformations.
335-357: LGTM!The
WrenDataSourceconstructor correctly implements mode-aware column filtering, bypassing access control checks inPermissionAnalyzemode to enable proper permission error detection.
| Ok(plan) => plan, | ||
| Err(e) => { | ||
| debug!("Failed to create logical plan: {e}"); | ||
| return Ok(()); |
There was a problem hiding this comment.
Error will be handled in AnalyzedWrenMDL.analyze()
2 participants
Description
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Documentation