feat(llc): add normalizeStringForSort and Filter.raw, fix array matching - #181
Draft
xsahil03x wants to merge 20 commits into
Draft
feat(llc): add normalizeStringForSort and Filter.raw, fix array matching#181xsahil03x wants to merge 20 commits into
xsahil03x wants to merge 20 commits into
Conversation
`Filter` modelled `$and` and `$or` but not `$nor`, so a query needing "none of these" had no way to express it — and with `Filter` sealed, a downstream SDK cannot add the operator itself. `NorOperator` joins the existing `LogicalOperator` family, so it serializes and nests like the other two, and `matches` is the negation of `OrOperator`'s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Sort` is `@JsonSerializable(createFactory: false)`, so nothing could read back
the `{field, direction}` shape it writes. Every SDK that persists a sort or
reads one echoed by the API has to re-derive the rule, and it is easy to get
wrong: anything other than `-1` is ascending, so a direction the API did not
write is ascending rather than an error. `SortDirection.fromJson` owns that
rule once. Field resolution stays with the caller, which is the part only it
can do.
`Sort` also had no `==`, so two independently written identical sorts compared
unequal. All its fields are final and its constructors are const, so it is
annotated `@immutable` and compares on the field's remote name, the direction
and the null ordering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Filter` is sealed, so a downstream SDK cannot add a variant. Two cases had
nowhere to go as a result.
`Filter.empty` constrains nothing and serializes to `{}`. It is the identity
element of `Filter.and`, for the common case of building a filter
conditionally. `Filter.and([])` is not a substitute: it serializes to
`{"$and": []}`, which the API rejects.
`Filter.raw` carries a query this package does not model, serialized verbatim.
Without it a caller who needs an operator core has not modelled is stuck, since
the sealed hierarchy leaves no way to work around the gap. It cannot be
interpreted, so `matches` returns `true` for it rather than guessing, and its
dartdoc says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ComparableField` compares strings with `String.compareTo`, which orders by code unit and so pushes lowercase-leading and accented names (`jhon`, `Łukasz`, `Øystein`) past `Zara`. A list sorted locally then disagrees with the one a query returns, which every product hits the moment it sorts by name. Folding inside `ComparableField` was considered and rejected: `Filter` compares through it too, so `$eq`, `$gt` and `$lt` would become case- and diacritic-insensitive locally while the API stayed exact. This is a utility to apply in a `SortField`'s value getter instead, leaving comparison semantics alone. Ported from `stream_chat`, along with its tests. Preserves Japanese, Thai and Vietnamese-specific runes, and mirrors the API's own `NormalizeName`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
+ Coverage 68.02% 68.17% +0.14%
==========================================
Files 209 210 +1
Lines 8554 8597 +43
==========================================
+ Hits 5819 5861 +42
- Misses 2735 2736 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adding it here would have put this package ahead of the other two cores: neither StreamCore (Swift) nor stream-android-core models `$nor`, and their operator sets are otherwise identical to this one. The case for adding it was that the chat SDKs all expose `$nor` and would lose it on migrating. That does not hold. stream-chat-swift depends on StreamCore and keeps its own 671-line `Filter`, undeprecated, so no chat SDK uses core's filter and none is scheduled to. Core has no consumer for the operator. Widening the core filter family is a cross-core decision to take with all three owners at once, not a side effect of one product's migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Filter.and([])` was not a substitute for it, but nullability is, and that is
how the one package using this filter already works: `stream_feeds` types its
query filters `Filter?` and defaults at the request boundary, so it never needs
a neutral value.
It is also not merely unused. It serializes to `{}`, and `queryThreads` reads
`request.Filter != nil` rather than a length — so a neutral filter is non-nil
there and switches the request onto a different, wider server query, while
omitting the field does not. Every other endpoint checks the length and cannot
tell the two apart. An API whose only correct use is "never pass this to
threads" is worse than no API.
`Filter.raw` stays: it carries a query this package does not model, which the
sealed hierarchy otherwise leaves no way to express.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The equality was not merely unused, it was wrong. It compared `field.remote` and ignored the comparator, so two sorts naming the same wire field with different value getters — a declared field and a custom one for the same key — compared equal and hashed equal while ordering a list in opposite directions. As a map key or in a dedupe that silently yields the wrong order. It cannot be fixed either. Comparing the field itself is identity, since `SortField` has no `==`, which defeats the purpose; and the field's comparator is a closure, which has no meaningful equality. There is no correct value equality for `Sort` while a field carries a closure. `@immutable` went with it, having been added only to satisfy the lint that guards `==`/`hashCode`. Also `@JsonEnum(valueField: 'value')` on `SortDirection`: the wire values were declared twice, once as `@JsonValue` for the generated map and once as the enum field that `fromJson` reads. Changing one and not the other would have made serialization disagree with deserialization silently. The generated map is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generator never calls it. `json_serializable` uses a `fromJson` static for class-typed fields, but decodes an enum through `$enumDecode` against the generated value map, taking its fallback from `@JsonKey(unknownValue:)` at each field site and otherwise throwing. So the static would be reachable only by hand, while any generated code disagreed with it: strict where the enum documented itself as lenient. An enum cannot force the lenient path — only a field can — which makes this the wrong place for the rule regardless of how right the rule is. `@JsonEnum(valueField: 'value')` stays. It removes a real duplicate: the wire values were declared once as `@JsonValue` for the generated map and once as the enum field, and changing one without the other would have gone unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With `fromJson` gone nothing reads the enum's `value` field for JSON, so the duplication the `valueField` form removed no longer has anything depending on it. Unrelated churn in a PR about two additions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`matches` returned `true`, which is right under `and` and wrong under `or` and `nor`, where an opaque node would match every record. Reporting no match inverts the problem. There is no answer that is correct under every composition, because the node cannot be interpreted at all. Swift resolves this with a nullable predicate and `compactMap`, dropping nodes it cannot wire, but `bool matches(T)` has no third state to drop into. So it throws, which is the only outcome that cannot silently produce a wrong result, and the message names the query that caused it. `ComparableField` already throws from this layer for the same reason. Nothing evaluates filters locally today, so this costs nothing now and turns a future wrong list into an immediate, precise failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three deviations from STYLE_GUIDE and Effective Dart. The `Filter.raw` summary opened with an article where every sibling factory in the file uses a bare noun phrase (`Equality filter matching …`), and its second paragraph was a fragment. `String.compareTo` was backticked in the normalizer, but this repo reserves backticks for names that are *not* in scope and `dart doc` resolves the bracketed form to a link. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs had grown into rationale dumps: why refusing to evaluate beats guessing, why folding is selective. That reasoning belongs in the commit and the PR, not in what a caller reads on hover. The normalizer also listed its four folding steps in order and linked the backend Go source that inspired them. Both are implementation, and the link points at a repository a consumer of this package cannot open. The link moves to a `//` comment on the function, where a maintainer will find it and the generated docs will not carry it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It sat between the dartdoc and the declaration, which reads as a mistake, and it was attached to the wrong thing: the selectivity it explains is `_foldRune`'s preserved-script branches, not the public entry point. `_foldRune` already carried a `//` comment describing exactly that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Trimming the verbosity took the steer with it, leaving `raw` reading as one option among the operators rather than the escape hatch it is. It now says so first, and says what a caller gives up: a declared operator names its field through a `FilterField`, so a typo is a compile error and the filter stays evaluatable, and `raw` is neither validated nor evaluatable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It had grown into one dense paragraph carrying three separate points: when the escape hatch applies, why a declared operator is better, and what it costs. Split so each reads on its own — summary, when and what to prefer, then the two things a caller gives up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
page_width is 120 and the line is 83; dart format does not reflow comments, so the narrow wrap was mine to fix rather than the formatter's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Filter.equal` and `Filter.in_` compared an array field with deep equality, which is order-sensitive and so never matched the way a query does. A field whose getter returns a list now compares as a set for `$eq`, and intersects for `$in`. An array passed to `$in` still compares whole, so a value that is itself an array keeps its meaning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three changes to
stream_core, all walls thestream_chat→stream_coremigration hit.Five more were added and taken back out under review. The history is left intact rather than squashed — Considered and dropped is the more useful half of this PR, and three of those five were things the migration plan had recommended before the evidence came in.
normalizeStringForSortComparableFieldcompares strings withString.compareTo, which orders by code unit and pushes lowercase-leading and accented names (jhon,Łukasz,Øystein) pastZara. A locally sorted list then disagrees with the one a query returns, which every product hits the moment it sorts by name.Folding inside
ComparableFieldwas considered and rejected:Filtercompares through it too, so$eq,$gtand$ltwould become case- and diacritic-insensitive locally while the API stayed exact. This is a utility to apply in aSortFieldvalue getter instead. Ported fromstream_chatwith its tests; preserves Japanese, Thai and Vietnamese-specific runes.Filter.rawFilterissealed, so a downstream package cannot add a variant — a gap here has no workaround. The concrete case: a Stream predefined filter is authored server-side and echoed back to the SDK, and GetStream/chat#15657 withdrew$neand$ninfrom the published spec without removing them from the query layer, so an echoed filter can legitimately carry an operator core does not model.rawis the fallback leaf that lets a decoder stay total, the way an unrecognised sort field falls back to a custom one.matchesthrows for it. Returningtrueis right underandand wrong underor/nor, where an opaque node would match every record; returningfalseinverts the problem. Swift resolves this with a nullable predicate andcompactMap, dropping nodes it cannot wire, butbool matches(T)has no third state. Refusing is the only outcome that cannot silently produce a wrong result.ComparableFieldalready throws from this layer.Array-valued fields match element-wise
Filter.equalandFilter.in_compared a field's value with deep equality, which is order-sensitive for lists. A field whose getter returns a list therefore never matched in practice:$inasked whether the whole list equalled one of the query values, and$eqasked for the same elements in the same order.Neither is what a query does. Every array field the API exposes — a channel's
membersandfilter_tags, a user'steams, a message'sattachments.type— is matched element-wise, comparing as a set for$eqand intersecting for$in. Not one is order-sensitive, so the old behaviour could not agree with any query the API accepts.An array passed to
$instill compares whole, so a value that is itself an array keeps its meaning; only plain values intersect. The$eqtest that asserted order-sensitivity is updated, since no query can produce that answer.Considered and dropped
$nor. Neither StreamCore (Swift) nor stream-android-core models it, and adding it to Dart alone breaks a parity the three otherwise keep.stream-chat-swiftdepends on StreamCore and keeps its own 671-lineFilter, undeprecated, so no chat SDK uses core's filter today. Chat is removing$norfrom its own surface rather than pushing it up.Filter.empty. Nullability covers it, which is howstream_feedsalready works:Filter?on the query, defaulted at the request boundary. It is also not merely unused — it serializes to{}, andqueryThreadsreadsrequest.Filter != nilrather than a length, so a neutral filter is non-nil there and switches the request onto a wider server query while omitting the field does not.Sortvalue equality. It comparedfield.remoteand ignored the comparator, so two sorts naming the same wire field with different value getters compared equal and hashed equal while ordering a list in opposite directions. Unfixable while a field carries a closure: comparing the field itself is identity, and closures have no meaningful equality.SortDirection.fromJson. The generator never calls it.json_serializableuses afromJsonstatic for class-typed fields, but decodes enums through$enumDecodeagainst the generated map, taking its fallback from@JsonKey(unknownValue:)at each field site. It would have been reachable only by hand, while generated code disagreed with it.@JsonEnum(valueField:)onSortDirection. Removed a real duplicate, but oncefromJsonwas gone nothing depended on the two declarations agreeing.sort.dartis untouched by this PR.A per-field
isCollectionopt-in. The first shape of the array fix, so the default could stay order-sensitive. It needed a new constructor onFilterFieldand a flag at every collection declaration, to reach behaviour the runtime type already determines — and no API field turned out to want the order-sensitive reading.Verification
dart analyze --fatal-infos lib testclean, full suite green at 827 tests, up from 812.The commit count is high for the size of this — the net diff is one new file, one new filter variant, two
matchesbodies and their tests. Reviewing the diff rather than the commits is the faster path; the commits are there for the rejections.🤖 Generated with Claude Code