fix: rework search API generics to replace unnecessary type params with String#3716
Conversation
aca713c to
aeb5532
Compare
aeb5532 to
eecece8
Compare
9c29c1c to
5300e85
Compare
59689ba to
7230ad5
Compare
7230ad5 to
7c19f36
Compare
tishun
left a comment
There was a problem hiding this comment.
LGTM
(best effort since it as a huge review)
| * @deprecated {@code FT.TAGVALS} has been deprecated by Redis. See | ||
| * <a href="https://redis.io/docs/latest/commands/ft.tagvals/">FT.TAGVALS</a>. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
@a-TODO-rov TBH I am wondering if we should not just remove this. Perhaps @uglide can help us understand if there is a use case for keeping it?
| * AggregateArgs<String, String> args = AggregateArgs.<String, String> builder().groupBy("category") | ||
| * .reduce(Reducer.count().as("count")).sortBy("count", SortDirection.DESC).build(); | ||
| * SearchReply<String, String> result = redis.ftAggregate("myindex", "*", args); | ||
| * AggregateArgs<String> args = AggregateArgs.builder().groupBy(GroupBy.of("category").reduce(Reducer.count().as("count"))) |
There was a problem hiding this comment.
For some reason the AI agent has changed the groupBy notation from groupBy("category") to groupBy(GroupBy.of("category").
| * AggregateArgs<String, String> args = AggregateArgs.<String, String> builder().load("price", "quantity", "category") | ||
| * .apply("@price * @quantity", "total_value").filter("@total_value > 100").groupBy("category") | ||
| * .reduce(Reducer.sum("@total_value").as("category_total")).reduce(Reducer.avg("@price").as("avg_price")) | ||
| * AggregateArgs<String> args = AggregateArgs.builder().load("price").load("quantity").load("category") |
There was a problem hiding this comment.
The AI agent has changed the example from load("price", "quantity", "category") to load("price").load("quantity").load("category").
0eeb412 to
cf48a11
Compare
cf48a11 to
fd16d95
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 0786cd8. Configure here.
1564fff to
5a0215e
Compare
5a0215e to
f93e2e7
Compare

Summary
Reworks the generics across the RediSearch API to remove unnecessary type parameter exposure and replace them with
Stringwhere values were never codec-encoded in the first place. Fixes incorrect codec usage in warning decoding across both search parsers.Changes
Arguments classes —
KandVremovedThe following argument classes were fully de-genericized. All fields and builder methods that previously carried
KorVbecomeString, since they represent schema attribute names, filter expressions, stop words, prefixes, and other index-definition strings that are always UTF-8 text:CreateArgs<K, V>→CreateArgs(prefixes, filter, language field, score field, payload field, stop words)FieldArgs<K>→FieldArgs(field name, alias); all concrete subclasses follow:GeoFieldArgs,GeoshapeFieldArgs,NumericFieldArgs,TagFieldArgs,TextFieldArgs,VectorFieldArgsExplainArgs<K, V>→ExplainArgsSpellCheckArgs<K, V>→SpellCheckArgsSynUpdateArgs<K, V>→SynUpdateArgsSugAddArgs<K, V>→SugAddArgsSugGetArgs<K, V>→SugGetArgsHybridArgs<K, V>→HybridArgsHighlightArgs<K, V>→HighlightArgs<K>—Vremoved (tags becomeString);Kis retained because highlight fields are hash field identifiers, consistent withhget(K key, K field).Tags<V>→Tags,addValue()→add()inbuild()AggregateArgs<K, V>→AggregateArgs<K>Vis removed entirely. InsideAggregateArgs,Kis narrowed toLoadFieldonly, since hash field identifiers are legitimately typed asK. As a consequence:LoadField.fieldis now correctly encoded viaargs.addKey()instead of the previous.toString()bypass;LoadField.aliasbecomesStringApply.name,Reducer.alias,GroupBy.properties,SortProperty.property, param keys) becomeStringGroupBy,SortBy,SortProperty,Apply,Reducer,Filter,LimitandPipelineOperationare now non-generic withbuild()signatures narrowed fromCommandArgs<K, ?>toCommandArgs<?, ?>SearchReplyandHybridReplywarningswarnings: List<V>→List<String>in both classes. Server-generated warning messages are always UTF-8 text and must not be decoded through the user-configured value codec.SearchReplyParserandHybridReplyParserare updated to useStringCodec.UTF8.decodeValue()at all warning-decoding sites — including both the RESP2 and RESP3 paths inHybridReplyParser, which had the same bug in two separate places.Command interfaces and implementations
All method signatures updated across the full command surface:
RediSearchCommandsRediSearchAsyncCommandsRediSearchReactiveCommandsAbstractRedisAsyncCommands,AbstractRedisReactiveCommands,RediSearchCommandBuilderJavadoc corrections
AggregateArgscorrected: wrong return type (SearchReply→AggregationReply), stale generic witnesses, invalidload()varargs call, and.reduce()chained directly onBuilderinstead ofGroupByWithCursorfactory method Javadocs corrected: were copy-pasted fromApplywith wrong@paramdescriptions and@returntextfilter()builder example corrected for the same.reduce()onBuildermistakeTesting
testSearchWarningsReturnedAsStringsadded toRediSearchIntegrationTests, verifying thatgetWarnings()returnsList<String>, that the list is non-null for normal searches, and that any warning content produced under a 1 ms timeout is correctly decoded as a non-empty UTF-8 stringHybridReply.getWarnings()(size() >= 0) replaced with a typedList<String>assignment and per-element content validationNote
Medium Risk
Public RediSearch command signatures are changed broadly (sync/async/reactive/cluster/builders), which is a source-compatible breaking change for clients and could affect downstream compilation and behavior. Runtime risk is moderate and mainly limited to parsing/encoding of RediSearch protocol literals (queries, warnings, dict/synonyms).
Overview
This PR reworks the RediSearch Java API types to stop treating query/dictionary/synonym/index metadata as user
Vvalues, replacing those parameters/return types withStringacross command interfaces and implementations (sync/async/reactive and cluster), and updatingRediSearchCommandBuilderaccordingly (e.g.,ftSearch/ftAggregate/ftExplain/ftSpellcheck, dict/synonym ops,ftList).It also fixes warning decoding in
SearchReplyParser/HybridReplyParserand changesSearchReply/HybridReplywarnings toList<String>so server warnings are decoded viaStringCodec.UTF8instead of the user value codec;SpellCheckResultis de-genericized toString-based terms/suggestions. Minor repo hygiene:.gitignorenow ignores.augmentand correctly matches.vscode*.Reviewed by Cursor Bugbot for commit bb65286. Bugbot is set up for automated code reviews on this repo. Configure here.