-
Notifications
You must be signed in to change notification settings - Fork 0
Implement support for local filtering #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for local filtering of WebSocket events by introducing localValue lambdas to filter fields and implementing a matches function that can evaluate filters against local data without making API calls.
- Adds
localValueproperty toFilterFieldinterface to extract field values from model instances - Implements
matchesfunction to evaluate filters locally using new filter operations - Splits
FilterOperatorinto separateBinaryOperatorandCollectionOperatorenums for better organization
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| FilterToRequestTest.kt | Updates test to use new generic FilterField signature with TODO placeholder |
| FilterMatchingTest.kt | Comprehensive test coverage for the new local filter matching functionality |
| FilterOperator.kt | Renames and splits operator enum into separate binary and collection types |
| FilterOperations.kt | New file implementing comparison and evaluation operations for local filtering |
| Filters.kt | Updates filter functions to support new generic type parameters |
| FilterField.kt | Adds localValue property for extracting field values from model instances |
| Filter.kt | Adds matches function and updates filter types with new generic parameters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
stream-android-core/src/test/java/io/getstream/android/core/api/filter/FilterToRequestTest.kt
Outdated
Show resolved
Hide resolved
stream-android-core/src/main/java/io/getstream/android/core/internal/filter/FilterOperations.kt
Show resolved
Hide resolved
stream-android-core/src/main/java/io/getstream/android/core/internal/filter/FilterOperations.kt
Outdated
Show resolved
Hide resolved
f12b817 to
1fa2294
Compare
VelikovPetar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool! 😄
Left some small suggestions, but otherwise looks pretty good!
stream-android-core/src/main/java/io/getstream/android/core/api/filter/Filters.kt
Show resolved
Hide resolved
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.getstream.android.core.api.filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR: I can see that the io.getstream.android.core.api.filter also contains the Sort logic. Perhaps we should move it to another package sort, or to rename this whole package to something like query, which will hold both filtering and sorting logic (would be good to unify this before the first major release to avoid breaking changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember @aleksandar-apostolov not being fully convinced about the query name for filtering. I wanted to find an even more generic name, but couldn't. I asked cursor and it agrees with you :D
It also suggested other stuff, but I don't think they're fitting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not convinced because it was called query and only had Filters.kt in it.
But if its 3 on 1 (including Cursor, feel free to rename it)
Here is Codex opinion btw:
The code in io.getstream.android.core.api.filter and io.getstream.android.core.internal.filter is all about the filtering DSL—operators, filter builders, and sort helpers—not about end-to-end “query” construction.
- Public API surface such as Filters.and, Filters.or, and the extension helpers like FilterField.equal, FilterField.in, etc. are strictly building filter expressions that map onto the server-side filter JSON (Filters.kt:20-154).
- Those helpers are backed by the internal pieces FilterOperator and SortFieldImpl (FilterOperator.kt:16-63, SortFieldImpl.kt:16-31), which simply translate our filter clauses into the $eq, $in, $and, … operators the API expects.
A “query” in Stream terminology usually combines multiple concerns (filters, sort, pagination limits, watch flags, etc.), and there are already request DTOs and client methods that represent full queries elsewhere in the SDK. Renaming this package to query would therefore overstate what the code does and blur the separation between the low-level filter DSL and the higher-level query request objects. Keeping it as filter/filters makes it clear these classes are the filter-and-sort building blocks that other query-related components consume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify I asked Codex "Is query a better package name for the filter package." :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took Petar's first suggestion and moved sort code to its own package, so we avoid the query name, since core doesn't know anything about queries now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
If @aleksandar-apostolov agrees as well, I would say to go ahead and merge it!
stream-android-core/src/main/java/io/getstream/android/core/api/filter/Filter.kt
Show resolved
Hide resolved
9c6e6bf to
521c54a
Compare
Goal
Add support for local filtering of WS events so we can avoid handling events that don't match filters.
iOS PR: GetStream/stream-core-swift#2
Implementation
localValuelambda to filter fields to get the value to use for filteringmatchesfunction which takes the above + the filter operation & delegates to the correct implementationTesting
There are tests covering the new logic. Testing manually means writing something similar to what's there and verifying that it returns the expected result.
Checklist