Fix non-leaf activities with explicit identifiers

When a non-leaf activity (e.g. a reconciliation) was defined with an explicit identifier, all its children obtained the identifier, causing troubles. This is now fixed by stopping cloning the identifier value. (Thanks to Marc Fueller for the analysis.) This resolves MID-7894. Other changes: - Added a check for conflicting identifiers in child activities. - Slightly improved thread safety of Activity#childrenMap.
Evolveum · Apr 21, 2022 · c32beee · c32beee
1 parent e1b8a6d
commit c32beee
Show file tree

Hide file tree

Showing 9 changed files with 60 additions and 26 deletions.
diff --git a/...l-impl/src/main/java/com/evolveum/midpoint/model/impl/cleanup/CleanupActivityHandler.java b/...l-impl/src/main/java/com/evolveum/midpoint/model/impl/cleanup/CleanupActivityHandler.java
@@ -90,7 +90,7 @@ public void unregister() {
 
         ActivityStateDefinition<AbstractActivityWorkStateType> stateDef = ActivityStateDefinition.normal();
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) ->
                         new CleanupPartialActivityRun<>(
                                 context, Part.AUDIT_RECORDS, CleanupPoliciesType::getAuditRecords,
@@ -101,7 +101,7 @@ public void unregister() {
                 parentActivity));
 
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) ->
                         new CleanupPartialActivityRun<>(
                                 context, Part.CLOSED_TASKS, CleanupPoliciesType::getClosedTasks,
@@ -112,7 +112,7 @@ public void unregister() {
                 parentActivity));
 
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) ->
                         new CleanupPartialActivityRun<>(
                                 context, Part.CLOSED_CASES, CleanupPoliciesType::getClosedCases,
@@ -123,7 +123,7 @@ public void unregister() {
                 parentActivity));
 
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) ->
                         new CleanupPartialActivityRun<>(
                                 context, Part.DEAD_NODES, CleanupPoliciesType::getDeadNodes,
@@ -134,7 +134,7 @@ public void unregister() {
                 parentActivity));
 
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) ->
                         new CleanupPartialActivityRun<>(
                                 context, Part.OUTPUT_REPORTS, CleanupPoliciesType::getOutputReports,
@@ -145,7 +145,7 @@ public void unregister() {
                 parentActivity));
 
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) ->
                         new CleanupPartialActivityRun<>(
                                 context, Part.CLOSED_CERTIFICATION_CAMPAIGNS, CleanupPoliciesType::getClosedCertificationCampaigns,

diff --git a/...java/com/evolveum/midpoint/model/impl/sync/tasks/recon/ReconciliationActivityHandler.java b/...java/com/evolveum/midpoint/model/impl/sync/tasks/recon/ReconciliationActivityHandler.java
@@ -76,7 +76,7 @@ public void unregister() {
             Activity<ReconciliationWorkDefinition, ReconciliationActivityHandler> parentActivity) {
         ArrayList<Activity<?, ?>> children = new ArrayList<>();
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) -> new OperationCompletionActivityRun(context,
                         "Reconciliation (operation completion)"),
                 null,
@@ -100,15 +100,15 @@ public void unregister() {
                 ActivityStateDefinition.normal(),
                 parentActivity));
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) -> new ResourceObjectsReconciliationActivityRun(context,
                         "Reconciliation (on resource)" + modeSuffix(context)),
                 this::beforeResourceObjectsReconciliation,
                 (i) -> ModelPublicConstants.RECONCILIATION_RESOURCE_OBJECTS_ID,
                 ActivityStateDefinition.normal(),
                 parentActivity));
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) -> new RemainingShadowsActivityRun(context,
                         "Reconciliation (remaining shadows)" + modeSuffix(context)),
                 null,
@@ -120,7 +120,7 @@ public void unregister() {
 
     private ActivityDefinition<ReconciliationWorkDefinition> createPreviewDefinition(
             @NotNull ActivityDefinition<ReconciliationWorkDefinition> original) {
-        ActivityDefinition<ReconciliationWorkDefinition> clone = original.clone();
+        ActivityDefinition<ReconciliationWorkDefinition> clone = original.cloneWithoutId();
         clone.getWorkDefinition().setExecutionMode(ExecutionModeType.PREVIEW);
         clone.getControlFlowDefinition().setSkip();
         return clone;

diff --git a/...java/com/evolveum/midpoint/model/impl/tasks/scanner/FocusValidityScanActivityHandler.java b/...java/com/evolveum/midpoint/model/impl/tasks/scanner/FocusValidityScanActivityHandler.java
@@ -63,7 +63,7 @@ public void unregister() {
         switch (queryStyle) {
             case SINGLE_QUERY:
                 children.add(EmbeddedActivity.create(
-                        parentActivity.getDefinition().clone(),
+                        parentActivity.getDefinition().cloneWithoutId(),
                         (context, result) -> new FocusValidityScanPartialRun(context, COMBINED),
                         null,
                         (i) -> ModelPublicConstants.FOCUS_VALIDITY_SCAN_FULL_ID,
@@ -72,14 +72,14 @@ public void unregister() {
                 break;
             case SEPARATE_OBJECT_AND_ASSIGNMENT_QUERIES:
                 children.add(EmbeddedActivity.create(
-                        parentActivity.getDefinition().clone(),
+                        parentActivity.getDefinition().cloneWithoutId(),
                         (context, result) -> new FocusValidityScanPartialRun(context, OBJECTS),
                         null,
                         (i) -> ModelPublicConstants.FOCUS_VALIDITY_SCAN_OBJECTS_ID,
                         stateDef,
                         parentActivity));
                 children.add(EmbeddedActivity.create(
-                        parentActivity.getDefinition().clone(),
+                        parentActivity.getDefinition().cloneWithoutId(),
                         (context, result) -> new FocusValidityScanPartialRun(context, ASSIGNMENTS),
                         null,
                         (i) -> ModelPublicConstants.FOCUS_VALIDITY_SCAN_ASSIGNMENTS_ID,

diff --git a/model/model-intest/src/test/resources/tasks/recon/task-reconciliation.xml b/model/model-intest/src/test/resources/tasks/recon/task-reconciliation.xml
@@ -18,6 +18,7 @@
     <executionState>closed</executionState> <!-- run by the test code -->
 
     <activity>
+        <identifier>my-reconciliation</identifier>
         <work>
             <reconciliation>
                 <resourceObjects>

diff --git a/...va/com/evolveum/midpoint/report/impl/activity/DistributedReportExportActivityHandler.java b/...va/com/evolveum/midpoint/report/impl/activity/DistributedReportExportActivityHandler.java
@@ -88,14 +88,14 @@ public void unregister() {
             Activity<DistributedReportExportWorkDefinition, DistributedReportExportActivityHandler> parentActivity) {
         ArrayList<Activity<?, ?>> children = new ArrayList<>();
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) -> new ReportDataCreationActivityRun(context),
                 this::createEmptyAggregatedDataObject,
                 (i) -> "data-creation",
                 ActivityStateDefinition.normal(),
                 parentActivity));
         children.add(EmbeddedActivity.create(
-                parentActivity.getDefinition().clone(),
+                parentActivity.getDefinition().cloneWithoutId(),
                 (context, result) -> new ReportDataAggregationActivityRun(context),
                 null,
                 (i) -> "data-aggregation",

diff --git a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/activity/Activity.java b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/activity/Activity.java
@@ -151,6 +151,12 @@ public void setLocalRoot(boolean localRoot) {
         }
     }
 
+    private @NotNull Map<String, Activity<?, ?>> getChildrenMapCopy() {
+        synchronized (childrenMap) {
+            return new HashMap<>(childrenMap);
+        }
+    }
+
     public @NotNull List<Activity<?, ?>> getChildrenCopyExceptSkipped() {
         synchronized (childrenMap) {
             return childrenMap.values().stream()
@@ -168,7 +174,8 @@ public String debugDump(int indent) {
         DebugUtil.debugDumpWithLabelLn(sb, "parent", String.valueOf(getParent()), indent + 1);
         DebugUtil.debugDumpWithLabelLn(sb, "path", String.valueOf(getPath()), indent + 1);
         DebugUtil.debugDumpWithLabelLn(sb, "local path", String.valueOf(getLocalPath()), indent + 1);
-        DebugUtil.debugDumpWithLabel(sb, "children (initialized=" + childrenMapInitialized + ")", childrenMap, indent + 1);
+        DebugUtil.debugDumpWithLabel(sb, "children (initialized=" + childrenMapInitialized + ")",
+                getChildrenMapCopy(), indent + 1);
         return sb.toString();
     }
 
@@ -248,24 +255,34 @@ private TaskRoleType getRoleOfTask(Task activityTask) {
             return child;
         } else {
             throw new IllegalArgumentException("Child with identifier " + identifier + " was not found among children of "
-                    + this + ". Known children are: " + childrenMap.keySet());
+                    + this + ". Known children are: " + getChildrenMapCopy().keySet());
         }
     }
 
     public void initializeChildrenMapIfNeeded() throws SchemaException {
-        if (!childrenMapInitialized) {
-            assert childrenMap.isEmpty();
-            createChildren();
-            childrenMapInitialized = true;
+        synchronized (childrenMap) { // just for sure
+            if (!childrenMapInitialized) {
+                assert childrenMap.isEmpty();
+                createChildren();
+                childrenMapInitialized = true;
+            }
         }
     }
 
+    /** Creates children in {@link #childrenMap}. The caller must hold the lock on {@link #childrenMap}. */
     private void createChildren() throws SchemaException {
         ArrayList<Activity<?, ?>> childrenList = getHandler().createChildActivities(this);
         setupIdentifiers(childrenList);
         tailorChildren(childrenList);
         setupIdentifiers(childrenList);
-        childrenList.forEach(child -> childrenMap.put(child.getIdentifier(), child));
+        childrenList.forEach(this::addChild);
+    }
+
+    private void addChild(@NotNull Activity<?, ?> child) {
+        var previous = childrenMap.put(child.getIdentifier(), child);
+        stateCheck(previous == null,
+                "Multiple child activities with the same identifier: %s (%s, %s)",
+                child.getIdentifier(), child, previous);
     }
 
     private void setupIdentifiers(List<Activity<?, ?>> childrenList) {
@@ -389,7 +406,7 @@ void applySubtaskTailoring(@NotNull ActivitySubtaskDefinitionType subtaskSpecifi
 
     public void accept(@NotNull ActivityVisitor visitor) {
         visitor.visit(this);
-        childrenMap.values()
+        getChildrenCopy()
                 .forEach(child -> child.accept(visitor));
     }
 

diff --git a/...epo-common/src/main/java/com/evolveum/midpoint/repo/common/activity/EmbeddedActivity.java b/...epo-common/src/main/java/com/evolveum/midpoint/repo/common/activity/EmbeddedActivity.java
@@ -49,7 +49,8 @@ private EmbeddedActivity(@NotNull ActivityDefinition<WD> definition, @NotNull Ac
     /**
      * Creates an embedded activity.
      *
-     * @param definition Definition to be used. Should be freely modifiable (typically cloned)!
+     * @param definition Definition to be used. Should be freely modifiable (typically cloned,
+     * e.g. via {@link ActivityDefinition#cloneWithoutId()})!
      */
     public static <WD extends WorkDefinition, AH extends ActivityHandler<WD, AH>> EmbeddedActivity<WD, AH> create(
             @NotNull ActivityDefinition<WD> definition,

diff --git a/...c/main/java/com/evolveum/midpoint/repo/common/activity/definition/ActivityDefinition.java b/...c/main/java/com/evolveum/midpoint/repo/common/activity/definition/ActivityDefinition.java
@@ -246,10 +246,25 @@ private void applyExecutionModeTailoring(@NotNull ActivityTailoringType tailorin
 
     /**
      * Does the deep clone. The goal is to be able to modify cloned definition freely.
+     *
+     * BEWARE: Do not use this method to create a definition clone to be used for child activities.
+     * Use {@link #cloneWithoutId()} instead. See MID-7894.
      */
     @SuppressWarnings({ "MethodDoesntCallSuperMethod" })
     @Override
     public ActivityDefinition<WD> clone() {
+        return cloneInternal(explicitlyDefinedIdentifier);
+    }
+
+    /**
+     * As {@link #clone()} but discards the {@link #explicitlyDefinedIdentifier} value.
+     */
+    public ActivityDefinition<WD> cloneWithoutId() {
+        return cloneInternal(null);
+    }
+
+    @NotNull
+    private ActivityDefinition<WD> cloneInternal(String explicitlyDefinedIdentifier) {
         //noinspection unchecked
         return new ActivityDefinition<>(
                 explicitlyDefinedIdentifier,

diff --git a/.../evolveum/midpoint/repo/common/tasks/handlers/composite/CompositeMockActivityHandler.java b/.../evolveum/midpoint/repo/common/tasks/handlers/composite/CompositeMockActivityHandler.java
@@ -79,7 +79,7 @@ public void unregister() {
         ArrayList<Activity<?, ?>> children = new ArrayList<>();
         if (workDefinition.isOpeningEnabled()) {
             children.add(EmbeddedActivity.create(
-                    parentActivity.getDefinition().clone(),
+                    parentActivity.getDefinition().cloneWithoutId(),
                     (context, result) -> new MockOpeningActivityRun(context),
                     this::runBeforeExecution,
                     (i) -> "opening",
@@ -88,7 +88,7 @@ public void unregister() {
         }
         if (workDefinition.isClosingEnabled()) {
             children.add(EmbeddedActivity.create(
-                    parentActivity.getDefinition().clone(),
+                    parentActivity.getDefinition().cloneWithoutId(),
                     (context, result) -> new MockClosingActivityRun(context),
                     this::runBeforeExecution,
                     (i) -> "closing",