From e4ffd6c61d38f7cbc431fa40fa15ffd1970094c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 1 Feb 2024 09:51:34 +0100 Subject: [PATCH 1/3] fix: multiple dependents of same type exceptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../dependent/workflow/WorkflowResult.java | 19 +++++- .../workflow/WorkflowResultTest.java | 67 +++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index ba9e0f5755..0b9eef239e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -1,8 +1,8 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; -import java.util.stream.Collectors; import io.javaoperatorsdk.operator.AggregatedOperatorException; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @@ -36,9 +36,22 @@ public boolean erroredDependentsExist() { public void throwAggregateExceptionIfErrorsPresent() { if (erroredDependentsExist()) { + Map exceptionMap = new HashMap<>(); + Map numberOfClasses = new HashMap<>(); + + for (Entry entry : erroredDependents.entrySet()) { + String name = entry.getKey().getClass().getName(); + var num = numberOfClasses.getOrDefault(name, 0); + if (num > 0) { + exceptionMap.put(name + "_" + num, entry.getValue()); + } else { + exceptionMap.put(name, entry.getValue()); + } + numberOfClasses.put(name, num + 1); + } + throw new AggregatedOperatorException("Exception(s) during workflow execution.", - erroredDependents.entrySet().stream() - .collect(Collectors.toMap(e -> e.getKey().getClass().getName(), Entry::getValue))); + exceptionMap); } } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java new file mode 100644 index 0000000000..9a234da570 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java @@ -0,0 +1,67 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.AggregatedOperatorException; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.*; + +class WorkflowResultTest { + + @Test + void throwsExceptionWithoutNumberingIfAllDifferentClass() { + var res = new WorkflowResult(Map.of(new DependentA(), new RuntimeException(), + new DependentB(), new RuntimeException())); + try { + res.throwAggregateExceptionIfErrorsPresent(); + } catch (AggregatedOperatorException e) { + assertThat(e.getAggregatedExceptions()).containsOnlyKeys(DependentA.class.getName(), + DependentB.class.getName()); + } + } + + @Test + void numbersDependentClassNamesIfMoreOfSameType() { + var res = new WorkflowResult(Map.of(new DependentA(), new RuntimeException(), + new DependentA(), new RuntimeException())); + try { + res.throwAggregateExceptionIfErrorsPresent(); + } catch (AggregatedOperatorException e) { + assertThat(e.getAggregatedExceptions()) + .containsOnlyKeys(DependentA.class.getName(), DependentA.class.getName() + "_" + 1); + } + } + + @SuppressWarnings("rawtypes") + static class DependentA implements DependentResource { + @Override + public ReconcileResult reconcile(HasMetadata primary, Context context) { + return null; + } + + @Override + public Class resourceType() { + return null; + } + } + + @SuppressWarnings("rawtypes") + static class DependentB implements DependentResource { + @Override + public ReconcileResult reconcile(HasMetadata primary, Context context) { + return null; + } + + @Override + public Class resourceType() { + return null; + } + } +} From 801939343a535ea821864e963e3c9dbd4e2ec810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 1 Feb 2024 09:53:55 +0100 Subject: [PATCH 2/3] extract delimiter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../processing/dependent/workflow/WorkflowResult.java | 3 ++- .../processing/dependent/workflow/WorkflowResultTest.java | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 0b9eef239e..cac8388a6e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -10,6 +10,7 @@ @SuppressWarnings("rawtypes") class WorkflowResult { + public static final String NUMBER_DELIMITER = "_"; private final Map erroredDependents; WorkflowResult(Map erroredDependents) { @@ -43,7 +44,7 @@ public void throwAggregateExceptionIfErrorsPresent() { String name = entry.getKey().getClass().getName(); var num = numberOfClasses.getOrDefault(name, 0); if (num > 0) { - exceptionMap.put(name + "_" + num, entry.getValue()); + exceptionMap.put(name + NUMBER_DELIMITER + num, entry.getValue()); } else { exceptionMap.put(name, entry.getValue()); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java index 9a234da570..8a4c9e7fa3 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java @@ -10,6 +10,7 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; +import static io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowResult.NUMBER_DELIMITER; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.*; @@ -35,7 +36,8 @@ void numbersDependentClassNamesIfMoreOfSameType() { res.throwAggregateExceptionIfErrorsPresent(); } catch (AggregatedOperatorException e) { assertThat(e.getAggregatedExceptions()) - .containsOnlyKeys(DependentA.class.getName(), DependentA.class.getName() + "_" + 1); + .containsOnlyKeys(DependentA.class.getName(), + DependentA.class.getName() + NUMBER_DELIMITER + 1); } } From 93b611d78fc2cf56c798ce0aa21b2689a250c204 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 6 Feb 2024 11:24:31 +0100 Subject: [PATCH 3/3] fix: do not expose underlying de-duplication mechanism Signed-off-by: Chris Laprun --- .../processing/dependent/workflow/WorkflowResult.java | 2 +- .../processing/dependent/workflow/WorkflowResultTest.java | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index cac8388a6e..77fc2dcbfe 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -10,7 +10,7 @@ @SuppressWarnings("rawtypes") class WorkflowResult { - public static final String NUMBER_DELIMITER = "_"; + private static final String NUMBER_DELIMITER = "_"; private final Map erroredDependents; WorkflowResult(Map erroredDependents) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java index 8a4c9e7fa3..48cf3fa75a 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java @@ -10,9 +10,7 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; -import static io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowResult.NUMBER_DELIMITER; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.*; class WorkflowResultTest { @@ -35,9 +33,7 @@ void numbersDependentClassNamesIfMoreOfSameType() { try { res.throwAggregateExceptionIfErrorsPresent(); } catch (AggregatedOperatorException e) { - assertThat(e.getAggregatedExceptions()) - .containsOnlyKeys(DependentA.class.getName(), - DependentA.class.getName() + NUMBER_DELIMITER + 1); + assertThat(e.getAggregatedExceptions()).hasSize(2); } }