fix: implementation of put should return value instead of Optional

Signed-off-by: Chris Laprun <[email protected]>

Author: metacosm

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java

@@ -3,11 +3,16 @@
 import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowCleanupResult;
 import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowReconcileResult;
 
 @SuppressWarnings("rawtypes")
 public class DefaultManagedDependentResourceContext implements ManagedDependentResourceContext {
+  private static final Logger log =
+      LoggerFactory.getLogger(DefaultManagedDependentResourceContext.class);
   public static final Object RECONCILE_RESULT_KEY = new Object();
   public static final Object CLEANUP_RESULT_KEY = new Object();
   private final ConcurrentHashMap attributes = new ConcurrentHashMap();
@@ -21,18 +26,27 @@ public <T> Optional<T> get(Object key, Class<T> expectedType) {
 
   @Override
   @SuppressWarnings("unchecked")
-  @Deprecated(forRemoval = true)
   public <T> T put(Object key, T value) {
-    return (T) putOrRemove(key, value);
-  }
-
-  @Override
-  @SuppressWarnings("unchecked")
-  public <T> Optional<T> putOrRemove(Object key, T value) {
+    Object previous;
     if (value == null) {
-      return (Optional<T>) Optional.ofNullable(attributes.remove(key));
+      return (T) attributes.remove(key);
+    } else {
+      previous = attributes.put(key, value);
     }
-    return (Optional<T>) Optional.ofNullable(attributes.put(key, value));
+
+    if (previous != null && !previous.getClass().isAssignableFrom(value.getClass())) {
+      logWarning("Previous value (" + previous +
+          ") for key (" + key +
+          ") was not of type " + value.getClass() +
+          ". This might indicate an issue in your code. If not, use put(" + key +
+          ", null) first to remove the previous value.");
+    }
+    return (T) previous;
+  }
+
+  // only for testing purposes
+  void logWarning(String message) {
+    log.warn(message);
   }
 
   @Override

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedDependentResourceContext.java

@@ -30,31 +30,24 @@ public interface ManagedDependentResourceContext {
    * the semantics of this operation is defined as removing the mapping associated with the
    * specified key.
    *
-   * @param <T> object type
-   * @param key the key identifying which contextual object to add or remove from the context
-   * @param value the value to add to the context or {@code null} to remove an existing entry
-   *        associated with the specified key
-   * @return an Optional containing the previous value associated with the key or
-   *         {@link Optional#empty()} if none existed
-   * @deprecated use {@link #putOrRemove(Object, Object)} instead
-   */
-  @SuppressWarnings("unchecked")
-  @Deprecated(forRemoval = true)
-  <T> T put(Object key, T value);
-
-  /**
-   * Associates the specified contextual value to the specified key. If the value is {@code null},
-   * the semantics of this operation is defined as removing the mapping associated with the
-   * specified key.
+   * <p>
+   * Note that, while implementations shouldn't throw a {@link ClassCastException} when the new
+   * value type differs from the type of the existing value, calling sites might encounter such
+   * exceptions if they bind the return value to a specific type. Users are either expected to
+   * disregard the return value (most common case) or "reset" the value type associated with the
+   * specified key by first calling {@code put(key, null)} if they want to ensure some level of type
+   * safety in their code (where attempting to store values of different types under the same key
+   * might be indicative of an issue).
+   * </p>
    *
    * @param <T> object type
    * @param key the key identifying which contextual object to add or remove from the context
    * @param value the value to add to the context or {@code null} to remove an existing entry
    *        associated with the specified key
-   * @return an Optional containing the previous value associated with the key or
-   *         {@link Optional#empty()} if none existed
+   * @return the previous value if one was associated with the specified key, {@code null}
+   *         otherwise.
    */
-  <T> Optional<T> putOrRemove(Object key, T value);
+  <T> T put(Object key, T value);
 
   /**
    * Retrieves the value associated with the key or fail with an exception if none exists.

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

@@ -93,7 +93,7 @@ public WorkflowReconcileResult reconcile(P primary, Context<P> context) {
         new WorkflowReconcileExecutor<>(this, primary, context);
     var result = workflowReconcileExecutor.reconcile();
     context.managedDependentResourceContext()
-        .putOrRemove(DefaultManagedDependentResourceContext.RECONCILE_RESULT_KEY, result);
+        .put(DefaultManagedDependentResourceContext.RECONCILE_RESULT_KEY, result);
     if (throwExceptionAutomatically) {
       result.throwAggregateExceptionIfErrorsPresent();
     }
@@ -106,7 +106,7 @@ public WorkflowCleanupResult cleanup(P primary, Context<P> context) {
         new WorkflowCleanupExecutor<>(this, primary, context);
     var result = workflowCleanupExecutor.cleanup();
     context.managedDependentResourceContext()
-        .putOrRemove(DefaultManagedDependentResourceContext.CLEANUP_RESULT_KEY, result);
+        .put(DefaultManagedDependentResourceContext.CLEANUP_RESULT_KEY, result);
     if (throwExceptionAutomatically) {
       result.throwAggregateExceptionIfErrorsPresent();
     }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContextTest.java

@@ -14,7 +14,8 @@
 
 class DefaultManagedDependentResourceContextTest {
 
-  private ManagedDependentResourceContext context = new DefaultManagedDependentResourceContext();
+  private final ManagedDependentResourceContext context =
+      new DefaultManagedDependentResourceContext();
 
   @Test
   void getWhenEmpty() {
@@ -37,26 +38,29 @@ void putNewValueOverwrites() {
     assertThat(actual).contains("valueB");
   }
 
-  @Test
-  void putOrRemoveNewValueOverwrites() {
-    context.putOrRemove("key", "value");
-    context.putOrRemove("key", "valueB");
-    Optional<String> actual = context.get("key", String.class);
-    assertThat(actual).contains("valueB");
-  }
-
   @Test
   void putNewValueReturnsPriorValue() {
-    context.put("key", "value");
-    Optional<String> actual = (Optional<String>) (Object) context.put("key", "valueB");
-    assertThat(actual).contains("value");
+    final var prior = "value";
+    context.put("key", prior);
+    String actual = context.put("key", "valueB");
+    assertThat(actual).isEqualTo(prior);
   }
 
   @Test
-  void putOrRemoveNewValueReturnsPriorValue() {
-    context.put("key", "value");
-    Optional<String> actual = context.putOrRemove("key", "valueB");
-    assertThat(actual).contains("value");
+  void putNewValueThrowsExceptionIfTypesDiffer() {
+    // to check that we properly log things without setting up a complex fixture
+    final String[] messages = new String[1];
+    var context = new DefaultManagedDependentResourceContext() {
+      @Override
+      void logWarning(String message) {
+        messages[0] = message;
+      }
+    };
+    final var prior = "value";
+    final var key = "key";
+    context.put(key, prior);
+    context.put(key, 10);
+    assertThat(messages[0]).contains(key).contains(prior).contains("put(" + key + ", null)");
   }
 
   @Test
@@ -67,25 +71,11 @@ void putNullRemoves() {
     assertThat(actual).isEmpty();
   }
 
-  @Test
-  void putOrRemoveNullRemoves() {
-    context.putOrRemove("key", "value");
-    context.putOrRemove("key", null);
-    Optional<String> actual = context.get("key", String.class);
-    assertThat(actual).isEmpty();
-  }
 
   @Test
   void putNullReturnsPriorValue() {
     context.put("key", "value");
-    Optional<String> actual = context.put("key", null);
-    assertThat(actual).contains("value");
-  }
-
-  @Test
-  void putOrRemoveNullReturnsPriorValue() {
-    context.putOrRemove("key", "value");
-    Optional<String> actual = context.putOrRemove("key", null);
+    String actual = context.put("key", null);
     assertThat(actual).contains("value");
   }
 
@@ -113,5 +103,4 @@ void getWorkflowReconcileResult() {
     Optional<WorkflowReconcileResult> actual = context.getWorkflowReconcileResult();
     assertThat(actual).containsSame(result);
   }
-
 }