MID-11094 Fix correlation case closing on resolution error

(cherry picked from commit d35f7ff)
(cherry picked from commit 8dfa4dc)

Author: kay1313

model/cases-api/src/main/java/com/evolveum/midpoint/cases/api/extensions/EngineExtension.java

@@ -29,10 +29,24 @@ public interface EngineExtension {
     @NotNull Collection<String> getArchetypeOids();
 
     /**
-     * Called to finish case closing procedure. E.g. for approvals we may submit execution task here.
+     * Called before the case-closing state is persisted.
      *
-     * When called, the case should be in `closing` state. This may happen e.g. when approval execution task is submitted.
-     * After return, the case may be still in this state, or it may be `closed`.
+     * Implementations may execute retry-safe business logic required before the case
+     * may proceed to closing. If this method fails, the case remains open.
+     *
+     * Logic executed here must be safe under case-engine retries, e.g. in-memory,
+     * idempotent, or repository-transactional and repeatable.
+     */
+    default void prepareCaseClosing(@NotNull CaseEngineOperation operation, @NotNull OperationResult result)
+            throws SchemaException, ObjectNotFoundException,
+            ExpressionEvaluationException, ConfigurationException, CommunicationException, SecurityViolationException {
+    }
+
+    /**
+     * Called after the case-closing state has been successfully persisted.
+     *
+     * This hook is intended for post-commit finalization. Failures here do not mean
+     * that the case should remain open.
      */
     void finishCaseClosing(@NotNull CaseEngineOperation operation, @NotNull OperationResult result)
             throws SchemaException, ObjectAlreadyExistsException, ObjectNotFoundException,

model/cases-impl/src/main/java/com/evolveum/midpoint/cases/impl/engine/CaseEngineOperationImpl.java

@@ -122,14 +122,20 @@ private void commit(OperationResult parentResult)
                 .setMinor()
                 .build();
         try {
+            boolean caseIsClosing = CaseState.of(currentCase).isClosing();
+
+            if (caseIsClosing) {
+                // Run retry-safe pre-close logic before persisting the case as closing.
+                engineExtension.prepareCaseClosing(this, result);
+            }
 
             if (getCaseOid() == null) {
                 addCaseToRepo(result);
             } else {
                 modifyCaseInRepo(result); // Throws PreconditionViolationException if there's a race condition
             }
 
-            if (CaseState.of(currentCase).isClosing()) {
+            if (caseIsClosing) {
                 closeTheCase(result);
             }
 
@@ -178,7 +184,7 @@ private void closeTheCase(OperationResult result)
             throws SchemaException, ObjectAlreadyExistsException, ObjectNotFoundException, ExpressionEvaluationException,
             ConfigurationException, CommunicationException, SecurityViolationException {
 
-        // Invoking specific postprocessing of the case, like submitting a task that executes approved deltas.
+        // Invoking post-commit finalization specific to the case type.
         engineExtension.finishCaseClosing(this, result);
 
         // Note that the case may still be in "closing" state. E.g. an approval case is still closing here,

model/cases-impl/src/main/java/com/evolveum/midpoint/cases/impl/engine/extension/CorrelationCaseEngineExtension.java

@@ -51,6 +51,18 @@ protected SimpleCaseSchemaType getCaseSchema(@NotNull CaseEngineOperation operat
         return context != null ? context.getSchema() : null;
     }
 
+    @Override
+    public void prepareCaseClosing(
+            @NotNull CaseEngineOperation operation,
+            @NotNull OperationResult result)
+            throws SchemaException, ObjectNotFoundException, ExpressionEvaluationException,
+            CommunicationException, SecurityViolationException, ConfigurationException {
+        correlationService.prepareCorrelationCaseClosing(
+                operation.getCurrentCase(),
+                operation.getTask(),
+                result);
+    }
+
     @Override
     public void finishCaseClosing(
             @NotNull CaseEngineOperation operation,

model/model-api/src/main/java/com/evolveum/midpoint/model/api/correlation/CorrelationService.java

@@ -81,6 +81,18 @@ void completeCorrelationCase(
             throws SchemaException, ExpressionEvaluationException, CommunicationException, SecurityViolationException,
             ConfigurationException, ObjectNotFoundException;
 
+    /**
+     * Executes retry-safe business logic required before a correlation case may be
+     * persisted as closing. If this method fails, the case remains open.
+     */
+    default void prepareCorrelationCaseClosing(
+            @NotNull CaseType currentCase,
+            @NotNull Task task,
+            @NotNull OperationResult result)
+            throws SchemaException, ExpressionEvaluationException, CommunicationException, SecurityViolationException,
+            ConfigurationException, ObjectNotFoundException {
+    }
+
     /**
      * Instantiates a correlator
      */

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/correlation/CorrelationCaseManager.java

@@ -63,6 +63,8 @@
 public class CorrelationCaseManager {
 
     private static final Trace LOGGER = TraceManager.getTrace(CorrelationCaseManager.class);
+    private static final String OP_APPLY_CORRELATION_OUTCOME =
+            CorrelationCaseManager.class.getName() + ".importSelectedOutcome";
 
     @Autowired @Qualifier("cacheRepositoryService") private RepositoryService repositoryService;
     @Autowired private ModelService modelService;
@@ -122,7 +124,7 @@ private void createCase(
                         .schema(createCaseSchema(resource.getBusiness())))
                 .state(SchemaConstants.CASE_STATE_CREATED)
                 .metadata(new MetadataType()
-                    .createTimestamp(now));
+                        .createTimestamp(now));
         try {
             repositoryService.addObject(newCase.asPrismObject(), null, result);
         } catch (ObjectAlreadyExistsException e) {
@@ -290,33 +292,79 @@ void completeCorrelationCase(
             return;
         }
 
-        recordCaseCompletionInShadow(aCase, task, result);
+        recordCorrelationCompletionMetadataInShadow(aCase, task, result);
         caseCloser.closeCase(result);
+
+    }
+
+    /**
+     * Executes retry-safe correlation completion logic before the case is persisted as closing.
+     * Throws on failure to keep the case open.
+     */
+    void prepareCorrelationCaseClosing(
+            @NotNull CaseType aCase,
+            @NotNull Task task,
+            @NotNull OperationResult result)
+            throws SchemaException, ExpressionEvaluationException, CommunicationException, SecurityViolationException,
+            ConfigurationException, ObjectNotFoundException {
+        recordCorrelationOutcomeInShadow(aCase, result);
+
         correlationService.resolve(aCase, task, result);
+        if (result.isError()) {
+            throw new SystemException(result.getMessage());
+        }
 
         // As a convenience, we try to re-import the object. Technically this is not a part of the correlation case processing.
         // Whether we do this should be made configurable (in the future).
 
         String shadowOid = CorrelationCaseUtil.getShadowOidRequired(aCase);
+        OperationResult importResult = result.createSubresult(OP_APPLY_CORRELATION_OUTCOME);
         try {
-            modelService.importFromResource(shadowOid, task, result);
+            modelService.importFromResource(shadowOid, task, importResult);
+            importResult.computeStatusIfUnknown();
+            if (importResult.isError()) {
+                // Abort the case close if the real import/projector path reported an error.
+                throw new SystemException(importResult.getMessage());
+            }
         } catch (Exception e) {
             LoggingUtils.logUnexpectedException(LOGGER, "Couldn't import shadow {} from resource", e, shadowOid);
-            // Not throwing the exception higher (import itself is not prerequisite for completing the correlation case
+            throw e;
+        } finally {
+            importResult.close();
         }
+
     }
 
-    private void recordCaseCompletionInShadow(CaseType aCase, Task task, OperationResult result)
+    private void recordCorrelationOutcomeInShadow(CaseType aCase, OperationResult result)
             throws ObjectNotFoundException, SchemaException {
-
         String shadowOid = CorrelationCaseUtil.getShadowOidRequired(aCase);
-        XMLGregorianCalendar now = XmlTypeConverter.createXMLGregorianCalendar();
         ObjectReferenceType resultingOwnerRef =
                 CloneUtil.clone(
                         CorrelationCaseUtil.getResultingOwnerRef(aCase));
         CorrelationSituationType situation = resultingOwnerRef != null ?
                 CorrelationSituationType.EXISTING_OWNER : CorrelationSituationType.NO_OWNER;
 
+        try {
+            repositoryService.modifyObject(
+                    ShadowType.class,
+                    shadowOid,
+                    prismContext.deltaFor(ShadowType.class)
+                            .item(CORRELATION_RESULTING_OWNER_PATH)
+                            .replace(resultingOwnerRef)
+                            .item(CORRELATION_SITUATION_PATH)
+                            .replace(situation)
+                            .asItemDeltas(),
+                    result);
+        } catch (SchemaException | ObjectAlreadyExistsException e) {
+            throw SystemException.unexpected(e, "while recording correlation outcome in shadow");
+        }
+    }
+
+    private void recordCorrelationCompletionMetadataInShadow(CaseType aCase, Task task, OperationResult result)
+            throws ObjectNotFoundException, SchemaException {
+        String shadowOid = CorrelationCaseUtil.getShadowOidRequired(aCase);
+        XMLGregorianCalendar now = XmlTypeConverter.createXMLGregorianCalendar();
+
         try {
             repositoryService.modifyObject(
                     ShadowType.class,
@@ -328,10 +376,6 @@ private void recordCaseCompletionInShadow(CaseType aCase, Task task, OperationRe
                             .replaceRealValues(getPerformerRefs(aCase))
                             .item(CORRELATION_PERFORMER_COMMENT_PATH)
                             .replaceRealValues(getPerformerComments(aCase, task, result))
-                            .item(CORRELATION_RESULTING_OWNER_PATH)
-                            .replace(resultingOwnerRef)
-                            .item(CORRELATION_SITUATION_PATH)
-                            .replace(situation)
                             .asItemDeltas(),
                     result);
         } catch (SchemaException | ObjectAlreadyExistsException e) {

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/correlation/CorrelationServiceImpl.java

@@ -305,6 +305,15 @@ public void completeCorrelationCase(
         correlationCaseManager.completeCorrelationCase(currentCase, caseCloser, task, result);
     }
 
+    @Override
+    public void prepareCorrelationCaseClosing(
+            @NotNull CaseType currentCase,
+            @NotNull Task task,
+            @NotNull OperationResult result)
+            throws SchemaException, ObjectNotFoundException, ExpressionEvaluationException, CommunicationException, SecurityViolationException, ConfigurationException {
+        correlationCaseManager.prepareCorrelationCaseClosing(currentCase, task, result);
+    }
+
     /**
      * Resolves the given correlation case - in the correlator.
      * (For majority of correlators this is no-op. See {@link Correlator#resolve(CaseType, String, Task, OperationResult)}.)

release-notes.adoc

@@ -38,6 +38,7 @@ Overall, midPoint 4.9 is a major step from the past into the future.
 == Changes with Respect To Version 4.9.6
 
 * Fixed reconc task with tailoring GUI error in Subtasks tab. See bug:MID-11107[].
+* Fixed correlation case closing on resolution error. See bug:MID-11094[]
 
 == Changes with Respect To Version 4.9.5
 

testing/story/src/test/java/com/evolveum/midpoint/testing/story/correlation/AbstractSimpleIdMatchCorrelationTest.java

@@ -13,15 +13,18 @@
 import com.evolveum.midpoint.task.api.Task;
 import com.evolveum.midpoint.test.CsvTestResource;
 import com.evolveum.midpoint.test.TestObject;
+import com.evolveum.midpoint.util.exception.SystemException;
 import com.evolveum.midpoint.xml.ns._public.common.common_3.*;
 
 import org.testng.annotations.Test;
 
 import java.io.File;
+import java.util.Objects;
 
 import static com.evolveum.midpoint.schema.constants.MidPointConstants.NS_RI;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 /**
  * Tests ID Match correlation in the most simple case:
@@ -123,15 +126,53 @@ public void test110ImportJohnFromSisDisputed() throws Exception {
         assertUserByUsername("smith1", "after import")
                 .display()
                 .assertLinks(1, 0); // The account should not be linked yet
+    }
+
+    /**
+     * Resolves the disputed case created in the previous test by selecting the existing owner.
+     *
+     * The resolution must fail in the real import/projector path, and the case must remain open.
+     */
+    @Test
+    public void test115ResolveDisputedCaseAsExistingOwnerFailsAndKeepsCaseOpen() throws Exception {
+        Task task = getTestTask();
+        OperationResult result = task.getResult();
+
+        given("previously created disputed case for SIS account #2");
+        PrismObject<ShadowType> a2Before = findShadowByPrismName("2", RESOURCE_SIS.get(), result);
+        CaseType aCaseBefore = correlationCaseManager.findCorrelationCase(a2Before.asObjectable(), true, result);
+        assertThat(aCaseBefore).as("correlation case before resolution").isNotNull();
+
+        String caseOid = aCaseBefore.getOid();
 
         when("resolving the case");
-        resolveCase(aCase, johnOid, task, result);
+        assertThatThrownBy(() -> resolveCase(aCaseBefore, johnOid, task, result))
+                .isInstanceOf(SystemException.class)
+                .hasMessageContaining("already exists in lens context");
 
-        then("error should be reported");
-        assertFailure(result);
+        then("error should be reported and the case should remain open");
+        assertResultTreeContainsMessage(result);
 
-        // May be fragile. Adapt as needed.
-        assertThat(result.getMessage()).as("error message").contains("already exists in lens context");
+        CaseType caseAfter = getCase(caseOid);
+        assertCase(caseAfter, "case after failed resolution")
+                .display()
+                .assertOpen()
+                .workItems()
+                    .single()
+                        .assertNotClosed();
+
+        PrismObject<ShadowType> a2After = findShadowByPrismName("2", RESOURCE_SIS.get(), result);
+        ShadowCorrelationStateType correlationState = a2After.asObjectable().getCorrelation();
+        assertThat(correlationState).as("correlation state").isNotNull();
+        assertThat(correlationState.getCorrelationCaseCloseTimestamp())
+                .as("correlation case close timestamp")
+                .isNull();
+        assertThat(correlationState.getPerformerRef())
+                .as("correlation performer refs")
+                .isEmpty();
+        assertThat(correlationState.getPerformerComment())
+                .as("correlation performer comments")
+                .isEmpty();
 
         assertUserByUsername("smith1", "after case resolution")
                 .display()
@@ -191,4 +232,14 @@ public void test120ImportJohnFromSisDisputedAgain() throws Exception {
                 .display()
                 .assertLinks(1, 0);
     }
+
+    private void assertResultTreeContainsMessage(OperationResult result) {
+        assertThat(
+                result.getResultStream()
+                        .map(OperationResult::getMessage)
+                        .filter(Objects::nonNull)
+                        .anyMatch(message -> message.contains("already exists in lens context")))
+                .as("operation result tree should contain message fragment '%s'", "already exists in lens context")
+                .isTrue();
+    }
 }