redis / lettuce

Advanced Java Redis client for thread-safe sync, async, and reactive usage. Supports Cluster, Sentinel, Pipelining, and codecs.
https://lettuce.io
MIT License
5.36k stars 959 forks source link

`ClusterTopologyRefreshOptions.Builder. enableAdaptiveRefreshTrigger (…)` without options should throw `IllegalArgumentException` #2575

Closed zzmmff closed 8 months ago

zzmmff commented 9 months ago

Bug Report

I set the cluster topology refresh options in my code

    ClusterTopologyRefreshOptions clusterTopologyRefreshOptions = ClusterTopologyRefreshOptions.builder()
              .enableAdaptiveRefreshTrigger()
              // 开启定时刷新
              .enablePeriodicRefresh(Duration.ofSeconds(5))
              .build();

The code is wrong,because no param passed to the enableAdaptiveRefreshTrigger method.

The ClusterTopologyRefreshOptions::Builder::enableAdaptiveRefreshTrigger method souce code :

   /**
         * Enables adaptive topology refreshing using one or more {@link RefreshTrigger triggers}. Adaptive refresh triggers
         * initiate topology view updates based on events happened during Redis Cluster operations. Adaptive triggers lead to an
         * immediate topology refresh. Adaptive triggered refreshes are rate-limited using a timeout since events can happen on
         * a large scale. Adaptive refresh triggers are disabled by default. See also
         * {@link #adaptiveRefreshTriggersTimeout(long, TimeUnit)} and {@link RefreshTrigger}.
         *
         * @param refreshTrigger one or more {@link RefreshTrigger} to enabled
         * @return {@code this}
         */
        public Builder enableAdaptiveRefreshTrigger(RefreshTrigger... refreshTrigger) {

            LettuceAssert.notNull(refreshTrigger, "RefreshTriggers must not be null");
            LettuceAssert.noNullElements(refreshTrigger, "RefreshTriggers must not contain null elements");

            adaptiveRefreshTriggers.addAll(Arrays.asList(refreshTrigger));
            return this;
        }

I think the method should throw IllegalArgumentException if I don't pass any param to the method. But enableAdaptiveRefreshTrigger method didn't work like this,if you don't pass any params to the method ,it wouldn't throw any exceptions (Like the case I mentioned).I think this is a bug

Current Behavior

throw IllegalArgumentException

mp911de commented 8 months ago

Varargs methods are easy to misuse, especially in this context, because it isn't apparent that you should specify values. We can add an exception here to indicate that no argument has been supplied. We have tons of other places that also accept varargs but these lead on command execution to failures.