refactor: isolate index handling to BulkDependentResource interface

Author: metacosm

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java

@@ -1,7 +1,5 @@
 package io.javaoperatorsdk.operator.processing.dependent;
 
-import java.util.ArrayList;
-import java.util.List;
 import java.util.Optional;
 
 import org.slf4j.Logger;
@@ -20,15 +18,15 @@ public abstract class AbstractDependentResource<R, P extends HasMetadata>
     implements DependentResource<R, P> {
   private static final Logger log = LoggerFactory.getLogger(AbstractDependentResource.class);
 
-  protected final boolean creatable = this instanceof Creator;
-  protected final boolean updatable = this instanceof Updater;
-  protected final boolean bulk = this instanceof BulkDependentResource;
+  private final boolean creatable = this instanceof Creator;
+  private final boolean updatable = this instanceof Updater;
+  private final boolean bulk = this instanceof BulkDependentResource;
 
   protected Creator<R, P> creator;
   protected Updater<R, P> updater;
-  protected BulkDependentResource<R, P> bulkDependentResource;
-
-  private final List<ResourceDiscriminator<R, P>> resourceDiscriminator = new ArrayList<>(1);
+  private final BulkDependentResource<R, P> bulkDependentResource;
+  private ResourceDiscriminator<R, P> resourceDiscriminator;
+  private int currentCount;
 
   @SuppressWarnings("unchecked")
   public AbstractDependentResource() {
@@ -42,48 +40,33 @@ public AbstractDependentResource() {
   public ReconcileResult<R> reconcile(P primary, Context<P> context) {
     if (bulk) {
       final var count = bulkDependentResource.count(primary, context);
-      deleteBulkResourcesIfRequired(count, lastKnownBulkSize(), primary, context);
-      adjustDiscriminators(count);
+      deleteBulkResourcesIfRequired(count, primary, context);
       @SuppressWarnings("unchecked")
       final ReconcileResult<R>[] results = new ReconcileResult[count];
       for (int i = 0; i < count; i++) {
         results[i] = reconcileIndexAware(primary, i, context);
       }
+      currentCount = count;
       return ReconcileResult.aggregatedResult(results);
     } else {
       return reconcileIndexAware(primary, 0, context);
     }
   }
 
-  protected void deleteBulkResourcesIfRequired(int targetCount, int actualCount, P primary,
-      Context<P> context) {
-    if (targetCount >= actualCount) {
+  protected void deleteBulkResourcesIfRequired(int targetCount, P primary, Context<P> context) {
+    if (targetCount >= currentCount) {
       return;
     }
-    for (int i = targetCount; i < actualCount; i++) {
-      var resource = getSecondaryResourceIndexAware(primary, i, context);
+    for (int i = targetCount; i < currentCount; i++) {
+      var resource = bulkDependentResource.getSecondaryResource(primary, i, context);
       var index = i;
       resource.ifPresent(
           r -> bulkDependentResource.deleteBulkResourceWithIndex(primary, r, index, context));
     }
   }
 
-  private void adjustDiscriminators(int count) {
-    if (resourceDiscriminator.size() == count) {
-      return;
-    }
-    if (resourceDiscriminator.size() < count) {
-      for (int i = resourceDiscriminator.size(); i < count; i++) {
-        resourceDiscriminator.add(bulkDependentResource.getResourceDiscriminator(i));
-      }
-    }
-    if (resourceDiscriminator.size() > count) {
-      resourceDiscriminator.subList(count, resourceDiscriminator.size()).clear();
-    }
-  }
-
   protected ReconcileResult<R> reconcileIndexAware(P primary, int i, Context<P> context) {
-    Optional<R> maybeActual = bulk ? getSecondaryResourceIndexAware(primary, i, context)
+    Optional<R> maybeActual = bulk ? bulkDependentResource.getSecondaryResource(primary, i, context)
         : getSecondaryResource(primary, context);
     if (creatable || updatable) {
       if (maybeActual.isEmpty()) {
@@ -99,7 +82,7 @@ protected ReconcileResult<R> reconcileIndexAware(P primary, int i, Context<P> co
         if (updatable) {
           final Matcher.Result<R> match;
           if (bulk) {
-            match = updater.match(actual, primary, i, context);
+            match = bulkDependentResource.match(actual, primary, i, context);
           } else {
             match = updater.match(actual, primary, context);
           }
@@ -124,17 +107,12 @@ protected ReconcileResult<R> reconcileIndexAware(P primary, int i, Context<P> co
   }
 
   private R desiredIndexAware(P primary, int i, Context<P> context) {
-    return bulk ? desired(primary, i, context)
-        : desired(primary, context);
+    return bulk ? desired(primary, i, context) : desired(primary, context);
   }
 
   public Optional<R> getSecondaryResource(P primary, Context<P> context) {
-    return resourceDiscriminator.isEmpty() ? context.getSecondaryResource(resourceType())
-        : resourceDiscriminator.get(0).distinguish(resourceType(), primary, context);
-  }
-
-  protected Optional<R> getSecondaryResourceIndexAware(P primary, int index, Context<P> context) {
-    return context.getSecondaryResource(resourceType(), resourceDiscriminator.get(index));
+    return resourceDiscriminator == null ? context.getSecondaryResource(resourceType())
+        : resourceDiscriminator.distinguish(resourceType(), primary, context);
   }
 
   private void throwIfNull(R desired, P primary, String descriptor) {
@@ -195,28 +173,35 @@ protected R desired(P primary, Context<P> context) {
   }
 
   protected R desired(P primary, int index, Context<P> context) {
-    throw new IllegalStateException(
-        "Must be implemented for bulk DependentResource creation");
+    throw new IllegalStateException("Must be implemented for bulk DependentResource creation");
   }
 
-  public AbstractDependentResource<R, P> setResourceDiscriminator(
-      ResourceDiscriminator<R, P> resourceDiscriminator) {
-    if (resourceDiscriminator != null) {
-      this.resourceDiscriminator.add(resourceDiscriminator);
+  public void delete(P primary, Context<P> context) {
+    if (bulk) {
+      deleteBulkResourcesIfRequired(0, primary, context);
+    } else {
+      handleDelete(primary, context);
     }
-    return this;
   }
 
-  public ResourceDiscriminator<R, P> getResourceDiscriminator() {
-    if (this.resourceDiscriminator.isEmpty()) {
-      return null;
-    } else {
-      return this.resourceDiscriminator.get(0);
-    }
+  protected void handleDelete(P primary, Context<P> context) {
+    throw new IllegalStateException("delete method be implemented if Deleter trait is supported");
   }
 
-  protected int lastKnownBulkSize() {
-    return resourceDiscriminator.size();
+  public void setResourceDiscriminator(
+      ResourceDiscriminator<R, P> resourceDiscriminator) {
+    this.resourceDiscriminator = resourceDiscriminator;
   }
 
+  protected boolean isCreatable() {
+    return creatable;
+  }
+
+  protected boolean isUpdatable() {
+    return updatable;
+  }
+
+  protected boolean isBulk() {
+    return bulk;
+  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java

@@ -1,9 +1,11 @@
 package io.javaoperatorsdk.operator.processing.dependent;
 
+import java.util.Optional;
+
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
-import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
+import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
 
 /**
  * Manages dynamic number of resources created for a primary resource. Since the point of a bulk
@@ -30,6 +32,21 @@ public interface BulkDependentResource<R, P extends HasMetadata> extends Creator
    */
   void deleteBulkResourceWithIndex(P primary, R resource, int i, Context<P> context);
 
-  ResourceDiscriminator<R, P> getResourceDiscriminator(int index);
+  /**
+   * Determines whether the specified secondary resource matches the desired state with target index
+   * of a bulk resource as defined from the specified primary resource, given the specified
+   * {@link Context}.
+   *
+   * @param actualResource the resource we want to determine whether it's matching the desired state
+   * @param primary the primary resource from which the desired state is inferred
+   * @param context the context in which the resource is being matched
+   * @return a {@link Result} encapsulating whether the resource matched its desired state and this
+   *         associated state if it was computed as part of the matching process. Use the static
+   *         convenience methods ({@link Result#nonComputed(boolean)} and
+   *         {@link Result#computed(boolean, Object)})
+   */
+  Result<R> match(R actualResource, P primary, int index, Context<P> context);
+
+  Optional<R> getSecondaryResource(P primary, int index, Context<P> context);
 
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkUpdater.java

@@ -13,8 +13,12 @@
 public interface BulkUpdater<R, P extends HasMetadata> extends Updater<R, P> {
 
   default Matcher.Result<R> match(R actualResource, P primary, Context<P> context) {
-    throw new IllegalStateException();
+    if (!(this instanceof BulkDependentResource)) {
+      throw new IllegalStateException(
+          BulkUpdater.class.getSimpleName() + " interface should only be implemented by "
+              + BulkDependentResource.class.getSimpleName() + " implementations");
+    }
+    throw new IllegalStateException("This method should not be called from a "
+        + BulkDependentResource.class.getSimpleName() + " implementation");
   }
-
-  Matcher.Result<R> match(R actualResource, P primary, int index, Context<P> context);
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DesiredEqualsMatcher.java

@@ -16,10 +16,4 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
     var desired = abstractDependentResource.desired(primary, context);
     return Result.computed(actualResource.equals(desired), desired);
   }
-
-  @Override
-  public Result<R> match(R actualResource, P primary, int index, Context<P> context) {
-    var desired = abstractDependentResource.desired(primary, index, context);
-    return Result.computed(actualResource.equals(desired), desired);
-  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/Matcher.java

@@ -95,19 +95,4 @@ public Optional<T> computedDesired() {
    *         {@link Result#computed(boolean, Object)})
    */
   Result<R> match(R actualResource, P primary, Context<P> context);
-
-  /**
-   * Determines whether the specified secondary resource matches the desired state with target index
-   * of a bulk resource as defined from the specified primary resource, given the specified
-   * {@link Context}.
-   *
-   * @param actualResource the resource we want to determine whether it's matching the desired state
-   * @param primary the primary resource from which the desired state is inferred
-   * @param context the context in which the resource is being matched
-   * @return a {@link Result} encapsulating whether the resource matched its desired state and this
-   *         associated state if it was computed as part of the matching process. Use the static
-   *         convenience methods ({@link Result#nonComputed(boolean)} and
-   *         {@link Result#computed(boolean, Object)})
-   */
-  Result<R> match(R actualResource, P primary, int index, Context<P> context);
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/Updater.java

@@ -8,8 +8,4 @@ public interface Updater<R, P extends HasMetadata> {
   R update(R actual, R desired, P primary, Context<P> context);
 
   Result<R> match(R actualResource, P primary, Context<P> context);
-
-  default Result<R> match(R actualResource, P primary, int index, Context<P> context) {
-    throw new IllegalStateException("Implement this for bulk matching");
-  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java

@@ -24,42 +24,19 @@ private GenericKubernetesResourceMatcher(KubernetesDependentResource<R, P> depen
   static <R extends HasMetadata, P extends HasMetadata> Matcher<R, P> matcherFor(
       Class<R> resourceType, KubernetesDependentResource<R, P> dependentResource) {
     if (Secret.class.isAssignableFrom(resourceType)) {
-      return new Matcher<>() {
-        @Override
-        public Result<R> match(R actualResource, P primary, Context<P> context) {
-          final var desired = dependentResource.desired(primary, context);
-          return Result.computed(
-              ResourceComparators.compareSecretData((Secret) desired, (Secret) actualResource),
-              desired);
-        }
-
-        @Override
-        public Result<R> match(R actualResource, P primary, int index, Context<P> context) {
-          final var desired = dependentResource.desired(primary, index, context);
-          return Result.computed(
-              ResourceComparators.compareSecretData((Secret) desired, (Secret) actualResource),
-              desired);
-        }
+      return (actualResource, primary, context) -> {
+        final var desired = dependentResource.desired(primary, context);
+        return Result.computed(
+            ResourceComparators.compareSecretData((Secret) desired, (Secret) actualResource),
+            desired);
       };
     } else if (ConfigMap.class.isAssignableFrom(resourceType)) {
-      return new Matcher<>() {
-        @Override
-        public Result<R> match(R actualResource, P primary, Context<P> context) {
-          final var desired = dependentResource.desired(primary, context);
-          return Result.computed(
-              ResourceComparators.compareConfigMapData((ConfigMap) desired,
-                  (ConfigMap) actualResource),
-              desired);
-        }
-
-        @Override
-        public Result<R> match(R actualResource, P primary, int index, Context<P> context) {
-          final var desired = dependentResource.desired(primary, index, context);
-          return Result.computed(
-              ResourceComparators.compareConfigMapData((ConfigMap) desired,
-                  (ConfigMap) actualResource),
-              desired);
-        }
+      return (actualResource, primary, context) -> {
+        final var desired = dependentResource.desired(primary, context);
+        return Result.computed(
+            ResourceComparators.compareConfigMapData((ConfigMap) desired,
+                (ConfigMap) actualResource),
+            desired);
       };
     } else {
       return new GenericKubernetesResourceMatcher(dependentResource);
@@ -72,12 +49,6 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
     return match(desired, actualResource, false);
   }
 
-  @Override
-  public Result<R> match(R actualResource, P primary, int index, Context<P> context) {
-    var desired = dependentResource.desired(primary, index, context);
-    return match(desired, actualResource, false);
-  }
-
   public static <R extends HasMetadata> Result<R> match(
       R desired, R actualResource, boolean considerMetadata) {
     if (considerMetadata) {

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

@@ -135,23 +135,19 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
   }
 
   public Result<R> match(R actualResource, P primary, int index, Context<P> context) {
-    return matcher.match(actualResource, primary, index, context);
+    final var desired = desired(primary, index, context);
+    return GenericKubernetesResourceMatcher.match(desired, actualResource, false);
   }
 
-  public void delete(P primary, Context<P> context) {
-    if (bulk) {
-      deleteBulkResourcesIfRequired(0, lastKnownBulkSize(), primary, context);
-    } else {
-      var resource = getSecondaryResource(primary, context);
-      resource.ifPresent(r -> client.resource(r).delete());
-    }
+  protected void handleDelete(P primary, Context<P> context) {
+    var resource = getSecondaryResource(primary, context);
+    resource.ifPresent(r -> client.resource(r).delete());
   }
 
   public void deleteBulkResourceWithIndex(P primary, R resource, int i, Context<P> context) {
     client.resource(resource).delete();
   }
 
-  @SuppressWarnings("unchecked")
   protected Resource<R> prepare(R desired, P primary, String actionName) {
     log.debug("{} target resource with type: {}, with id: {}",
         actionName,
@@ -192,7 +188,7 @@ protected InformerEventSource<R, P> createEventSource(EventSourceContext<P> cont
   }
 
   private boolean useDefaultAnnotationsToIdentifyPrimary() {
-    return !(this instanceof SecondaryToPrimaryMapper) && !garbageCollected && creatable;
+    return !(this instanceof SecondaryToPrimaryMapper) && !garbageCollected && isCreatable();
   }
 
   private void addDefaultSecondaryToPrimaryMapperAnnotations(R desired, P primary) {