Method signature mismatched with javadoc and implementation (#2587)

* refactor: add unit tests for DefaultManagedDependentResourceContext Signed-off-by: Robert Young <[email protected]> * fix: implementation of put should return value instead of Optional Signed-off-by: Chris Laprun <[email protected]> * fix: update test name Signed-off-by: Robert Young <[email protected]> --------- Signed-off-by: Robert Young <[email protected]> Signed-off-by: Chris Laprun <[email protected]> Co-authored-by: Chris Laprun <[email protected]>
operator-framework · Nov 20, 2024 · 1f6cf2c · 1f6cf2c
1 parent 9ad77ae
commit 1f6cf2c
Show file tree

Hide file tree

Showing 3 changed files with 140 additions and 5 deletions.
diff --git a/...sdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java b/...sdk/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 --git a/...peratorsdk/operator/api/reconciler/dependent/managed/ManagedDependentResourceContext.java b/...peratorsdk/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 --git a/...operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContextTest.java b/...operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContextTest.java
@@ -0,0 +1,105 @@
+package io.javaoperatorsdk.operator.api.reconciler.dependent.managed;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import org.junit.jupiter.api.Test;
+
+import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowReconcileResult;
+
+import static io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedDependentResourceContext.RECONCILE_RESULT_KEY;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class DefaultManagedDependentResourceContextTest {
+
+  private final ManagedDependentResourceContext context =
+      new DefaultManagedDependentResourceContext();
+
+  @Test
+  void getWhenEmpty() {
+    Optional<String> actual = context.get("key", String.class);
+    assertThat(actual).isEmpty();
+  }
+
+  @Test
+  void get() {
+    context.put("key", "value");
+    Optional<String> actual = context.get("key", String.class);
+    assertThat(actual).contains("value");
+  }
+
+  @Test
+  void putNewValueOverwrites() {
+    context.put("key", "value");
+    context.put("key", "valueB");
+    Optional<String> actual = context.get("key", String.class);
+    assertThat(actual).contains("valueB");
+  }
+
+  @Test
+  void putNewValueReturnsPriorValue() {
+    final var prior = "value";
+    context.put("key", prior);
+    String actual = context.put("key", "valueB");
+    assertThat(actual).isEqualTo(prior);
+  }
+
+  @Test
+  void putNewValueLogsWarningIfTypesDiffer() {
+    // 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
+  void putNullRemoves() {
+    context.put("key", "value");
+    context.put("key", null);
+    Optional<String> actual = context.get("key", String.class);
+    assertThat(actual).isEmpty();
+  }
+
+  @Test
+  void putNullReturnsPriorValue() {
+    context.put("key", "value");
+    String actual = context.put("key", null);
+    assertThat(actual).contains("value");
+  }
+
+  @Test
+  void getMandatory() {
+    context.put("key", "value");
+    String actual = context.getMandatory("key", String.class);
+    assertThat(actual).isEqualTo("value");
+  }
+
+  @Test
+  void getMandatoryWhenEmpty() {
+    assertThatThrownBy(() -> {
+      context.getMandatory("key", String.class);
+    }).isInstanceOf(IllegalStateException.class)
+        .hasMessage(
+            "Mandatory attribute (key: key, type: java.lang.String) is missing or not of the expected type");
+  }
+
+  @Test
+  void getWorkflowReconcileResult() {
+    WorkflowReconcileResult result =
+        new WorkflowReconcileResult(List.of(), List.of(), Map.of(), Map.of());
+    context.put(RECONCILE_RESULT_KEY, result);
+    Optional<WorkflowReconcileResult> actual = context.getWorkflowReconcileResult();
+    assertThat(actual).containsSame(result);
+  }
+}