-
Notifications
You must be signed in to change notification settings - Fork 211
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
fix: avoid FALSEPREDICATE for user-defined FilterKey #3482
Conversation
@@ -68,28 +68,28 @@ extension Filter where Scope == ChannelListFilterScope { | |||
) -> NSPredicate? { | |||
switch op { | |||
case .and where mappedValue is [Filter<Scope>]: | |||
guard let filters = mappedValue as? [Filter<Scope>] else { | |||
guard let predicates = (mappedValue as? [Filter<Scope>])?.compactMap(\.predicate), predicates.count > 0 else { |
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.
@kaijietti Thank you for the PR. I have verified this change.
Could you please replace predicates.count > 0
with !predicates.isEmpty
due to our enforced formatting rules. If this is done, I will then proceed with merging this PR after verifying that it passes all the CI checks (I will run it separately).
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.
@laevandus I replaced in the latest commit, please take a look, thanks!
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.
@kaijietti @laevandus Would be nice to add this edge case to the ChatChannelListController_Tests.test_filterPredicate_XXX...
tests. So that we do not have regressions in the future 🙏
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'll merge this PR and will create a new one with tests and changelog entry.
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.
Note, I ran these changes on a CI in a separate job and all was green.
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.
As said, I'll open a follow-up PR to add some tests and fill in the changelog.
🔗 Issue Links
None. The issue is easy to reproduce.
Filter is mainly used in two cases:
After creating a ChatChannelListController, the controller owns a query inside which is a filter. The filter is mix-used in the above two cases.
On one hand, stream API supprt filtering extraData of channel. So user can defined their custom field via extension:
and use it in this way:
We have checked the the query sent to the stream backend is correct as it should be.
However, on the other hand, the filter inside the query is also used in the process of constructing predicate of request sent to the local Database. And since the custom FilterKey has no related ChannelDTO field, so the KeyPathString is nil, and thus for all custom FilterKey, the predicate is actually a nil. This works fine when the filter is so flat. But when all the subpredicates inside a logicalPredicate are custom Filterkey, all the
NSPredicate?
they return is nil and thus being filtered out when using.compactMap(...)
, and the predicate would beNSCompoundPredicate(andPredicateWithSubpredicates: [])
or NSCompoundPredicate(orPredicateWithSubpredicates: []) which is actually aFALSEPREDICATE
. And since the final predicate is an.and
operator, the predicate is always false and no data is fetched back.stream-chat-swift/Sources/StreamChat/Database/DTOs/ChannelDTO.swift
Lines 411 to 415 in f908d69
🎯 Goal
Make sure the request SQL sent to the CoreData DB is correct (or, avoid FALSEPREDICATE) when user is using compound predicate in which all the subpredicates are related to user-defined FilterKey.
📝 Summary
Provide bullet points with the most important changes in the codebase.
🛠 Implementation
Provide a detailed description of the implementation and explain your decisions if you find them relevant.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
🧪 Manual Testing Notes
Explain how this change can be tested manually, if applicable.
☑️ Contributor Checklist
🎁 Meme
Provide a funny gif or image that relates to your work on this pull request. (Optional)