Rework codecs for redis search module - #3884
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 48187a5. Configure here.
| } | ||
|
|
||
| final SearchReply.SearchResult<K, V> searchResult = new SearchReply.SearchResult<>(id); | ||
| final SearchReply.SearchResult<K> searchResult = new SearchReply.SearchResult<>(id); |
There was a problem hiding this comment.
Aggregation replies invent document id
Medium Severity
When parsing aggregation rows with withIds false, the RESP2 path still builds a result whose id is codec.decodeKey of the literal "0". The RESP3 path correctly uses a null id. Callers reading getId() on aggregate rows therefore see a fake "0" (or a codec-transformed form of it) instead of no id.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 48187a5. Configure here.
There was a problem hiding this comment.
Confirmed, but pre-existing on main rather than introduced by this PR. The diff here only changes the generics around that line.
Neither protocol returns an id for aggregation rows, so the RESP2 path fabricates a "0" the server never sent, while RESP3 correctly yields null. It has gone unnoticed because no test in either protocol asserts getId() on aggregation rows.
The fix is initializing the id to null in SearchReplyParser.parseResults (aligning RESP2 with RESP3), plus a regression test in RediSearchAggregateIntegrationTests asserting aggregation rows expose null ids — inherited by the RESP2 subclass, so it runs under both protocols.
First I want to check this with @a-TODO-rov and @tishun.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48187a53ab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| Map<String, String> doc = new HashMap<>(); | ||
| doc.put("title", "Wireless Headphones"); | ||
| doc.put("embedding", embeddingStr); |
There was a problem hiding this comment.
In this example the index is declared as a FLOAT32 vector and the query parameter below is converted to little-endian bytes, but the document's embedding is stored as Arrays.toString(embedding) through a Map<String, String>. RediSearch vector fields expect the stored hash value to be the binary vector blob (for FLOAT32, 4 bytes per dimension), so users following this guide will index an invalid/non-queryable vector and the KNN search below will not work as shown; use a binary value/byte-array write for the embedding field instead.
Useful? React with 👍 / 👎.
| Map<String, FieldValue> result = hybridResult.getFields(); | ||
| if ("apple".equals(result.get("brand"))) { |
There was a problem hiding this comment.
After the reply fields were changed to FieldValue, result.get("brand") no longer returns a String, so both the Apple and Samsung branches are always skipped. This lets hybridWithReducerAvgMinMax pass without checking any AVG/MIN/MAX values as long as some result exists; compare result.get("brand").asString() and assert that the expected branches were exercised.
Useful? React with 👍 / 👎.
| * @since 7.2 | ||
| */ | ||
| @Experimental | ||
| HybridReply<K> ftHybrid(String index, HybridArgs args); |
There was a problem hiding this comment.
This new template declares ftHybrid with HybridReply and HybridArgs, but those types are not imported in the template's import block. The checked-in sync/async/reactive interfaces add the imports manually, so regenerating from this template would emit an interface that does not compile until the hybrid imports are added here as well.
Useful? React with 👍 / 👎.
| * @param value the raw field value exactly as returned by the server. Must not be {@code null}. | ||
| * @return a {@link FieldValue} view over the given bytes | ||
| */ | ||
| public static FieldValue of(byte[] value) { |
There was a problem hiding this comment.
This new public factory is part of the newly added FieldValue API, but its Javadoc omits @since 7.7. The repository requires @since on new public API, so generated docs will lack versioning for this entry point unless the tag is added.
AGENTS.md reference: AGENTS.md:L145-L146
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6d2ec3812
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| System.out.println("Key: " + result.getKey()); | ||
| for (SearchReply.SearchResult<String> result : results.getResults()) { | ||
| System.out.println("Key: " + result.getId()); | ||
| System.out.println("Title: " + result.getFields().get("title")); |
There was a problem hiding this comment.
With the new SearchReply API, getFields() now returns FieldValue instances instead of decoded strings. In the guide, users following this snippet will print the FieldValue object identity rather than the title; the other result-printing examples in this page have the same pattern. Decode textual fields with .asString() (or document .asBytes() for binary fields) so the updated public API is demonstrated correctly.
AGENTS.md reference: AGENTS.md:L40-L41
Useful? React with 👍 / 👎.
| public RediSearchPrefixingStringCodecSafetyIntegrationTests() { | ||
| RedisURI uri = RedisURI.Builder.redis("127.0.0.1").withPort(16379).build(); | ||
| client = RedisClient.create(uri); | ||
| connection = client.connect(new PrefixingStringCodec(CODEC_PREFIX)); | ||
| redis = connection.sync(); |
There was a problem hiding this comment.
JUnit Jupiter uses per-method test instances unless the class is annotated with @TestInstance(PER_CLASS), but this constructor writes to static client/connection fields. Each test and parameterized invocation recreates and overwrites them, while @AfterAll closes only the last objects, leaving earlier Redis clients/connections running during the integration suite; the structured-key safety test uses the same pattern. Move this setup to @BeforeAll or make the fields instance-scoped and close them per test.
Useful? React with 👍 / 👎.
…th String # Conflicts: # .gitignore # Conflicts: # src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java # src/main/java/io/lettuce/core/AbstractRedisReactiveCommands.java # src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java # src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImpl.java # src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt # src/main/templates/io/lettuce/core/api/RediSearchCommands.java
…pdate all affected files. Add additional unit and integration tests for the WITHSCORE param.
… field - it stays generic K.
…ot neede and fix all affected files.
…o be returned by search.
b6d2ec3 to
a605ccf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5063dc096
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static RedisCommands<String, String> redis; | ||
|
|
||
| public RediSearchPrefixingStringCodecSafetyIntegrationTests() { | ||
| RedisURI uri = RedisURI.Builder.redis("127.0.0.1").withPort(16379).build(); |
There was a problem hiding this comment.
When integration tests run against a configured non-default host, port, or TLS topology, this constructor ignores the test settings and always connects to loopback port 16379, so both newly added codec-safety test classes can fail before reaching their assertions. Build the URI from TestSettings/TlsSettings instead of hard-coding the endpoint.
AGENTS.md reference: AGENTS.md:L135-L136
Useful? React with 👍 / 👎.
| * | ||
| * @param <K> Key type. | ||
| * @param <V> Value type. | ||
| * @param Key type. |
There was a problem hiding this comment.
After GroupBy became non-generic, this @param Key tag refers to no declared parameter, which the Javadoc tool treats as an error and can break API-documentation generation. Remove this tag and the equivalent stale tags in SortBy, SortProperty, and their factory methods.
AGENTS.md reference: AGENTS.md:L153-L154
Useful? React with 👍 / 👎.
| public SugAddArgs payload(String payload) { | ||
| this.payload = payload; |
There was a problem hiding this comment.
When callers store a non-UTF-8 FT.SUGADD PAYLOAD—previously supported with a binary value codec—the new String signature removes any way to supply the original bytes, and build() always UTF-8-encodes the value; SuggestionParser similarly UTF-8-decodes returned payloads. Keep a binary payload path, such as a byte[] overload and a byte-preserving result accessor, so opaque suggestion metadata still round-trips.
Useful? React with 👍 / 👎.
2 participants
Summary
Updates the RediSearch API types to match how command data is encoded. Complete Redis keys keep
K; schema identifiers, query tokens, index prefixes, and server-generated text useStringand bypass the connection codec. The change also fixes warning decoding and RESP2 suggestion-score parsing.Why
A schema field can be a hash field for
ON HASHor a JSONPath forON JSON, so it cannot consistently use the key codec. The same applies to query tokens and other RediSearch identifiers.Changes
CreateArgsand argument types used for schema fields, queries, aggregation, hybrid search, highlighting, spell checking, and synonyms. These identifiers are sent raw; applications using a transforming key codec must pass an index prefix that matches the stored keys.Kfor document keys inINKEYS, suggestion dictionary keys, and document ids returned in search replies.String.FT.TAGVALSto returnList<String>, matchingFT.DICTDUMPandFT._LIST.Testing
Codec-safety integration tests cover HASH and JSON with prefixing and structured key codecs. They verify that
PREFIXis sent raw using the caller-provided physical prefix, whileINKEYSstill uses the key codec. Parser tests cover warnings, mixed field values, and RESP2 suggestion scores.Risk
This is a breaking change to the experimental RediSearch APIs. Applications using transforming codecs must review schema identifiers and index prefixes when migrating.
Make sure that:
mvn formatter:formattarget. Don’t submit any formatting related changes.Note
High Risk
Breaking change across the public RediSearch command, argument, and reply types (sync/async/reactive/cluster). Callers with custom codecs must re-check prefixes, queries, and result field types.
Overview
Aligns RediSearch encoding with Redis: schema names, queries, prefixes, and server text go as raw
String, while document keys (INKEYS, suggestion dict keys, result ids) stayK.RediSearchCommands(and async/reactive/cluster) drop theVtype parameter.CreateArgs, field args, hybrid/aggregate/spellcheck/synonym helpers, and most replies (SearchReply<K>,AggregationReply<K>,SpellCheckResult,Suggestion) no longer use the value codec. Queries and dict/synonym terms areString;FT.TAGVALS/FT.DICTDUMP/FT._LISTreturnList<String>. Field names and warnings decode as UTF-8; field values stay raw bytes.Command builders use
args.add(...)instead ofaddValuefor those tokens. Docs and tests cover HASH/JSON with prefixing codecs, warning parsing, and RESP2 suggestion scores. This is a breaking change to the experimental search APIs.Reviewed by Cursor Bugbot for commit f5063dc. Bugbot is set up for automated code reviews on this repo. Configure here.