[Commands API interface consistency #1] tests and non-breaking fixes - #3865
Conversation
![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: f29cddcb82
ℹ️ 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".
f29cddc to
2b19668
Compare
uglide
changed the title
7d09bed to
fddac9d
Compare
![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 default effort and found 1 potential issue.
❌ 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 fddac9d. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fddac9d08f
ℹ️ 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".
16eebdb to
622d675
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 622d6750ad
ℹ️ 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".
| * This catalog is the single place where a new command group must be registered so that the API consistency test suite covers | ||
| * it. | ||
| */ | ||
| public enum CommandInterfaces { |
There was a problem hiding this comment.
i need more time to digest what is in this PR but one thing i noted is lack of checks around pub/sub interfaces.
atakavci
left a comment
atakavci
left a comment
There was a problem hiding this comment.
other than those small notes i left, LGTM
|
|
||
| @ParameterizedTest | ||
| @EnumSource(CommandInterfaces.class) | ||
| void syncMethodsExistOnAsyncApiWithWrappedReturnType(CommandInterfaces group) { |
There was a problem hiding this comment.
we need same type of tests for among aggregate intefaces; RedisClusterCoroutinesCommands almost has no coverage with those methods in RedisClusterCommands
4edb596 to
950c691
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68a27c866d
ℹ️ 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".
| if (method.isSynthetic() || Modifier.isStatic(method.getModifiers()) || Modifier.isPrivate(method.getModifiers())) { | ||
| continue; | ||
| } | ||
| if (io.lettuce.core.protocol.RedisCommand.class.isAssignableFrom(method.getReturnType())) { |
There was a problem hiding this comment.
Replace this package-qualified RedisCommand reference with an import and simple name; the same new test package also qualifies java.util.Collections in NodeSelectionConsistencyUnitTests and reflection types in KnownApiDeviations. These usages violate the repository’s explicit Java convention and make the new consistency suite itself inconsistent with the required import-driven style.
AGENTS.md reference: AGENTS.md:L150-L152
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b871be753
ℹ️ 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".
| * @deprecated since 7.7 in favor of consuming large results through the {@link org.reactivestreams.Publisher} returned by | ||
| * {@link #hgetex}. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
If the API generators are run before the generator-removal PR lands, this deprecation and the sibling Java API/Javadoc fixes will be overwritten because the corresponding src/main/templates/io/lettuce/core/api/RedisHashCommands.java entry still has no @Deprecated/@deprecated text; the new consistency suite would then fail its streaming-channel deprecation check. The command interfaces with @generated markers need their source template updated and regenerated rather than patched directly.
AGENTS.md reference: AGENTS.md:L89-L93
Useful? React with 👍 / 👎.
| val unmatched = keys.filterNot { key -> | ||
| syncInterfaces.any { syncGroup -> | ||
| TypeSignatures.apiMethods(syncGroup).any { KnownApiDeviations.contains(setOf(key), it, syncGroup) } |
There was a problem hiding this comment.
For tables such as AGGREGATE_PENDING, this staleness check only proves that the key still names a sync method; it does not prove that the coroutine counterpart is still absent. If RedisClusterCoroutinesCommands.clusterLinks is added but the pending entry is left behind, aggregateDeclaredMethodsExistOnCoroutinesAggregate continues to skip shape verification and this test still passes, so the opt-out can permanently hide the newly implemented aggregate method. Make staleness check the condition each table suppresses, not just the existence of the reference method.
Useful? React with 👍 / 👎.
| "cfInsertNxValues", "topKAddValues", "topKIncrByValues", "hscanStreaming", "hscanNoValuesStreaming", | ||
| "scanStreaming", "sscanStreaming", "zscanStreaming", "hello", "sync", "clusterAddslots", "clusterDelslots", | ||
| // connection-level commands exposed via StatefulRedisConnection, not via the command interfaces | ||
| "select", "swapdb"); |
There was a problem hiding this comment.
select and swapdb are public command methods on the standalone aggregates and have RedisCommandBuilder counterparts, but this table marks the builder methods as internal because builderMethodsAreReachableFromCommandInterfaces() never collects RedisCommands/RedisAsyncCommands. If those aggregate API entries are dropped while the builder methods remain, the new builder-coverage suite still passes, so aggregate-only commands are not actually protected; collect the standalone aggregates instead of whitelisting these names.
Useful? React with 👍 / 👎.
| for (CommandInterfaces group : CommandInterfaces.values()) { | ||
| Set<String> names = reachableNamesByBuilder.computeIfAbsent(group.commandBuilderClassName(), | ||
| builder -> new HashSet<>()); | ||
| collectMethodNames(names, group.sync()); | ||
| collectMethodNames(names, group.async()); |
There was a problem hiding this comment.
Because this coverage map is built only from CommandInterfaces plus the cluster aggregates, it never creates an entry for PubSubCommandBuilder or collects the psubscribe/subscribe family declared on RedisPubSubCommands. A PubSub builder method can therefore be removed or left dead while this new bidirectional builder-coverage suite remains green; include the PubSub aggregate and its dedicated builder in the same coverage path.
Useful? React with 👍 / 👎.
8b871be to
463a98b
Compare
Introduce reflection-based unit tests that verify the sync, async, reactive, Kotlin coroutine and cluster node-selection command interfaces stay in lockstep, replacing the consistency-by-regeneration guarantee of the template-based API generators: - CommandInterfaces: catalog of all command groups and their flavors - KnownApiDeviations: single registry of intentional deviations, ported from the generator rule tables - TypeSignatures: shared reflection and type-normalization helpers - SyncAsync/SyncReactive/NodeSelection/AggregateInterface/ CommandBuilderCoverage/KotlinCoroutines consistency tests The suite checks method parity in both directions, the per-flavor return-type mapping (RedisFuture, Mono/Flux, suspend/Flow, Executions), aggregate interface wiring and interface-to-command- builder coverage. Sync/async parity is load-bearing at runtime: the sync API is a dynamic proxy over async (FutureSyncInvocationHandler). Supersedes SyncAsyncApiConvergenceUnitTests. Adds kotlin-reflect as a test dependency. Note: at this commit the suite documents real drift in the committed interfaces — several tests fail intentionally; the deviations are fixed in the follow-up commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Real drift found by the new consistency tests, accumulated while the API generators were out of use: - RedisCoroutinesCommands did not extend RediSearchCoroutinesCommands (the implementation already delegated to it) - Kotlin coroutine API was missing bitopDiff/bitopDiff1/bitopAndor/ bitopOne, hgetex(key, fields), hsetex(key, map) and the xackdel/xdelex stream commands - reactive hgetdel/hgetex streaming-channel variants were missing their @deprecated marker The node-selection aggregate wiring drift (missing ACL/Array groups, async aggregate extending the sync Stream interface) is exempted via KnownApiDeviations.NODE_SELECTION_AGGREGATE_PENDING — fixing it is a breaking change scheduled for the 8.0 release. Makes the API consistency test suite pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- verify the methods declared directly on the aggregate interfaces (auth, select, the CLUSTER commands, ...) against their async and reactive counterparts, restoring coverage the removed SyncAsyncApiConvergenceUnitTests had via RedisCommands.getMethods() - include RedisClusterCoroutinesCommands in the aggregate wiring check and add its missing FUNCTION/JSON/VECTOR_SET/ARRAY/SEARCH groups (experimental API; the impl already delegates per group) - keep the Kotlin generator's FLOW_METHODS in sync with the hand-added Flow methods for as long as the generators still exist - add behavioral coroutine integration tests for the new hash-field-expiry, BITOP and stream-deletion commands Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Erased-parameter matching alone would accept flavors whose generic parameter signatures diverge (e.g. Map<K, V> vs Map<V, K>), and nothing compared @deprecated across the Java flavors. The suite now verifies both for sync/async, sync/reactive, node-selection and the aggregate-declared methods. The reactive dispatch parameter mutation (CommandOutput<K, V, ?>) and the intentional deprecation of the CommandOutput-based node-selection dispatch overloads are recorded in KnownApiDeviations. Fixes surfaced by the new checks: RedisAdvancedClusterReactiveCommands .keysLegacy and RedisAdvancedClusterAsyncCommands.masters carried a @deprecated tag without the matching @deprecated annotation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Coroutine return types are effectively unchecked. Coroutine parameter generics unchecked. The catalog isn't verified complete. CommandInterfaces calls itself "the single place where a new command group must be registered", but nothing asserts every io.lettuce.core.api.sync.*Commands interface appears in it. A group added without registering is invisible PubSub isn't covered at all. Coroutine aggregates only get an extends check. AGGREGATE_FLAVORS (:101-105) is sync/async/reactive triples; RedisCoroutinesCommands/RedisClusterCoroutinesCommands declare their own methods, and the branch had to hand-patch the latter. Javadoc is out of scope entirely Staleness is tested for one table only.
463a98b to
8354828
Compare
3 participants
Introduces a reflection-based unit-test suite that verifies the sync, async, reactive, Kotlin coroutine and cluster node-selection command interfaces stay in lockstep, replacing the consistency-by-regeneration guarantee of the template-based API generators (removal of the generators follows in #3866).
What the suite checks
RedisFuture<T>return-type wrapping. This is load-bearing at runtime: the sync API is a dynamic proxy over async (FutureSyncInvocationHandler), so a mismatch previously failed only at call time.Mono/Fluxmapping rules (collections →Flux, forced-Flux,Value<>-wrapped elements) and@Deprecatedon streaming-channel variants.suspend/Flowshape (viakotlin-reflect, new test-scope dependency managed by the Kotlin BOM).Executions/AsyncExecutionswrapping and the exclusion list, with a staleness check on the list itself.RedisCommands,RedisClusterCommands,NodeSelectionCommands,RedisClusterCoroutinesCommandsetc. extend every command group they must cover, and the methods declared directly on the aggregates (auth,select, theCLUSTERcommands, …) are parity-checked against their async/reactive counterparts.Structure:
CommandInterfaces(group catalog — the single place to register a new command group),KnownApiDeviations(all intentional irregularities, ported from the generator rule tables),TypeSignatures(shared reflection helpers). SupersedesSyncAsyncApiConvergenceUnitTests.Deviations found and fixed
Calibrating the suite against the committed interfaces surfaced real drift, fixed in the second commit:
RedisCoroutinesCommandsdid not extendRediSearchCoroutinesCommands(the impl already delegated to it)bitopDiff/bitopDiff1/bitopAndor/bitopOne,hgetex(key, fields),hsetex(key, map)and thexackdel/xdelexstream commandshgetdel/hgetexstreaming-channel variants were missing@DeprecatedDeviations found and deferred
The node-selection aggregate wiring drift (missing ACL/Array groups;
NodeSelectionAsyncCommandsextending the syncNodeSelectionStreamCommands) is not fixed here — correcting it changes return types onAsyncNodeSelectionand is therefore a breaking change. It is exempted viaKnownApiDeviations.NODE_SELECTION_AGGREGATE_PENDING(with a documenting comment) and fixed in #3867, targeted for the 8.0.0 release.Note: the first commit intentionally fails the new tests (it demonstrates the drift); the second commit makes the suite green.
Make sure that:
mvn formatter:formattarget. Don't submit any formatting related changes.🤖 Generated with Claude Code
Note
Low Risk
Changes are mostly tests and public interface/Javadoc declarations; runtime behavior is unchanged aside from newly exposed Kotlin coroutine methods.
Overview
Adds a reflection- and source-based test suite so sync, async, reactive, Kotlin coroutines, cluster node-selection, aggregate umbrellas, command builders, and Javadoc stay aligned—replacing the old
SyncAsyncApiConvergenceUnitTestsand the consistency guarantee that used to come from regenerating templates.The suite is driven by hand-maintained catalogs (
CommandInterfaces,AggregateInterfaces) and deviation registries (KnownApiDeviations,KnownKotlinApiDeviations), with staleness checks so opt-out entries cannot rot.kotlin-reflectis added as a test dependency for coroutine shape checks.API fixes surfaced while calibrating the suite:
RedisCoroutinesCommandsnow extendsRediSearchCoroutinesCommands; cluster coroutine aggregates pick up Function/JSON/VectorSet/Array/RediSearch; Kotlin gains BITOP variants, simpler hsetex/hgetex, and xackdel/xdelex; reactive hgetdel/hgetex channel overloads are@Deprecated. Generated interfaces get broad Javadoc cleanup (@param,@deprecatedongetStatefulConnection, etc.). Node-selection aggregate gaps remain documented in deviations for a later breaking fix (#3867).Reviewed by Cursor Bugbot for commit 8354828. Bugbot is set up for automated code reviews on this repo. Configure here.