feat: add misc fields to filter #1321
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FilterStruct
Client->>FilterStruct: Send JSON with known and unknown fields
FilterStruct-->>FilterStruct: Deserialize known fields normally
FilterStruct-->>FilterStruct: Store extra fields in other_fields (if any)
FilterStruct-->>Client: Struct holds all fields, including arbitrary extras
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ 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 (
|
![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 (2)
src/users/filters.rs (2)
44-46: Good approach for flexible extension!Using
#[serde(flatten)]to capture arbitrary fields is a robust pattern that allows theFilterstruct to be extended without breaking existing code. This change enables storing additional metadata without requiring schema changes.Consider enhancing the documentation to be more explicit about how this field works with Serde's flattening behavior:
- /// all other fields are variable and can be added as needed + /// Additional arbitrary fields not defined in the struct. + /// The #[serde(flatten)] attribute causes Serde to collect any JSON fields + /// not matching other struct fields into this map during deserialization. + /// This allows extending the filter with custom metadata without schema changes. #[serde(flatten)] pub other_fields: Option<serde_json::Map<String, Value>>,
44-46: Consider adding tests for the flattened fieldsTo ensure that the new
other_fieldsfunctionality works as expected, it would be beneficial to add tests.Consider adding tests that verify:
- Deserialization of JSON with extra fields populates
other_fieldscorrectly- Serialization of a
Filterwith populatedother_fieldsincludes those fields in the JSON output- Migration functions preserve
other_fieldsdata- Edge cases like empty objects, null fields, etc.
A simple test could look like:
#[test] fn test_filter_with_other_fields() { let json = r#"{ "stream_name": "test", "filter_name": "test_filter", "query": { "filter_type": "filter" }, "custom_field": "custom_value", "metadata": { "nested": "value" } }"#; let filter: Filter = serde_json::from_str(json).unwrap(); // Verify standard fields assert_eq!(filter.stream_name, "test"); assert_eq!(filter.filter_name, "test_filter"); // Verify other_fields let other_fields = filter.other_fields.unwrap(); assert_eq!(other_fields.get("custom_field").unwrap().as_str().unwrap(), "custom_value"); assert!(other_fields.get("metadata").unwrap().is_object()); // Verify roundtrip serialization let serialized = serde_json::to_string(&filter).unwrap(); let deserialized: serde_json::Value = serde_json::from_str(&serialized).unwrap(); assert_eq!(deserialized.get("custom_field").unwrap().as_str().unwrap(), "custom_value"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/users/filters.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
🔇 Additional comments (1)
src/users/filters.rs (1)
44-46:❓ Verification inconclusive
Verify handling of flattened fields
While this change is well-implemented, it's important to verify how these additional fields behave through the full lifecycle.
Consider how these arbitrary fields are handled in these scenarios:
- When filters are saved to storage
- When filters are loaded from storage
- When filters are migrated between versions
- When filters are accessed through APIs
🏁 Script executed:
#!/bin/bash # Check if we have any handling of other_fields in the codebase echo "Checking references to other_fields:" rg "other_fields" --type rust echo -e "\nChecking serialization/deserialization of Filter struct:" rg "serde_json::(from|to)_(slice|string|value)" -A 2 -B 2 --type rust | rg -A 2 -B 2 "Filter"Length of output: 764
🏁 Script executed:
#!/bin/bash # Locate the Filter struct definition and its implementation to inspect serialization/deserialization echo "Extracting struct Filter definition and impl blocks:" rg --context 10 "struct Filter" -n src/users/filters.rs rg --context 10 "impl Filter" -n src/users/filters.rsLength of output: 4071
🏁 Script executed:
#!/bin/bash # Checking retrieval of raw filters and saving/migration logic echo "Looking for get_all_saved_filters usage:" rg -n "get_all_saved_filters" -C 3 src/users/filters.rs echo -e "\nInspecting save/migration logic in Filters impl:" rg -n "filter_path" -C 5 src/users/filters.rs rg -n "save" -C 3 src/users/filters.rs rg -n "put" -C 3 src/users/filters.rs echo -e "\nChecking migration helper functions:" rg -n "migration::" -C 3 src/users/filters.rs rg -n "to_bytes" -C 3 src/users/filters.rsLength of output: 3496
🏁 Script executed:
#!/bin/bash # Extract full load method for Filters rg -C50 "pub async fn load" -n src/users/filters.rsLength of output: 3988
Verify persistence of
other_fieldsacross storage, migration, and API layersWe’ve confirmed that
#[serde(flatten)] pub other_fieldsis only referenced in theFilterstruct and populated duringFilters::load(viaserde_json::from_value). Migration code usesto_bytes(&Value)and re-writes v1→v2 filters—so unknown fields will round-trip there—but we didn’t find any method in this module that serializes aFilterinstance back to storage outside of migration.Please ensure:
Filters::load / migration
•migrate_v1_v2preserves all extra JSON fields in its output.
•to_bytes(&filter_value)includesother_fieldswhen re-saving migrated filters.New‐filter persistence
• Locate where new or updatedFilterinstances are serialized (e.g. viaserde_json::to_vecorto_bytes) and saved (e.g.put_object) after API calls or service operations.
• Confirm thatother_fieldsis included in that serialization.API input/output
• Check any REST/gRPC handlers that accept or returnFilterJSON to ensure they allow arbitrary fields on input and echo them back on output.Cross‐version migrations
• If you add future schema changes, update migration functions to carry through unknown fields in the JSON map.
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit