fix(core): fix the required session properties for CLAC#1287
fix(core): fix the required session properties for CLAC#1287douenergy merged 3 commits intoCanner:mainfrom
Conversation
WalkthroughAdds redacted request-header logging, threads session properties into local query runs and context creation, updates RLAC tests to verify case-insensitive requiredProperties and column gating, and changes MDL analysis to return detailed errors and use provided properties when creating execution context. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ibis-server
participant wren-core
participant MDL_Analyzer
participant ExecutionCtx
Client->>ibis-server: Send query + headers (session properties)
ibis-server->>ibis-server: Log method/path/body and redacted headers
ibis-server->>wren-core: Create SessionContext(manifest, session, properties)
wren-core->>MDL_Analyzer: analyze(manifest, properties, Unparse)
alt Analysis OK
MDL_Analyzer-->>wren-core: analyzed_mdl
wren-core->>ExecutionCtx: create_ctx_with_mdl(analyzed_mdl, properties)
ExecutionCtx-->>ibis-server: Execution result
ibis-server-->>Client: 200 + data
else Analysis Error
MDL_Analyzer-->>wren-core: Error(e)
wren-core-->>ibis-server: "Failed to analyze MDL: e"
ibis-server-->>Client: Error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
✨ 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/Issue comments)Type 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 (3)
ibis-server/tools/query_local_run.py (1)
73-79: Session properties wiring looks good; consider sourcing from an env var for local runs.Current code always passes an empty properties set. For local experimentation with CLAC/RLAC, reading from an env var (JSON) is convenient and avoids code edits.
Apply this diff to load properties from an optional JSON env var and normalize keys to lowercase:
-# Set the session properties here. It should be lowercase. -properties = {} -properties = frozenset(properties.items()) -print("### Session Properties ###") -for key, value in properties: - print(f"# {key}: {value}") -session_context = SessionContext(encoded_str, function_list_path + f"/{data_source}.csv", properties) +# Set the session properties here. Keys will be normalized to lowercase. +# Optionally supply JSON via WREN_SESSION_PROPERTIES_JSON, e.g.: +# export WREN_SESSION_PROPERTIES_JSON='{"session_level":"2","session_user":"Customer#000000001"}' +properties_json = os.getenv("WREN_SESSION_PROPERTIES_JSON") +properties = json.loads(properties_json) if properties_json else {} +properties = {str(k).lower(): (None if v is None else str(v)) for k, v in properties.items()} +properties_fs = frozenset(properties.items()) +print("### Session Properties ###") +for key, value in properties_fs: + print(f"# {key}: {value}") +session_context = SessionContext( + encoded_str, + function_list_path + f"/{data_source}.csv", + properties_fs, +)wren-core-py/src/context.rs (1)
168-171: Outdated comment contradicts the new behavior; update or remove.The comment says headers won’t be used and an empty map is provided, but you now pass the actual properties map to the context.
Apply this diff to correct the comment:
- // the headers won't be used in the context. Provide an empty map. + // Use the provided session properties when creating the execution context.ibis-server/tests/routers/v3/connector/postgres/test_query.py (1)
631-682: CLAC test with requiredProperties is solid; consider adding a negative case.Great coverage for requiredProperties + case-insensitive names. Optionally, add a test where the required property is missing to verify the expected behavior (e.g., column hidden vs. explicit error), aligned with product/spec intent.
Would you like me to add a follow-up test asserting behavior when a required property is absent?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
ibis-server/app/middleware/__init__.py(1 hunks)ibis-server/tests/routers/v3/connector/postgres/test_query.py(2 hunks)ibis-server/tools/query_local_run.py(1 hunks)wren-core-py/src/context.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
ibis-server/tests/routers/v3/connector/postgres/test_query.py (2)
ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/postgres/conftest.py (1)
connection_info(45-52)
wren-core-py/src/context.rs (4)
wren-core/core/src/mdl/mod.rs (4)
analyze(74-84)mdl(195-224)new(126-181)new(529-531)wren-core-base/manifest-macro/src/lib.rs (1)
manifest(26-56)wren-core/core/src/mdl/context.rs (2)
new(339-361)create_ctx_with_mdl(49-110)wren-core-py/src/errors.rs (11)
new(17-21)from(25-27)from(31-33)from(37-39)from(43-45)from(49-51)from(55-64)from(68-70)from(74-76)from(80-82)from(86-88)
ibis-server/tools/query_local_run.py (2)
wren-core/core/src/mdl/context.rs (1)
properties(86-92)wren-core/core/src/mdl/mod.rs (1)
data_source(318-320)
⏰ 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). (2)
- GitHub Check: ci
- GitHub Check: test
🔇 Additional comments (2)
wren-core-py/src/context.rs (1)
161-176: Correctly analyze MDL with provided properties and propagate errors.Good fix: you now analyze with the provided properties and pass them into the context, and you surface analyze errors instead of panicking.
Also applies to: 178-189
ibis-server/tests/routers/v3/connector/postgres/test_query.py (1)
102-107: Case-insensitive requiredProperties and variable usage covered—nice.Renaming to "Session_user" and using "@session_User" increases confidence that name matching is case-insensitive.
|
Thanks @goldmedal |
2 participants
I missed to pass the session propertes when construct the session cotnext. It leads to miss the properties for check the CLAC policy.
Summary by CodeRabbit