refactor: clean some reported code smells (#1660)

Author: metacosm

micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java

@@ -55,6 +55,7 @@ public void controllerRegistered(Controller<?> controller) {
     gauges.put(controllerQueueName, controllerQueueSize);
   }
 
+  @Override
   public <T> T timeControllerExecution(ControllerExecution<T> execution) {
     final var name = execution.controllerName();
     final var execName = PREFIX + "controllers.execution." + execution.name();
@@ -65,7 +66,7 @@ public <T> T timeControllerExecution(ControllerExecution<T> execution) {
         "controller", name,
         "resource.name", resourceID.getName(),
         "resource.namespace", resourceID.getNamespace().orElse(""),
-        "resource.scope", resourceID.getNamespace().isPresent() ? "namespace" : "cluster"));
+        "resource.scope", getScope(resourceID)));
     final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY);
     if (gvk != null) {
       tags.addAll(List.of(
@@ -101,6 +102,11 @@ public <T> T timeControllerExecution(ControllerExecution<T> execution) {
     }
   }
 
+  private static String getScope(ResourceID resourceID) {
+    return resourceID.getNamespace().isPresent() ? "namespace" : "cluster";
+  }
+
+  @Override
   public void receivedEvent(Event event, Map<String, Object> metadata) {
     incrementCounter(event.getRelatedCustomResourceID(), "events.received",
         metadata,
@@ -151,6 +157,7 @@ public void reconciliationExecutionFinished(HasMetadata resource, Map<String, Ob
     controllerQueueSize.decrementAndGet();
   }
 
+  @Override
   public void failedReconciliation(HasMetadata resource, Exception exception,
       Map<String, Object> metadata) {
     var cause = exception.getCause();
@@ -164,6 +171,7 @@ public void failedReconciliation(HasMetadata resource, Exception exception,
         cause.getClass().getSimpleName());
   }
 
+  @Override
   public <T extends Map<?, ?>> T monitorSizeOf(T map, String name) {
     return registry.gaugeMapSize(PREFIX + name + ".size", Collections.emptyList(), map);
   }
@@ -183,7 +191,7 @@ private void incrementCounter(ResourceID id, String counterName, Map<String, Obj
     tags.addAll(List.of(
         "name", id.getName(),
         "namespace", id.getNamespace().orElse(""),
-        "scope", id.getNamespace().isPresent() ? "namespace" : "cluster"));
+        "scope", getScope(id)));
     if (additionalTagsNb > 0) {
       tags.addAll(List.of(additionalTags));
     }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/CustomResourceUtils.java

@@ -6,7 +6,9 @@
 import io.fabric8.kubernetes.api.model.Namespaced;
 import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinition;
 
-public abstract class CustomResourceUtils {
+public class CustomResourceUtils {
+
+  private CustomResourceUtils() {}
 
   /**
    * Applies internal validations that may not be handled by the fabric8 client.

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java

@@ -13,7 +13,12 @@
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.fabric8.kubernetes.client.KubernetesClientBuilder;
 import io.fabric8.kubernetes.client.Version;
-import io.javaoperatorsdk.operator.api.config.*;
+import io.javaoperatorsdk.operator.api.config.ConfigurationService;
+import io.javaoperatorsdk.operator.api.config.ConfigurationServiceOverrider;
+import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.config.ControllerConfigurationOverrider;
+import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 import io.javaoperatorsdk.operator.processing.Controller;
 import io.javaoperatorsdk.operator.processing.LifecycleAware;
@@ -112,9 +117,8 @@ public synchronized void start() {
       leaderElectionManager.start();
       started = true;
     } catch (Exception e) {
-      log.error("Error starting operator", e);
       stop();
-      throw e;
+      throw new OperatorException("Error starting operator", e);
     }
   }
 

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java

@@ -132,12 +132,11 @@ public static void handleKubernetesClientException(Exception e, String resourceT
 
     if (e instanceof KubernetesClientException) {
       KubernetesClientException ke = (KubernetesClientException) e;
-      if (404 == ke.getCode()) {
-        // only throw MissingCRDException if the 404 error occurs on the target CRD
-        if (resourceTypeName.equals(ke.getFullResourceName())
-            || matchesResourceType(resourceTypeName, ke)) {
-          throw new MissingCRDException(resourceTypeName, ke.getVersion(), e.getMessage(), e);
-        }
+      // only throw MissingCRDException if the 404 error occurs on the target CRD
+      if (404 == ke.getCode() &&
+          (resourceTypeName.equals(ke.getFullResourceName())
+              || matchesResourceType(resourceTypeName, ke))) {
+        throw new MissingCRDException(resourceTypeName, ke.getVersion(), e.getMessage(), e);
       }
     }
   }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResolvedControllerConfiguration.java

@@ -157,6 +157,9 @@ public ResourceEventFilter<P> getEventFilter() {
     return eventFilter;
   }
 
+  /**
+   * @deprecated Use {@link OnAddFilter}, {@link OnUpdateFilter} and {@link GenericFilter} instead
+   */
   @Deprecated(forRemoval = true)
   protected void setEventFilter(ResourceEventFilter<P> eventFilter) {
     this.eventFilter = eventFilter;

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java

@@ -24,6 +24,8 @@ public class Utils {
   private static final Logger log = LoggerFactory.getLogger(Utils.class);
   public static final String CHECK_CRD_ENV_KEY = "JAVA_OPERATOR_SDK_CHECK_CRD";
   public static final String DEBUG_THREAD_POOL_ENV_KEY = "JAVA_OPERATOR_SDK_DEBUG_THREAD_POOL";
+  public static final String GENERIC_PARAMETER_TYPE_ERROR_PREFIX =
+      "Couldn't retrieve generic parameter type from ";
 
   /**
    * Attempts to load version information from a properties file produced at build time, currently
@@ -102,7 +104,7 @@ public static Class<?> getFirstTypeArgumentFromExtendedClass(Class<?> clazz) {
       Type type = clazz.getGenericSuperclass();
       return (Class<?>) ((ParameterizedType) type).getActualTypeArguments()[0];
     } catch (Exception e) {
-      throw new RuntimeException("Couldn't retrieve generic parameter type from "
+      throw new RuntimeException(GENERIC_PARAMETER_TYPE_ERROR_PREFIX
           + clazz.getSimpleName()
           + " because it doesn't extend a class that is parameterized with the type we want to retrieve",
           e);
@@ -118,49 +120,55 @@ public static Class<?> getTypeArgumentFromInterfaceByIndex(Class<?> clazz,
       Class<?> expectedImplementedInterface, int index) {
     if (expectedImplementedInterface.isAssignableFrom(clazz)) {
       final var genericInterfaces = clazz.getGenericInterfaces();
-      Optional<? extends Class<?>> target = Optional.empty();
-      if (genericInterfaces.length > 0) {
-        // try to find the target interface among them
-        target = Arrays.stream(genericInterfaces)
-            .filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName())
-                && type instanceof ParameterizedType)
-            .map(ParameterizedType.class::cast)
-            .findFirst()
-            .map(t -> {
-              final Type argument = t.getActualTypeArguments()[index];
-              if (argument instanceof Class) {
-                return (Class<?>) argument;
-              }
-              // account for the case where the argument itself has parameters, which we will ignore
-              // and just return the raw type
-              if (argument instanceof ParameterizedType) {
-                final var rawType = ((ParameterizedType) argument).getRawType();
-                if (rawType instanceof Class) {
-                  return (Class<?>) rawType;
-                }
-              }
-              throw new IllegalArgumentException(clazz.getSimpleName() + " implements "
-                  + expectedImplementedInterface.getSimpleName()
-                  + " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: "
-                  + argument);
-            });
-      }
 
+      var target = extractType(clazz, expectedImplementedInterface, index, genericInterfaces);
       if (target.isPresent()) {
         return target.get();
       }
 
-      // try the parent
+      // try the parent if we didn't find a parameter type on the current class
       var parent = clazz.getSuperclass();
       if (!Object.class.equals(parent)) {
         return getTypeArgumentFromInterfaceByIndex(parent, expectedImplementedInterface, index);
       }
     }
-    throw new IllegalArgumentException("Couldn't retrieve generic parameter type from "
+    throw new IllegalArgumentException(GENERIC_PARAMETER_TYPE_ERROR_PREFIX
         + clazz.getSimpleName() + " because it or its superclasses don't implement "
         + expectedImplementedInterface.getSimpleName());
   }
 
+  private static Optional<? extends Class<?>> extractType(Class<?> clazz,
+      Class<?> expectedImplementedInterface, int index, Type[] genericInterfaces) {
+    Optional<? extends Class<?>> target = Optional.empty();
+    if (genericInterfaces.length > 0) {
+      // try to find the target interface among them
+      target = Arrays.stream(genericInterfaces)
+          .filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName())
+              && type instanceof ParameterizedType)
+          .map(ParameterizedType.class::cast)
+          .findFirst()
+          .map(t -> {
+            final Type argument = t.getActualTypeArguments()[index];
+            if (argument instanceof Class) {
+              return (Class<?>) argument;
+            }
+            // account for the case where the argument itself has parameters, which we will ignore
+            // and just return the raw type
+            if (argument instanceof ParameterizedType) {
+              final var rawType = ((ParameterizedType) argument).getRawType();
+              if (rawType instanceof Class) {
+                return (Class<?>) rawType;
+              }
+            }
+            throw new IllegalArgumentException(clazz.getSimpleName() + " implements "
+                + expectedImplementedInterface.getSimpleName()
+                + " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: "
+                + argument);
+          });
+    }
+    return target;
+  }
+
   public static Class<?> getFirstTypeArgumentFromSuperClassOrInterface(Class<?> clazz,
       Class<?> expectedImplementedInterface) {
     // first check super class if it exists
@@ -183,7 +191,7 @@ public static Class<?> getFirstTypeArgumentFromSuperClassOrInterface(Class<?> cl
       return getFirstTypeArgumentFromInterface(clazz, expectedImplementedInterface);
     } catch (Exception e) {
       throw new OperatorException(
-          "Couldn't retrieve generic parameter type from " + clazz.getSimpleName(), e);
+          GENERIC_PARAMETER_TYPE_ERROR_PREFIX + clazz.getSimpleName(), e);
     }
   }
 

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java

@@ -35,6 +35,9 @@ default void controllerRegistered(Controller<?> controller) {}
    */
   default void receivedEvent(Event event, Map<String, Object> metadata) {}
 
+  /**
+   * @deprecated Use {@link #reconcileCustomResource(HasMetadata, RetryInfo, Map)} instead
+   */
   @Deprecated(forRemoval = true)
   default void reconcileCustomResource(ResourceID resourceID, RetryInfo retryInfo,
       Map<String, Object> metadata) {}
@@ -51,6 +54,9 @@ default void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfo,
     reconcileCustomResource(ResourceID.fromResource(resource), retryInfo, metadata);
   }
 
+  /**
+   * @deprecated Use {@link #failedReconciliation(HasMetadata, Exception, Map)} instead
+   */
   @Deprecated(forRemoval = true)
   default void failedReconciliation(ResourceID resourceID, Exception exception,
       Map<String, Object> metadata) {}
@@ -102,6 +108,9 @@ default void finishedReconciliation(ResourceID resourceID) {
     finishedReconciliation(resourceID, Collections.emptyMap());
   }
 
+  /**
+   * @deprecated Use {@link #finishedReconciliation(HasMetadata, Map)} instead
+   */
   @Deprecated(forRemoval = true)
   default void finishedReconciliation(ResourceID resourceID, Map<String, Object> metadata) {}
 

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

@@ -22,18 +22,18 @@ public class DefaultManagedWorkflow<P extends HasMetadata> implements ManagedWor
 
   private final Set<String> topLevelResources;
   private final Set<String> bottomLevelResources;
-  private final List<DependentResourceSpec<?, ?>> orderedSpecs;
+  private final List<DependentResourceSpec> orderedSpecs;
   private final boolean hasCleaner;
 
-  protected DefaultManagedWorkflow(List<DependentResourceSpec<?, ?>> orderedSpecs,
+  protected DefaultManagedWorkflow(List<DependentResourceSpec> orderedSpecs,
       boolean hasCleaner) {
     this.hasCleaner = hasCleaner;
     topLevelResources = new HashSet<>(orderedSpecs.size());
     bottomLevelResources = orderedSpecs.stream()
         .map(DependentResourceSpec::getName)
         .collect(Collectors.toSet());
     this.orderedSpecs = orderedSpecs;
-    orderedSpecs.forEach(spec -> {
+    for (DependentResourceSpec<?, ?> spec : orderedSpecs) {
       // add cycle detection?
       if (spec.getDependsOn().isEmpty()) {
         topLevelResources.add(spec.getName());
@@ -42,12 +42,12 @@ protected DefaultManagedWorkflow(List<DependentResourceSpec<?, ?>> orderedSpecs,
           bottomLevelResources.remove(dependsOn);
         }
       }
-    });
+    }
   }
 
   @Override
   @SuppressWarnings("unused")
-  public List<DependentResourceSpec<?, ?>> getOrderedSpecs() {
+  public List<DependentResourceSpec> getOrderedSpecs() {
     return orderedSpecs;
   }
 

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

@@ -32,7 +32,7 @@ public DependentResourceNode(String name, Condition<R, P> reconcilePrecondition,
     this.dependentResource = dependentResource;
   }
 
-  public List<? extends DependentResourceNode> getDependsOn() {
+  public List<DependentResourceNode> getDependsOn() {
     return dependsOn;
   }
 

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

@@ -10,8 +10,8 @@
 
 public interface ManagedWorkflow<P extends HasMetadata> {
 
-  @SuppressWarnings("unused")
-  default List<DependentResourceSpec<?, ?>> getOrderedSpecs() {
+  @SuppressWarnings({"unused", "rawtypes"})
+  default List<DependentResourceSpec> getOrderedSpecs() {
     return Collections.emptyList();
   }
 

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

@@ -64,12 +64,11 @@ <P extends HasMetadata> DefaultManagedWorkflow<P> createAsDefault(
    * @return top-bottom ordered resources that can be added safely to workflow
    * @throws OperatorException if there is a cycle in the dependencies
    */
-  private List<DependentResourceSpec<?, ?>> orderAndDetectCycles(
+  private List<DependentResourceSpec> orderAndDetectCycles(
       List<DependentResourceSpec> dependentResourceSpecs, boolean[] cleanerHolder) {
 
     final var drInfosByName = createDRInfos(dependentResourceSpecs);
-    final var orderedSpecs =
-        new ArrayList<DependentResourceSpec<?, ?>>(dependentResourceSpecs.size());
+    final var orderedSpecs = new ArrayList<DependentResourceSpec>(dependentResourceSpecs.size());
     final var alreadyVisited = new HashSet<String>();
     var toVisit = getTopDependentResources(dependentResourceSpecs);
 
@@ -108,7 +107,7 @@ <P extends HasMetadata> DefaultManagedWorkflow<P> createAsDefault(
    * @return top-bottom ordered resources that can be added safely to workflow
    * @throws OperatorException if there is a cycle in the dependencies
    */
-  public List<DependentResourceSpec<?, ?>> orderAndDetectCycles(
+  public List<DependentResourceSpec> orderAndDetectCycles(
       List<DependentResourceSpec> dependentResourceSpecs) {
     return orderAndDetectCycles(dependentResourceSpecs, null);
   }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ResourceState.java

@@ -106,6 +106,9 @@ public void unMarkEventReceived() {
         throw new IllegalStateException("Cannot unmark processed marked for deletion.");
       case DELETE_EVENT_PRESENT:
         throw new IllegalStateException("Cannot unmark delete event.");
+      case NO_EVENT_PRESENT:
+        // do nothing
+        break;
     }
   }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSource.java

@@ -94,6 +94,8 @@ private boolean isAcceptedByFilters(ResourceAction action, T resource, T oldReso
         return onAddFilter == null || onAddFilter.accept(resource);
       case UPDATED:
         return onUpdateFilter.accept(resource, oldResource);
+      case DELETED:
+        throw new IllegalStateException("Should not be called with " + action);
     }
     return true;
   }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java

@@ -94,10 +94,10 @@ public void start() throws OperatorException {
       }
 
     } catch (Exception e) {
-      log.error("Couldn't start informer for " + versionedFullResourceName() + " resources", e);
       ReconcilerUtils.handleKubernetesClientException(e,
           HasMetadata.getFullResourceName(informer.getApiTypeClass()));
-      throw e;
+      throw new OperatorException(
+          "Couldn't start informer for " + versionedFullResourceName() + " resources", e);
     }
   }
 

sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java

@@ -30,8 +30,6 @@ public class MySQLSchemaReconciler
 
   static final Logger log = LoggerFactory.getLogger(MySQLSchemaReconciler.class);
 
-  public MySQLSchemaReconciler() {}
-
 
   @Override
   public UpdateControl<MySQLSchema> reconcile(MySQLSchema schema, Context<MySQLSchema> context) {