From 1f6cf2ccdfb5797b369fb43e39e064797165c17f Mon Sep 17 00:00:00 2001 From: Robert Young Date: Wed, 20 Nov 2024 22:13:57 +1300 Subject: [PATCH] Method signature mismatched with javadoc and implementation (#2587) * refactor: add unit tests for DefaultManagedDependentResourceContext Signed-off-by: Robert Young * fix: implementation of put should return value instead of Optional Signed-off-by: Chris Laprun * fix: update test name Signed-off-by: Robert Young --------- Signed-off-by: Robert Young Signed-off-by: Chris Laprun Co-authored-by: Chris Laprun --- ...efaultManagedDependentResourceContext.java | 25 ++++- .../ManagedDependentResourceContext.java | 15 ++- ...ltManagedDependentResourceContextTest.java | 105 ++++++++++++++++++ 3 files changed, 140 insertions(+), 5 deletions(-) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContextTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java index 61414468c8..84f7cfd03b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java +++ b/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 Optional get(Object key, Class expectedType) { @Override @SuppressWarnings("unchecked") public 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/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedDependentResourceContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedDependentResourceContext.java index 9c5b3dddb1..074f7776f4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedDependentResourceContext.java +++ b/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. * + *

+ * 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). + *

+ * * @param 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 put(Object key, T value); /** diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContextTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContextTest.java new file mode 100644 index 0000000000..79effe64af --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/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 actual = context.get("key", String.class); + assertThat(actual).isEmpty(); + } + + @Test + void get() { + context.put("key", "value"); + Optional actual = context.get("key", String.class); + assertThat(actual).contains("value"); + } + + @Test + void putNewValueOverwrites() { + context.put("key", "value"); + context.put("key", "valueB"); + Optional 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 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 actual = context.getWorkflowReconcileResult(); + assertThat(actual).containsSame(result); + } +}