From 13d58071153a1c8f2b0b2c1b219a9aafabf3e62a Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 25 Apr 2022 10:02:00 +0200 Subject: [PATCH 1/3] feat: throw exception if desired is null --- .../dependent/AbstractDependentResource.java | 9 ++ .../dependent/DependentResourceException.java | 20 ++++ .../AbstractDependentResourceTest.java | 106 ++++++++++++++++++ 3 files changed, 135 insertions(+) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceException.java create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 8611950e04..3bbfe561d0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -33,6 +33,7 @@ public ReconcileResult reconcile(P primary, Context

context) { if (maybeActual.isEmpty()) { if (creatable) { var desired = desired(primary, context); + throwIfNull(desired, primary); logForOperation("Creating", primary, desired); var createdResource = handleCreate(desired, primary, context); return ReconcileResult.resourceCreated(createdResource); @@ -43,6 +44,7 @@ public ReconcileResult reconcile(P primary, Context

context) { final var match = updater.match(actual, primary, context); if (!match.matched()) { final var desired = match.computedDesired().orElse(desired(primary, context)); + throwIfNull(desired, primary); logForOperation("Updating", primary, desired); var updatedResource = handleUpdate(actual, desired, primary, context); return ReconcileResult.resourceUpdated(updatedResource); @@ -59,6 +61,13 @@ public ReconcileResult reconcile(P primary, Context

context) { return ReconcileResult.noOperation(maybeActual.orElse(null)); } + private void throwIfNull(R desired, P primary) { + if (desired == null) { + throw new DependentResourceException( + "Desired cannot be null. Primary ID: " + ResourceID.fromResource(primary)); + } + } + private void logForOperation(String operation, P primary, R desired) { final var desiredDesc = desired instanceof HasMetadata ? "'" + ((HasMetadata) desired).getMetadata().getName() + "' " diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceException.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceException.java new file mode 100644 index 0000000000..a69eb1e0a8 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceException.java @@ -0,0 +1,20 @@ +package io.javaoperatorsdk.operator.processing.dependent; + +import io.javaoperatorsdk.operator.OperatorException; + +public class DependentResourceException extends OperatorException { + + public DependentResourceException() {} + + public DependentResourceException(String message) { + super(message); + } + + public DependentResourceException(Throwable cause) { + super(cause); + } + + public DependentResourceException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java new file mode 100644 index 0000000000..382f138468 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java @@ -0,0 +1,106 @@ +package io.javaoperatorsdk.operator.processing.dependent; + +import java.util.Optional; + +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class AbstractDependentResourceTest { + + + @Test + void throwsExceptionIfDesiredIsNullOnCreate() { + TestDependentResource testDependentResource = new TestDependentResource(); + testDependentResource.setSecondary(null); + testDependentResource.setDesired(null); + + assertThrows(DependentResourceException.class, + () -> testDependentResource.reconcile(new TestCustomResource(), null)); + + } + + @Test + void throwsExceptionIfDesiredIsNullOnUpdate() { + TestDependentResource testDependentResource = new TestDependentResource(); + testDependentResource.setSecondary(new ConfigMap()); + testDependentResource.setDesired(null); + + assertThrows(DependentResourceException.class, + () -> testDependentResource.reconcile(new TestCustomResource(), null)); + } + + private static class TestDependentResource + extends AbstractDependentResource + implements Creator, Updater { + + private ConfigMap secondary; + private ConfigMap desired; + + @Override + public Class resourceType() { + return ConfigMap.class; + } + + @Override + public Optional getSecondaryResource(TestCustomResource primary) { + return Optional.ofNullable(secondary); + } + + @Override + protected void onCreated(ResourceID primaryResourceId, ConfigMap created) {} + + @Override + protected void onUpdated(ResourceID primaryResourceId, ConfigMap updated, ConfigMap actual) {} + + @Override + protected ConfigMap desired(TestCustomResource primary, Context context) { + return desired; + } + + public ConfigMap getSecondary() { + return secondary; + } + + public TestDependentResource setSecondary(ConfigMap secondary) { + this.secondary = secondary; + return this; + } + + public ConfigMap getDesired() { + return desired; + } + + public TestDependentResource setDesired(ConfigMap desired) { + this.desired = desired; + return this; + } + + @Override + public ConfigMap create(ConfigMap desired, TestCustomResource primary, + Context context) { + return null; + } + + @Override + public ConfigMap update(ConfigMap actual, ConfigMap desired, TestCustomResource primary, + Context context) { + return null; + } + + @Override + public Matcher.Result match(ConfigMap actualResource, TestCustomResource primary, + Context context) { + var result = mock(Matcher.Result.class); + when(result.matched()).thenReturn(false); + return result; + } + } +} From b17e77876267e98f4651f697bdb6d4c35648f75d Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 25 Apr 2022 10:35:33 +0200 Subject: [PATCH 2/3] naming --- .../processing/dependent/AbstractDependentResource.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 3bbfe561d0..614e0f6a78 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -33,7 +33,7 @@ public ReconcileResult reconcile(P primary, Context

context) { if (maybeActual.isEmpty()) { if (creatable) { var desired = desired(primary, context); - throwIfNull(desired, primary); + throwIfDesiredNull(desired, primary); logForOperation("Creating", primary, desired); var createdResource = handleCreate(desired, primary, context); return ReconcileResult.resourceCreated(createdResource); @@ -44,7 +44,7 @@ public ReconcileResult reconcile(P primary, Context

context) { final var match = updater.match(actual, primary, context); if (!match.matched()) { final var desired = match.computedDesired().orElse(desired(primary, context)); - throwIfNull(desired, primary); + throwIfDesiredNull(desired, primary); logForOperation("Updating", primary, desired); var updatedResource = handleUpdate(actual, desired, primary, context); return ReconcileResult.resourceUpdated(updatedResource); @@ -61,7 +61,7 @@ public ReconcileResult reconcile(P primary, Context

context) { return ReconcileResult.noOperation(maybeActual.orElse(null)); } - private void throwIfNull(R desired, P primary) { + private void throwIfDesiredNull(R desired, P primary) { if (desired == null) { throw new DependentResourceException( "Desired cannot be null. Primary ID: " + ResourceID.fromResource(primary)); From 1dcd32afa2f083f34b02fd15a101b6926a87d638 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 25 Apr 2022 14:08:46 +0200 Subject: [PATCH 3/3] additional null checks --- .../dependent/AbstractDependentResource.java | 10 +++--- .../AbstractDependentResourceTest.java | 32 ++++++++++++++++++- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 614e0f6a78..c9c1d770c5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -33,7 +33,7 @@ public ReconcileResult reconcile(P primary, Context

context) { if (maybeActual.isEmpty()) { if (creatable) { var desired = desired(primary, context); - throwIfDesiredNull(desired, primary); + throwIfNull(desired, primary, "Desired"); logForOperation("Creating", primary, desired); var createdResource = handleCreate(desired, primary, context); return ReconcileResult.resourceCreated(createdResource); @@ -44,7 +44,7 @@ public ReconcileResult reconcile(P primary, Context

context) { final var match = updater.match(actual, primary, context); if (!match.matched()) { final var desired = match.computedDesired().orElse(desired(primary, context)); - throwIfDesiredNull(desired, primary); + throwIfNull(desired, primary, "Desired"); logForOperation("Updating", primary, desired); var updatedResource = handleUpdate(actual, desired, primary, context); return ReconcileResult.resourceUpdated(updatedResource); @@ -61,10 +61,10 @@ public ReconcileResult reconcile(P primary, Context

context) { return ReconcileResult.noOperation(maybeActual.orElse(null)); } - private void throwIfDesiredNull(R desired, P primary) { + private void throwIfNull(R desired, P primary, String descriptor) { if (desired == null) { throw new DependentResourceException( - "Desired cannot be null. Primary ID: " + ResourceID.fromResource(primary)); + descriptor + " cannot be null. Primary ID: " + ResourceID.fromResource(primary)); } } @@ -80,6 +80,7 @@ private void logForOperation(String operation, P primary, R desired) { protected R handleCreate(R desired, P primary, Context

context) { ResourceID resourceID = ResourceID.fromResource(primary); R created = creator.create(desired, primary, context); + throwIfNull(created, primary, "Created resource"); onCreated(resourceID, created); return created; } @@ -107,6 +108,7 @@ protected R handleCreate(R desired, P primary, Context

context) { protected R handleUpdate(R actual, R desired, P primary, Context

context) { ResourceID resourceID = ResourceID.fromResource(primary); R updated = updater.update(actual, desired, primary, context); + throwIfNull(updated, primary, "Updated resource"); onUpdated(resourceID, updated, actual); return updated; } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java index 382f138468..4edd3dfb4c 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java @@ -5,6 +5,7 @@ import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; @@ -30,13 +31,42 @@ void throwsExceptionIfDesiredIsNullOnCreate() { @Test void throwsExceptionIfDesiredIsNullOnUpdate() { TestDependentResource testDependentResource = new TestDependentResource(); - testDependentResource.setSecondary(new ConfigMap()); + testDependentResource.setSecondary(configMap()); testDependentResource.setDesired(null); assertThrows(DependentResourceException.class, () -> testDependentResource.reconcile(new TestCustomResource(), null)); } + @Test + void throwsExceptionIfCreateReturnsNull() { + TestDependentResource testDependentResource = new TestDependentResource(); + testDependentResource.setSecondary(null); + testDependentResource.setDesired(configMap()); + + assertThrows(DependentResourceException.class, + () -> testDependentResource.reconcile(new TestCustomResource(), null)); + } + + @Test + void throwsExceptionIfUpdateReturnsNull() { + TestDependentResource testDependentResource = new TestDependentResource(); + testDependentResource.setSecondary(configMap()); + testDependentResource.setDesired(configMap()); + + assertThrows(DependentResourceException.class, + () -> testDependentResource.reconcile(new TestCustomResource(), null)); + } + + private ConfigMap configMap() { + ConfigMap configMap = new ConfigMap(); + configMap.setMetadata(new ObjectMetaBuilder() + .withName("test") + .withNamespace("default") + .build()); + return configMap; + } + private static class TestDependentResource extends AbstractDependentResource implements Creator, Updater {