Add builders for classes based on UnifiedJedis

[uglide]

Motivation

This change creates a foundation for providing a new client class(es) for AA failover, allowing developers to easily instantiate JedisPooled, JedisSentineled, JedisCluster, etc.

Review

Summary

This commit introduces a formal Builder pattern for UnifiedJedis-based clients (JedisPooled, JedisCluster, JedisSentineled) and adds a shared builder base (AbstractClientBuilder) plus specialized builders for standalone, cluster, and sentinel deployments. It also exposes a couple of configuration hooks, aligns RediSearch default dialect handling, and adds reflection-based tests to ensure builder coverage over the existing constructor surfaces. Build tooling is adjusted to preserve parameter names for those tests.

---

High-level Impact

• Additive public API: new static builder() entry points and builder classes under redis.clients.jedis.builders.
• Internal visibility tweak: UnifiedJedis constructor made package-visible to allow subclass super(...) calls.
• Slightly broader access: CommandObjects.setKeyArgumentPreProcessor() is now public.
• Tests and tooling: reflection-based tests to validate builder coverage; Maven profile to compile with parameter names.
• Behavioral defaults remain intact (e.g., RediSearch DEFAULT_DIALECT still 2, now referenced via constant).

---

Detailed Changes

1) Builders

• New classes:
  • redis.clients.jedis.builders.AbstractClientBuilder<T,C>
  • redis.clients.jedis.builders.StandaloneClientBuilder
  • redis.clients.jedis.builders.ClusterClientBuilder
  • redis.clients.jedis.builders.SentinelClientBuilder

Key capabilities (AbstractClientBuilder):

• Common options: poolConfig, cache/cacheConfig, connectionProvider, commandExecutor, commandObjects, redisProtocol, key preprocessor, JSON mapper, search dialect.
• Build flow: validate -> optional cache from cacheConfig -> default provider/executor if unset -> apply common config to CommandObjects -> instantiate concrete client.
• Cache implies RESP3: calling cache(...) or cacheConfig(...) forces redisProtocol=RESP3 (UnifiedJedis guards against cache+RESP2).
• applyCommonConfiguration wires:
  • CommandObjects.setKeyArgumentPreProcessor(...)
  • CommandObjects.setJsonObjectMapper(...)
  • CommandObjects.setDefaultSearchDialect(...) when non-default

Specialized builders:

• StandaloneClientBuilder
  • hostAndPort(String,int|HostAndPort), clientConfig(JedisClientConfig)
  • fromURI(String|URI) builds DefaultJedisClientConfig based on URI and sets host/port
  • Default provider: PooledConnectionProvider(hostAndPort, config, cache, poolConfig)
  • Validation intends to ensure either URI or host/port, not both
• ClusterClientBuilder
  • nodes(Set), clientConfig, maxAttempts(int), maxTotalRetriesDuration(Duration), topologyRefreshPeriod(Duration)
  • Default provider: ClusterConnectionProvider(nodes, config, cache, poolConfig, topologyRefreshPeriod)
  • Validates nodes non-empty and non-negative durations/attempts
• SentinelClientBuilder
  • masterName(String), sentinels(Set), clientConfig, sentinelClientConfig
  • Default provider: SentineledConnectionProvider(masterName, clientConfig, cache, poolConfig, sentinels, sentinelClientConfig)
  • Validates required fields

2) Jedis* classes integrate builders

• JedisPooled, JedisCluster, JedisSentineled:
  • Add private constructors accepting (CommandExecutor, ConnectionProvider, CommandObjects, RedisProtocol, Cache) and delegating to super(...)
  • Add public static inner Builder extends respective specialized builder
  • Add public static Builder builder() factory method

3) UnifiedJedis visibility

• UnifiedJedis(CommandExecutor, ConnectionProvider, CommandObjects, RedisProtocol, Cache) constructor changed from private to package-visible to enable subclass super(...) calls from Jedis*.

4) CommandObjects tuning

• Replace hard-coded default search dialect (2) with SearchProtocol.DEFAULT_DIALECT.
• setKeyArgumentPreProcessor made public (was @experimental with package access). This enables configuration via builders.

5) RediSearch protocol constant usage

• SearchDefaultDialectTest and CommandObjects now reference SearchProtocol.DEFAULT_DIALECT rather than literals. SearchProtocol.DEFAULT_DIALECT remains 2.

6) Build tooling to support reflection tests

• Makefile: mvn-test-local and mvn-test now pass -Dwith-param-names=true.
• pom.xml: new profile tests-with-params activated via -Dwith-param-names=true, enabling:
  • maven-compiler-plugin true (preserve method/parameter names)
  • maven-surefire-plugin argLine includes --add-opens java.base/java.lang=ALL-UNNAMED ${JVM_OPTS}

7) Tests

• New reflection-based coverage tests:
  • builders/JedisPooledConstructorReflectionTest
  • builders/JedisClusterConstructorReflectionTest
  • builders/JedisSentineledConstructorReflectionTest
  • builders/UnifiedJedisConstructorReflectionTest
• Purpose: verify that for every constructor of the target classes, the builder surface offers an equivalent configuration path (maps each parameter type/name to a builder option). Tests assert full coverage and log any gaps.

---

API/Compatibility Assessment

• Additive: New builder APIs are additive; existing constructors remain.
• Internal constructor visibility change is not exposed as public API but is a package-level relaxation; safe and contained.
• Making setKeyArgumentPreProcessor public increases surface area; consider de-@experimental or documenting stability expectations.
• No default behavioral changes beyond wiring options when using the builder.

[github-actions]

Test Results

  281 files  + 6    281 suites  +6   10m 40s ⏱️ -4s
5 095 tests +19  5 091 ✅ +356  4 💤  - 337  0 ❌ ±0 
2 641 runs  +19  2 641 ✅ + 19  0 💤 ±  0  0 ❌ ±0 

Results for commit 3446743. ± Comparison against base commit f3ab77a.

♻️ This comment has been updated with latest results.

[ggivo]

Approach with the builders looks good.
I have put some comments, and have concerns around tests validating the constructor coverage.

[ggivo]

Not sure about silently overriding the protocol.
I will suggest validating it during build if cache is configured and configured protocol does not support it..

[uglide]

Fixed

[ggivo]

Same as above, don't think we should silently set the protocol;

[uglide]

Fixed

[ggivo]

What about using custom connection provider that does not require poolConfig?

I think pool config should be optional

[uglide]

Fixed

[ggivo]

Adding new components (metrics, tracing, etc.) would require breaking changes. No need to pass explicitly commandExecutor,... They are already available to extending classes

[uglide]

Good catch, thanks!

[uglide]

Fixed

[ggivo]

related to previous comment. Don't think we should silently set the protocol when enabling client side cache

[uglide]

Fixed

[ggivo]

Relates to previous comment. We should not enforce poolConfig as required. For example when using custom ConnectionProvide we might not use appache commons pool

[uglide]

This test is just off; it should test the host and port instead. I'm going to fix it.

[uglide]

Done

[ggivo]

What is the reason for Illegal argument exception? Should we have sensible defaults and build() pass with them?

[uglide]

Primary name and sentinels config are required and cannot have a default value. See https://github.com/redis/jedis/pull/4263/files/183b00d9046022a45ebe162c1342663c4f53d1b2#diff-af5d7cdfaeacc02ac468d5007e9860e001b9f90f80b80288ce3dccd22ae390b7R93-R104 for more details

[ggivo]

static imports

[uglide]

Fixed

[ggivo]

I have some concerns about the current reflection-based testing approach in JedisPooledConstructorReflectionTest.
The tests rely on validating hardcoded internal state, which can become outdated quickly and may fail to catch real functional differences.

Some issues with this approach:

• It can lead to false positives.
• It only validates that parameters can be provided, not that they work correctly.

Instead of reflection-based coverage validation, have you considered testing functional equivalence between constructor-created and builder-created clients?
This way, the tests also serve a documentation purpose while validating actual functionality parity.

For example:

@Test  
void constructorBuilderEquivalence() {  
    // Ensure constructor and builder produce functionally equivalent clients  
    
    // Constructor approach  
    JedisPooled constructorClient = new JedisPooled("localhost", 6379, "user", "pass");  
    
    // Builder approach    
    JedisPooled builderClient = JedisPooled.builder()  
        .hostAndPort("localhost", 6379)  
        .clientConfig(DefaultJedisClientConfig.builder()  
            .user("user")  
            .password("pass")  
            .build())  
        .build();  
    
    // Validate functional equivalence  
    assertClientsEquivalent(constructorClient, builderClient);  
}  

Where assertClientsEquivalent(...) could validate the equivalence of initialized client components such as:

• ConnectionProvider provider
• Pool<Connection> pool
• CommandExecutor executor
• CommandObjects commandObjects
• Cache cache

[uglide]

The approach I used serves as a safeguard to prevent the addition of new constructors and provides an efficient way to check the builder's coverage. Once we reach Jedis v9, we should drop it altogether with all deprecated constructors.

Another problem with functional tests is that they only work for simple cases like the one you have mentioned. Also, creating reasonable assertClientsEquivalent is quite tricky. I gave up on that approach after seeing that it became a reflection monster to expose all the internal states of UnifiedJedis and co. At best, with a functional approach, we end up testing that both client instances can connect to the same redis-server instance, which is not sufficient and can be achieved by just instantiating UnifiedJedis and co with the new builder in plenty of integration tests that we already have.

[ggivo]

I see your point.

Yes, I was thinking that we need a mix of functional tests and unit tests. We can still use the reflection, but based on comparing the actual objects built. e.g, ConnectionProvider built via the constructor is equivalent to ConnectionProvider built with the corresponding builder.

IMO, the current approach is also hard to verify via review.

It might take some time till v9 is out. Existing tests will give some confidence, but I still think it makes sense to open a follow-up issue and add additional tests focused on object build using a concrete constructor vs the corresponding one using builder.