refactor: make it easier to initialize mapper & cloner in subclasses (#1403)

* feat: use fabric8 client json mapper by default (#1382)

Also remove constants to avoid mismatched configurations

* refactor: make it easier to initialize mapper & cloner in subclasses

Co-authored-by: Attila Mészáros <[email protected]>

Author: metacosm

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

@@ -9,13 +9,40 @@
 import io.javaoperatorsdk.operator.ReconcilerUtils;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+
 @SuppressWarnings("rawtypes")
 public class AbstractConfigurationService implements ConfigurationService {
   private final Map<String, ControllerConfiguration> configurations = new ConcurrentHashMap<>();
   private final Version version;
+  private Cloner cloner;
+  private ObjectMapper mapper;
 
   public AbstractConfigurationService(Version version) {
+    this(version, null, null);
+  }
+
+  public AbstractConfigurationService(Version version, Cloner cloner) {
+    this(version, cloner, null);
+  }
+
+  public AbstractConfigurationService(Version version, Cloner cloner, ObjectMapper mapper) {
     this.version = version;
+    init(cloner, mapper);
+  }
+
+  /**
+   * Subclasses can call this method to more easily initialize the {@link Cloner} and
+   * {@link ObjectMapper} associated with this ConfigurationService implementation. This is useful
+   * in situations where the cloner depends on a mapper that might require additional configuration
+   * steps before it's ready to be used.
+   *
+   * @param cloner the {@link Cloner} instance to be used
+   * @param mapper the {@link ObjectMapper} instance to be used
+   */
+  protected void init(Cloner cloner, ObjectMapper mapper) {
+    this.cloner = cloner != null ? cloner : ConfigurationService.super.getResourceCloner();
+    this.mapper = mapper != null ? mapper : ConfigurationService.super.getObjectMapper();
   }
 
   protected <R extends HasMetadata> void register(ControllerConfiguration<R> config) {
@@ -77,10 +104,12 @@ protected <R extends HasMetadata> String keyFor(Reconciler<R> reconciler) {
     return ReconcilerUtils.getNameFor(reconciler);
   }
 
+  @SuppressWarnings("unused")
   protected ControllerConfiguration getFor(String reconcilerName) {
     return configurations.get(reconcilerName);
   }
 
+  @SuppressWarnings("unused")
   protected Stream<ControllerConfiguration> controllerConfigurations() {
     return configurations.values().stream();
   }
@@ -94,4 +123,14 @@ public Set<String> getKnownReconcilerNames() {
   public Version getVersion() {
     return version;
   }
+
+  @Override
+  public Cloner getResourceCloner() {
+    return cloner;
+  }
+
+  @Override
+  public ObjectMapper getObjectMapper() {
+    return mapper;
+  }
 }

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

@@ -6,6 +6,8 @@
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+
 public class BaseConfigurationService extends AbstractConfigurationService {
 
   private static final String LOGGER_NAME = "Default ConfigurationService implementation";
@@ -15,6 +17,10 @@ public BaseConfigurationService(Version version) {
     super(version);
   }
 
+  public BaseConfigurationService(Version version, Cloner cloner, ObjectMapper mapper) {
+    super(version, cloner, mapper);
+  }
+
   public BaseConfigurationService() {
     this(Utils.loadFromProperties());
   }

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

@@ -7,6 +7,7 @@
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.Config;
 import io.fabric8.kubernetes.client.CustomResource;
+import io.fabric8.kubernetes.client.utils.Serialization;
 import io.javaoperatorsdk.operator.api.monitoring.Metrics;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory;
@@ -17,20 +18,6 @@
 /** An interface from which to retrieve configuration information. */
 public interface ConfigurationService {
 
-  ObjectMapper OBJECT_MAPPER = new ObjectMapper();
-
-  Cloner DEFAULT_CLONER = new Cloner() {
-    @SuppressWarnings("unchecked")
-    @Override
-    public HasMetadata clone(HasMetadata object) {
-      try {
-        return OBJECT_MAPPER.readValue(OBJECT_MAPPER.writeValueAsString(object), object.getClass());
-      } catch (JsonProcessingException e) {
-        throw new IllegalStateException(e);
-      }
-    }
-  };
-
   /**
    * Retrieves the configuration associated with the specified reconciler
    *
@@ -93,12 +80,25 @@ default int concurrentReconciliationThreads() {
   }
 
   /**
-   * Used to clone custom resources.
+   * Used to clone custom resources. It is strongly suggested that implementors override this method
+   * since the default implementation creates a new {@link Cloner} instance each time this method is
+   * called.
    *
-   * @return the ObjectMapper to use
+   * @return the configured {@link Cloner}
    */
   default Cloner getResourceCloner() {
-    return DEFAULT_CLONER;
+    return new Cloner() {
+      @SuppressWarnings("unchecked")
+      @Override
+      public HasMetadata clone(HasMetadata object) {
+        try {
+          final var mapper = getObjectMapper();
+          return mapper.readValue(mapper.writeValueAsString(object), object.getClass());
+        } catch (JsonProcessingException e) {
+          throw new IllegalStateException(e);
+        }
+      }
+    };
   }
 
   int DEFAULT_TERMINATION_TIMEOUT_SECONDS = 10;
@@ -126,7 +126,7 @@ default boolean closeClientOnStop() {
   }
 
   default ObjectMapper getObjectMapper() {
-    return OBJECT_MAPPER;
+    return Serialization.jsonMapper();
   }
 
   default DependentResourceFactory dependentResourceFactory() {

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

@@ -81,7 +81,7 @@ public ConfigurationServiceOverrider withObjectMapper(ObjectMapper objectMapper)
   }
 
   public ConfigurationService build() {
-    return new BaseConfigurationService(original.getVersion()) {
+    return new BaseConfigurationService(original.getVersion(), cloner, objectMapper) {
       @Override
       public Set<String> getKnownReconcilerNames() {
         return original.getKnownReconcilerNames();
@@ -102,11 +102,6 @@ public int concurrentReconciliationThreads() {
         return threadNumber;
       }
 
-      @Override
-      public Cloner getResourceCloner() {
-        return cloner;
-      }
-
       @Override
       public int getTerminationTimeoutSeconds() {
         return timeoutSeconds;
@@ -130,11 +125,6 @@ public ExecutorService getExecutorService() {
           return super.getExecutorService();
         }
       }
-
-      @Override
-      public ObjectMapper getObjectMapper() {
-        return objectMapper;
-      }
     };
   }
 

operator-framework/src/test/java/io/javaoperatorsdk/operator/StandaloneDependentResourceIT.java

@@ -7,7 +7,7 @@
 
 import io.fabric8.kubernetes.api.model.ObjectMeta;
 import io.fabric8.kubernetes.api.model.apps.Deployment;
-import io.javaoperatorsdk.operator.api.config.ConfigurationService;
+import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
 import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
 import io.javaoperatorsdk.operator.sample.standalonedependent.StandaloneDependentTestCustomResource;
 import io.javaoperatorsdk.operator.sample.standalonedependent.StandaloneDependentTestCustomResourceSpec;
@@ -52,7 +52,7 @@ void executeUpdateForTestingCacheUpdateForGetResource() {
 
     awaitForDeploymentReadyReplicas(1);
 
-    var clonedCr = ConfigurationService.DEFAULT_CLONER.clone(createdCR);
+    var clonedCr = ConfigurationServiceProvider.instance().getResourceCloner().clone(createdCR);
     clonedCr.getSpec().setReplicaCount(2);
     operator.replace(StandaloneDependentTestCustomResource.class, clonedCr);