fix(core): normalize the name of session property#1331
fix(core): normalize the name of session property#1331douenergy merged 2 commits intoCanner:mainfrom
Conversation
WalkthroughImplements case-insensitive session property handling by introducing SessionProperty.normalized_name, custom deserialization, and refactoring access-control logic to use normalized names. Updates constructors, accessors, and tests (including headers and manifests) to reflect normalized lookups and varied results based on session_level values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as Server/API
participant AC as AccessControl
participant SP as SessionProperty
Note over C,S: Request with headers (e.g., "Session_level": "1")
C->>S: Execute query (headers, manifest)
S->>AC: Validate RLAC/CLAC and build filters
AC->>SP: Deserialize manifest SessionProperty
Note over SP: normalized_name = name.to_lowercase()
AC->>AC: Lookup header by property.normalized_name()
alt Required property missing
AC-->>S: Error (missing required property)
S-->>C: 4xx error
else Present or defaulted
AC->>AC: Apply rule/filter using normalized names
S-->>C: Result (shape depends on session_level)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
github-actions
bot
added
core
ibis
rust
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
wren-core-base/src/mdl/py_method.rs (1)
76-82: Unify normalization: prefer ASCII-lowercasing for property keysTo keep normalization consistent and predictable across platforms/locales, use to_ascii_lowercase() here (headers and other call sites also lower-case).
- pub fn new(name: String, required: bool, default_expr: Option<String>) -> Self { - Self { - normalized_name: name.to_lowercase(), + pub fn new(name: String, required: bool, default_expr: Option<String>) -> Self { + Self { + normalized_name: name.to_ascii_lowercase(), name, required, default_expr, } }wren-core/core/src/mdl/mod.rs (1)
3665-3665: Normalize headers with ASCII-lowercase for consistencyUse to_ascii_lowercase() to match other normalization paths and avoid locale surprises.
- headers.insert(key.to_lowercase(), value.clone()); + headers.insert(key.to_ascii_lowercase(), value.clone());wren-core/core/src/logical_plan/analyze/access_control.rs (2)
142-149: Normalize with ASCII-lowercase for consistencyElsewhere we lower-case header keys; prefer to_ascii_lowercase() here for uniform behavior (and to match collect_condition).
- let property_name = - value.trim_start_matches("@").to_string().to_lowercase(); + let property_name = + value.trim_start_matches("@").to_ascii_lowercase();
366-370: Treat whitespace-only values as missingCurrent check counts " " as present. Trim before checking emptiness to fail fast on invalid required values and avoid later parse errors.
- headers - .get(property.normalized_name()) - .map(|v| v.as_ref().is_some_and(|value| !value.is_empty())) - .unwrap_or(false) + headers + .get(property.normalized_name()) + .map(|v| v.as_ref().is_some_and(|value| !value.trim().is_empty())) + .unwrap_or(false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ibis-server/tests/routers/v3/connector/postgres/test_query.py(2 hunks)wren-core-base/manifest-macro/src/lib.rs(1 hunks)wren-core-base/src/mdl/builder.rs(2 hunks)wren-core-base/src/mdl/manifest.rs(1 hunks)wren-core-base/src/mdl/py_method.rs(1 hunks)wren-core/core/src/logical_plan/analyze/access_control.rs(6 hunks)wren-core/core/src/mdl/mod.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-05T02:27:29.829Z
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/mdl/mod.rswren-core/core/src/logical_plan/analyze/access_control.rs
🧬 Code graph analysis (5)
wren-core-base/manifest-macro/src/lib.rs (2)
wren-core-base/src/mdl/manifest.rs (5)
name(260-262)name(305-307)name(324-326)name(330-332)normalized_name(336-338)wren-core-base/src/mdl/py_method.rs (2)
new(76-83)new(90-96)
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(54-61)
wren-core/core/src/mdl/mod.rs (1)
wren-core-base/src/mdl/builder.rs (1)
new_required(177-179)
wren-core/core/src/logical_plan/analyze/access_control.rs (1)
wren-core/core/src/mdl/context.rs (1)
properties(93-99)
wren-core-base/src/mdl/builder.rs (2)
wren-core-base/src/mdl/py_method.rs (2)
new(76-83)new(90-96)wren-core-base/src/mdl/manifest.rs (5)
name(260-262)name(305-307)name(324-326)name(330-332)normalized_name(336-338)
⏰ 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). (3)
- GitHub Check: cargo check
- GitHub Check: test
- GitHub Check: ci
🔇 Additional comments (15)
wren-core-base/src/mdl/manifest.rs (1)
335-339: Accessor looks goodPublic getter is minimal and correct; aligns with serde skip-serialize field.
wren-core/core/src/mdl/mod.rs (2)
2662-2666: Case-insensitive property test: LGTMUsing "Session_level" in CLAC requiredProperties to verify case-insensitive handling is appropriate.
2706-2706: Snapshot expectation change aligns with behaviorError message reflecting original name "Session_level" is expected; no action.
wren-core-base/src/mdl/builder.rs (2)
176-182: Centralizing construction via SessionProperty::new: good moveRemoves duplication and guarantees normalized_name initialization.
841-850: Roundtrip test is solid
- Confirms normalizedName is not serialized.
- Ensures normalized_name equals lowercased name.
ibis-server/tests/routers/v3/connector/postgres/test_query.py (3)
81-83: Good coverage for case-insensitive property namesUsing "Session_level" exercises the new normalization path.
676-677: Header for required property path: LGTMEnsures required property present; consistent with case-insensitive lookups.
684-699: Extra case validates shape variance and cache keyingAdding the “session_level=2” call is valuable to assert column-shape changes and cache partitioning by variables.
wren-core/core/src/logical_plan/analyze/access_control.rs (3)
97-105: Use of normalized_name() in RLAC validation is correctAligns requiredProperties validation with case-insensitive normalization.
258-268: Swapping to property-aware presence checks is correctRouting both required/optional paths through is_property_present(&SessionProperty) is cleaner and less error-prone.
304-305: Lookup by normalized key: LGTMproperties.get(property.normalized_name()) matches the canonicalization strategy.
wren-core-base/manifest-macro/src/lib.rs (4)
407-407: RemovedDeserializefrom derive macro for custom deserialization.The
Deserializetrait has been correctly removed from the automatic derive since a custom implementation is provided below. This prevents compilation conflicts.
413-416: Added normalized_name field with appropriate serde configuration.The field is properly configured to skip serialization and use a default value during deserialization, which ensures backward compatibility with existing JSON manifests.
418-429: Constructor implementation follows consistent pattern.The constructor correctly initializes the
normalized_namefield usingto_lowercase()and is properly guarded with the feature flag to avoid conflicts with Python bindings. The implementation matches the pattern used inpy_method.rsas shown in the relevant code snippets.
431-452: Custom deserialization ensures normalized_name is always computed.The custom deserialize implementation correctly:
- Uses a helper struct to deserialize the original fields
- Computes
normalized_namefrom the deserializednamefield usingto_lowercase()- Maintains field order consistency with the struct definition
This approach ensures that all
SessionPropertyinstances have a properly initializednormalized_namefield regardless of how they are created.
|
Thanks @goldmedal |
2 participants
Description
The name of a session property should be case-insensitive. This PR adds the field
normalized_nameforSessionPropertywhich is used to match the required property.Summary by CodeRabbit
Bug Fixes
Tests