fix fabric8io#3349: adding consistency to the handling of option values

CHANGELOG.md

@@ -9,6 +9,7 @@
 #### Improvements
 * Fix #3316: allow locking deletion to resource version
 * Fix #3327: Removed generated ResourceHandlers
+* Fix #3349: ensuring that dsl context values always are applied over user ListOptions
 
 #### Dependency Upgrade
 

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/ResourceHandler.java

@@ -22,7 +22,6 @@
 import io.fabric8.kubernetes.api.model.DeletionPropagation;
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.api.model.KubernetesResourceList;
-import io.fabric8.kubernetes.api.model.ListOptions;
 import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.dsl.base.HasMetadataOperation;
 import okhttp3.OkHttpClient;
@@ -90,49 +89,6 @@ default Boolean delete(OkHttpClient client, Config config, String namespace, Del
     return resource(client, config, namespace, item).dryRun(dryRun).withPropagationPolicy(propagationPolicy).withGracePeriod(gracePeriodSeconds).delete();
   }
 
-
-    /**
-     * Watches the specified resource for changes.
-     * @param client        An instance of the http client.
-     * @param config        The client config.
-     * @param namespace     The target namespace.
-     * @param item          The resource to delete.
-     * @param watcher       The {@link Watcher} to use.
-     * @return              The {@link Watch}
-     */
-  default Watch watch(OkHttpClient client, Config config, String namespace, T item, Watcher<T> watcher) {
-    return resource(client, config, namespace, item).watch(watcher);
-  }
-
-  /**
-   * Watches the specified resource for changes.
-   * @param client          An instance of the http client.
-   * @param config          The client config.
-   * @param namespace       The target namespace.
-   * @param item            The resource to delete.
-   * @param resourceVersion The resourceVersion of object
-   * @param watcher         The {@link Watcher} to use.
-   * @return                The {@link Watch}
-   */
-  default Watch watch(OkHttpClient client, Config config, String namespace, T item, String resourceVersion, Watcher<T> watcher) {
-    return resource(client, config, namespace, item).watch(resourceVersion, watcher);
-  }
-
-  /**
-   * Watches the specified resource for changes
-   *
-   * @param client         An instance of http client.
-   * @param config         The client config.
-   * @param namespace      The target namespace.
-   * @param item           The resource to delete.
-   * @param listOptions    The {@link io.fabric8.kubernetes.api.model.ListOptions} for available options
-   * @param watcher        The {@link Watcher} to use.
-   * @return               The {@link Watch}
-   */
-  default Watch watch(OkHttpClient client, Config config, String namespace, T item, ListOptions listOptions, Watcher<T> watcher) {
-    return resource(client, config, namespace, item).watch(listOptions, watcher);
-  }
-
   /**
    * Waits until the specified resource is Ready.
    * For resources that 'readiness' is not applicable the method is equivalent to get.

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/Listable.java

@@ -24,7 +24,6 @@ public interface Listable<T> {
    * List resources from APIServer.
    * @deprecated : Please use {@link #list(ListOptions)} instead
    *
-   *
    * @param limitVal number of resources to list
    * @param continueVal an offset to pick listing from
    * @return resource list
@@ -34,6 +33,9 @@ public interface Listable<T> {
 
   /**
    * List resource from Kubernetes API server.
+   * <p>The passed in options may be modified as a side-effect of this call.
+   * <br>Values that already exist at this context, such as the labels and fields will be overridden
+   *  on the passed in options regardless of initial values.
    *
    * @param listOptions ListOptions is the query options to a standard REST list call.
    * @return list of resource type

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/Watchable.java

@@ -30,6 +30,9 @@ public interface Watchable<W> {
 
   /**
    * Watch returns {@link Watch} interface that watches requested resource
+   * <p>The passed in options may be modified as a side-effect of this call.
+   * <br>Values that already exist at this context, such as the labels, fields, 
+   * and resourceVersion will be overridden on the passed in options regardless of initial values.
    *
    * @param options options available for watch operation
    * @param watcher Watcher interface of Kubernetes resource

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java

@@ -47,7 +47,6 @@
 import io.fabric8.kubernetes.client.dsl.MixedOperation;
 import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation;
 import io.fabric8.kubernetes.client.dsl.ReplaceDeletable;
-import io.fabric8.kubernetes.client.dsl.Replaceable;
 import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.dsl.internal.DefaultOperationInfo;
 import io.fabric8.kubernetes.client.dsl.internal.WatchConnectionManager;
@@ -91,7 +90,8 @@ public class BaseOperation<T extends HasMetadata, L extends KubernetesResourceLi
   implements
   OperationInfo,
   MixedOperation<T, L, R>,
-  Resource<T> {
+  Resource<T>,
+  ListerWatcher<T, L> {
 
   private static final String READ_ONLY_UPDATE_EXCEPTION_MESSAGE = "Cannot update read-only resources";
   private static final String READ_ONLY_EDIT_EXCEPTION_MESSAGE = "Cannot edit read-only resources";
@@ -134,9 +134,6 @@ private L listRequestHelper(URL url) {
     try {
       HttpUrl.Builder requestUrlBuilder = HttpUrl.get(url).newBuilder();
 
-      addQueryStringParam(requestUrlBuilder, "labelSelector", getLabelQueryParam());
-      addQueryStringParam(requestUrlBuilder, "fieldSelector", getFieldQueryParam());
-
       Request.Builder requestBuilder = new Request.Builder().get().url(requestUrlBuilder.build());
       L answer = handleResponse(requestBuilder, listType);
       updateApiVersion(answer);
@@ -153,12 +150,6 @@ protected URL fetchListUrl(URL url, ListOptions listOptions) throws MalformedURL
     return new URL(HttpClientUtils.appendListOptionParams(HttpUrl.get(url.toString()).newBuilder(), listOptions).toString());
   }
 
-  private void addQueryStringParam(HttpUrl.Builder requestUrlBuilder, String name, String value) {
-    if (Utils.isNotNullOrEmpty(value)) {
-      requestUrlBuilder.addQueryParameter(name, value);
-    }
-  }
-
   @Override
   public T get() {
     try {
@@ -515,11 +506,17 @@ public String getLabelQueryParam() {
         sb.append(entry.getKey()).append(" notin ").append("(").append(Utils.join(entry.getValue())).append(")");
       }
     }
-    return sb.toString();
+    if (sb.length() > 0) {
+      return sb.toString();
+    }
+    return null;
   }
 
   public String getFieldQueryParam() {
     StringBuilder sb = new StringBuilder();
+    if (Utils.isNotNullOrEmpty(context.getName())) {
+      sb.append("metadata.name").append("=").append(context.getName());
+    }
     Map<String, String> fields = context.getFields();
     if (fields != null && !fields.isEmpty()) {
       for (Iterator<Map.Entry<String, String>> iter = fields.entrySet().iterator(); iter.hasNext(); ) {
@@ -545,29 +542,51 @@ public String getFieldQueryParam() {
         }
       }
     }
-    return sb.toString();
+    if (sb.length() > 0) {
+      return sb.toString();
+    }
+    return null;
   }
 
+  @Override
   public L list() {
-    try {
-      return listRequestHelper(getResourceUrl(namespace, name));
-    } catch (IOException e) {
-      throw KubernetesClientException.launderThrowable(forOperationType("list"), e);
-    }
+    return list(new ListOptions());
   }
 
+  @Override
   public L list(Integer limitVal, String continueVal) {
     return list(new ListOptionsBuilder().withLimit(Long.parseLong(limitVal.toString())).withContinue(continueVal).build());
   }
 
+  @Override
   public L list(ListOptions listOptions) {
     try {
-      return listRequestHelper(fetchListUrl(getNamespacedUrl(), listOptions));
+      return listRequestHelper(
+          fetchListUrl(getNamespacedUrl(), defaultListOptions(listOptions, null)));
     } catch (MalformedURLException e) {
       throw KubernetesClientException.launderThrowable(forOperationType("list"), e);
     }
   }
 
+  /**
+   * Override the options based upon the context / call
+   */
+  private ListOptions defaultListOptions(ListOptions options, Boolean watch) {
+    options.setWatch(watch);
+    String fieldQueryParam = getFieldQueryParam();
+    if (fieldQueryParam != null) {
+      options.setFieldSelector(fieldQueryParam);
+    }
+    String lableQueryParam = getLabelQueryParam();
+    if (lableQueryParam != null) {
+      options.setLabelSelector(lableQueryParam);
+    }
+    if (resourceVersion != null) {
+      options.setResourceVersion(resourceVersion);
+    }
+    return options;
+  }
+
   @Override
   public Boolean delete() {
     if (item != null || (name != null && !name.isEmpty())) {
@@ -668,14 +687,13 @@ void deleteList() {
   }
 
   @Override
-  public R withResourceVersion(String resourceVersion) {
-    return (R) newInstance(context.withResourceVersion(resourceVersion));
+  public BaseOperation<T, L, R> withResourceVersion(String resourceVersion) {
+    return newInstance(context.withResourceVersion(resourceVersion));
   }
 
+  @Override
   public Watch watch(final Watcher<T> watcher) {
-    return watch(new ListOptionsBuilder()
-      .withResourceVersion(resourceVersion)
-      .build(), watcher);
+    return watch(new ListOptions(), watcher); 
   }
 
   @Override
@@ -688,7 +706,7 @@ public Watch watch(String resourceVersion, Watcher<T> watcher) {
   @Override
   public Watch watch(ListOptions options, final Watcher<T> watcher) {
     WatcherToggle<T> watcherToggle = new WatcherToggle<>(watcher, true);
-    options.setWatch(Boolean.TRUE);
+    options = defaultListOptions(options, true);
     WatchConnectionManager<T, L> watch = null;
     try {
       watch = new WatchConnectionManager<>(
@@ -869,6 +887,7 @@ public DeletionPropagation getPropagationPolicy() {
     return propagationPolicy;
   }
 
+  @Override
   public String getResourceT() {
     return resourceT;
   }
@@ -1069,30 +1088,12 @@ public SharedIndexInformer<T> inform(ResourceEventHandler<T> handler, long resyn
 
   private DefaultSharedIndexInformer<T, L> createInformer(long resync) {
     T i = getItem();
-    String name = (Utils.isNotNullOrEmpty(getName()) || i != null) ? checkName(i) : null;
-
-    // use the local context / namespace
-    DefaultSharedIndexInformer<T, L> informer = new DefaultSharedIndexInformer<>(getType(), new ListerWatcher<T, L>() {
-      @Override
-      public L list(ListOptions params) {
-        params.setWatch(Boolean.FALSE); // for test compatibility
-        // convert the name into something listable
-        if (name != null) {
-          params.setFieldSelector("metadata.name="+name);
-        }
-        return BaseOperation.this.list(params);
-      }
+    if (Utils.isNotNullOrEmpty(getName()) && i != null) {
+      checkName(i);
+    }
 
-      @Override
-      public Watch watch(ListOptions params, Watcher<T> watcher) {
-        return BaseOperation.this.watch(params, watcher);
-      }
-
-      @Override
-      public String getNamespace() {
-        return context.getNamespace();
-      }
-    }, resync, Runnable::run); // just run the event notification in the websocket thread
+    // use the local context / namespace but without a resourceVersion
+    DefaultSharedIndexInformer<T, L> informer = new DefaultSharedIndexInformer<>(getType(), this.withResourceVersion(null), resync, Runnable::run); // just run the event notification in the websocket thread
     if (indexers != null) {
       informer.addIndexers(indexers);
     }

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperationRequestBuilder.java

@@ -26,7 +26,6 @@
 import io.fabric8.kubernetes.client.Config;
 import io.fabric8.kubernetes.client.dsl.base.BaseOperation;
 import io.fabric8.kubernetes.client.utils.HttpClientUtils;
-import io.fabric8.kubernetes.client.utils.Utils;
 import okhttp3.HttpUrl;
 import okhttp3.Request;
 
@@ -48,24 +47,6 @@ public BaseOperationRequestBuilder(BaseOperation<T, L, ?> baseOperation, ListOpt
   public Request build(final String resourceVersion) {
     HttpUrl.Builder httpUrlBuilder = HttpUrl.get(requestUrl).newBuilder();
 
-    String labelQueryParam = baseOperation.getLabelQueryParam();
-    if (Utils.isNotNullOrEmpty(labelQueryParam)) {
-      httpUrlBuilder.addQueryParameter("labelSelector", labelQueryParam);
-    }
-
-    String fieldQueryString = baseOperation.getFieldQueryParam();
-    String name = baseOperation.getName();
-
-    if (name != null && name.length() > 0) {
-      if (fieldQueryString.length() > 0) {
-        fieldQueryString += ",";
-      }
-      fieldQueryString += "metadata.name=" + name;
-    }
-    if (Utils.isNotNullOrEmpty(fieldQueryString)) {
-      httpUrlBuilder.addQueryParameter("fieldSelector", fieldQueryString);
-    }
-
     listOptions.setResourceVersion(resourceVersion);
     HttpClientUtils.appendListOptionParams(httpUrlBuilder, listOptions);
 

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java

@@ -21,7 +21,6 @@
 import io.fabric8.kubernetes.client.utils.Utils;
 
 import java.util.function.Predicate;
-
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.List;
@@ -45,6 +44,7 @@
 import io.fabric8.kubernetes.client.dsl.Gettable;
 import io.fabric8.kubernetes.client.dsl.NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicable;
 import io.fabric8.kubernetes.client.dsl.Readiable;
+import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.dsl.VisitFromServerGetWatchDeleteRecreateWaitApplicable;
 import io.fabric8.kubernetes.client.dsl.Waitable;
 import io.fabric8.kubernetes.client.dsl.base.OperationSupport;
@@ -209,23 +209,23 @@ public Waitable<HasMetadata, HasMetadata> withWaitRetryBackoff(long initialBacko
 
   @Override
   public Watch watch(Watcher<HasMetadata> watcher) {
-    HasMetadata meta = get();
-    ResourceHandler<HasMetadata, ?> h = handlerOf(meta);
-    return h.watch(client, config, meta.getMetadata().getNamespace(), meta, watcher);
+    return getResource().watch(watcher);
   }
 
   @Override
   public Watch watch(String resourceVersion, Watcher<HasMetadata> watcher) {
-    HasMetadata meta = get();
-    ResourceHandler<HasMetadata, ?> h = handlerOf(meta);
-    return h.watch(client, config, meta.getMetadata().getNamespace(), meta, resourceVersion, watcher);
+    return getResource().watch(resourceVersion, watcher);
   }
 
   @Override
   public Watch watch(ListOptions options, Watcher<HasMetadata> watcher) {
+    return getResource().watch(options, watcher);
+  }
+
+  Resource<HasMetadata> getResource() {
     HasMetadata meta = get();
     ResourceHandler<HasMetadata, ?> h = handlerOf(meta);
-    return h.watch(client, config, meta.getMetadata().getNamespace(), meta, options, watcher);
+    return h.resource(client, config, meta.getMetadata().getNamespace(), meta);
   }
 
   protected Readiness getReadiness() {

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/ListerWatcher.java

@@ -29,7 +29,7 @@
 public interface ListerWatcher<T, L> {
   Watch watch(ListOptions params, Watcher<T> watcher);
 
-  L list(ListOptions params);
+  L list();
 
   String getNamespace();
 }