fix: implementation of put should return value instead of Optional

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

Author: metacosm

Diff for: 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();
@@ -22,10 +27,26 @@ public <T> Optional<T> get(Object key, Class<T> expectedType) {
   @Override
   @SuppressWarnings("unchecked")
   public <T> T put(Object key, T value) {
+    Object previous;
     if (value == null) {
-      return (T) Optional.ofNullable(attributes.remove(key));
+      return (T) attributes.remove(key);
+    } else {
+      previous = 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) Optional.ofNullable(attributes.put(key, value));
+    return (T) previous;
+  }
+
+  // only for testing purposes
+  void logWarning(String message) {
+    log.warn(message);
   }
 
   @Override

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

@@ -30,14 +30,23 @@ public interface ManagedDependentResourceContext {
    * 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.
    */
-  @SuppressWarnings("unchecked")
   <T> T put(Object key, T value);
 
   /**

Diff for: 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() {
@@ -39,9 +40,27 @@ void putNewValueOverwrites() {
 
   @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 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
@@ -55,7 +74,7 @@ void putNullRemoves() {
   @Test
   void putNullReturnsPriorValue() {
     context.put("key", "value");
-    Optional<String> actual = context.put("key", null);
+    String actual = context.put("key", null);
     assertThat(actual).contains("value");
   }