Improve exception recording into OperationResult

In order to streamline recording of exceptions into OperationResult,
the following changes were done:

1. We now support recording exceptions in places other than the "catch"
block (near the place where OperationResult was created). See
OperationResult#recordException method and the OperationResult javadoc.

2. We now support flexible severity of exceptions. No more muting
ObjectNotFoundExceptions where "allow not found" is set.

3. Simpler cleanup() method as a replacement for cleanupResult(...).

We also improved ObjectNotFoundException throwing (at many places) by:
- adding more information (type, oid);
- using e.wrap() method instead of manually creating the new instance
if re-throwing the exception.

A few case of ObjectNotFoundException and SchemaException were changed
to ConfigurationException.

gui/admin-gui/src/main/java/com/evolveum/midpoint/gui/impl/page/login/PageSelfRegistration.java

@@ -486,7 +486,7 @@ private <O extends ObjectType> String generateNonce(NonceCredentialsPolicyType n
                     noncePolicy.getValuePolicyRef().getOid(), PageSelfRegistration.this, task, result);
             if (valuePolicy == null) {
                 LOGGER.error("Nonce cannot be generated, as value policy {} cannot be fetched", noncePolicy.getValuePolicyRef().getOid());
-                throw new ObjectNotFoundException("Nonce cannot be generated");         // no more information (security); TODO implement more correctly
+                throw new ObjectNotFoundException("Nonce cannot be generated"); // no more information (security); TODO implement more correctly
             }
             policy = valuePolicy.asObjectable();
         }

gui/admin-gui/src/main/java/com/evolveum/midpoint/web/page/admin/cases/PageCaseWorkItem.java

@@ -140,7 +140,10 @@ private CaseWorkItemType loadCaseWorkItemIfNecessary() {
         try {
             List<CaseWorkItemType> caseWorkItemList = caseModel.getObject().getWorkItem();
             if (caseWorkItemList == null) {
-                throw new ObjectNotFoundException("No case work item found for case " + workItemId.getCaseOid() + " with id " + workItemId.getId());
+                throw new ObjectNotFoundException(
+                        "No case work item found for case " + workItemId.getCaseOid() + " with id " + workItemId.getId(),
+                        CaseWorkItemType.class,
+                        workItemId.asString());
             }
             for (CaseWorkItemType caseWorkItemType : caseWorkItemList) {
                 if (caseWorkItemType.getId().equals(workItemId.getId())) {

infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptions.java

@@ -609,6 +609,11 @@ public static boolean isAllowNotFound(GetOperationOptions options) {
         return options.allowNotFound;
     }
 
+    public static boolean isAllowNotFound(Collection<SelectorOptions<GetOperationOptions>> options) {
+        return isAllowNotFound(
+                SelectorOptions.findRootOptions(options));
+    }
+
     public static Collection<SelectorOptions<GetOperationOptions>> createAllowNotFoundCollection() {
         return SelectorOptions.createCollection(createAllowNotFound());
     }

infra/schema/src/main/java/com/evolveum/midpoint/schema/result/OperationResult.java

@@ -24,6 +24,7 @@
 import java.util.stream.Stream;
 import javax.xml.namespace.QName;
 
+import com.evolveum.midpoint.util.exception.ObjectNotFoundException;
 import com.evolveum.midpoint.util.statistics.OperationsPerformanceMonitorImpl;
 
 import org.apache.commons.lang3.StringUtils;
@@ -121,6 +122,24 @@
  * there's non-negligible processing after that point), {@link #setStatus(OperationResultStatus)} method or its variants
  * (like {@link #setSuccess()}) can be used. These fill-in {@link #status} field without closing the whole operation result.
  *
+ * === Note: Recording Exceptions
+ *
+ * The Correct Closure Principle (#1) dictates that the operation result has to be correctly closed even in the case
+ * of exception occurring. That is why we usually create a "try-catch" block for the code covered by a single operation result.
+ * Traditionally, the {@link #recordFatalError(Throwable)} call was put into the `catch` code.
+ *
+ * However, there may be situations where a different handling is required:
+ *
+ * . If the exception is non-fatal or even completely benign. This may be the case of e.g. expected ("allowed")
+ * object-not-found conditions. See {@link ObjectNotFoundException#getSeverity()}.
+ * . If the exception was processed by other means. For example, a custom error message was already provided.
+ *
+ * To handle these cases, {@link #recordException(Throwable)} should be used instead of {@link #recordFatalError(Throwable)}.
+ * The difference is that the former checks the {@link #exceptionRecorded} flag to see if the exception was already
+ * processed. See also {@link #markExceptionRecorded()}.
+ *
+ * NOTE: This mechanism is *experimental*.
+ *
  * === Suggested Use
  *
  * Stemming from the above, the following can be seen as a suggested way how to use the operation result:
@@ -137,7 +156,7 @@
  *     try {
  *         // ... some meat here ...
  *     } catch (SomeException | RuntimeException e) {
- *         result.recordFatalError(e);
+ *         result.recordException(e);
  *         throw e;
  *     } finally {
  *         result.close();
@@ -237,6 +256,9 @@ public class OperationResult
 
     private final List<LogSegmentType> logSegments = new ArrayList<>();
 
+    /** See {@link #markExceptionRecorded()}. */
+    private boolean exceptionRecorded;
+
     // The following properties are NOT SERIALIZED
     private CompiledTracingProfile tracingProfile;
 
@@ -1165,22 +1187,6 @@ public void close() {
         computeStatusIfUnknown();
     }
 
-    public void closeAndCleanup() {
-        close();
-        cleanupResult();
-    }
-
-    /**
-     * Closes the result and cleans it up.
-     *
-     * BEWARE: It does *NOT* record the {@link Throwable} as a fatal error! Its value is used only for debugging
-     * potential problems during result cleanup.
-     */
-    public void closeAndCleanup(@Nullable Throwable t) {
-        close();
-        cleanupResult(t);
-    }
-
     public void computeStatusIfUnknown() {
         recordEnd();
         if (isUnknown()) {
@@ -1679,17 +1685,67 @@ public void recordFatalError(Throwable cause) {
     }
 
     /**
-     * Records a fatal error if it was not recorded before.
-     * TODO Not 100% safe, because the fatal error could be recorded for some other reason.
-     *  We have to improve error reporting in general.
+     * A more sophisticated replacement for {@link #recordFatalError(String, Throwable)}.
+     *
+     * . Takes care not to overwrite the exception if it was already processed.
+     * . Marks the exception as processed.
+     * . Sets the appropriate result status.
+     *
+     * See the class javadoc.
      */
-    @Experimental
-    public void recordFatalErrorIfNeeded(Throwable t) {
-        if (status != OperationResultStatus.FATAL_ERROR) {
-            recordFatalError(t);
+    public void recordException(String message, @NotNull Throwable cause) {
+        if (!exceptionRecorded) {
+            recordStatus(
+                    OperationResultStatus.forThrowable(cause),
+                    message,
+                    cause);
+            markExceptionRecorded();
         }
     }
 
+    /** Convenience version of {@link #recordException(String, Throwable)} (with no custom message). */
+    public void recordException(@NotNull Throwable cause) {
+        recordException(cause.getMessage(), cause);
+    }
+
+    /** As {@link #recordException(String, Throwable)} but does not mark operation as finished. */
+    public void recordExceptionNotFinish(String message, @NotNull Throwable cause) {
+        if (!exceptionRecorded) {
+            recordStatusNotFinish(
+                    OperationResultStatus.forThrowable(cause),
+                    message,
+                    cause);
+            markExceptionRecorded();
+        }
+    }
+
+    /** Convenience version of {@link #recordExceptionNotFinish(String, Throwable)} (with no custom message). */
+    public void recordExceptionNotFinish(@NotNull Throwable cause) {
+        recordExceptionNotFinish(cause.getMessage(), cause);
+    }
+
+    /**
+     * Marks the current exception (that is expected to be throws outside the context of the current operation)
+     * as already processed - so no further actions (besides closing the result) are necessary.
+     *
+     * See also the class javadoc.
+     *
+     * @see #unmarkExceptionRecorded()
+     */
+    @Experimental
+    public void markExceptionRecorded() {
+        exceptionRecorded = true;
+    }
+
+    /**
+     * "Un-marks" the exception as being recorded. To be used when the code decides e.g. that the exception will not
+     * be thrown out of the context of the current operation.
+     */
+    @SuppressWarnings({ "unused", "WeakerAccess" }) // Waiting to be used
+    public void unmarkExceptionRecorded() {
+        exceptionRecorded = false;
+    }
+
     public void recordFatalErrorNotFinish(Throwable cause) {
         recordStatusNotFinish(OperationResultStatus.FATAL_ERROR, cause.getMessage(), cause);
     }
@@ -2168,12 +2224,22 @@ public void cleanupResultDeeply() {
         emptyIfNull(subresults).forEach(OperationResult::cleanupResultDeeply);
     }
 
+    /**
+     * As {@link #cleanupResult(Throwable)} but uses the recorded exception for diagnostics. It is more convenient than that
+     * method, as it can be used in the `finally` block - assuming that the exception was recorded in the `catch` block
+     * or earlier.
+     */
+    public void cleanup() {
+        cleanupInternal(cause);
+    }
+
     /**
      * Removes all the successful minor results. Also checks if the result is roughly consistent
      * and complete. (e.g. does not have unknown operation status, etc.)
      */
+    @Deprecated // use cleanup()
     public void cleanupResult() {
-        cleanupResult(null);
+        cleanupInternal(null);
     }
 
     /**
@@ -2183,8 +2249,12 @@ public void cleanupResult() {
      * The argument "e" is for easier use of the cleanup in the exceptions handlers. The original exception is passed
      * to the IAE that this method produces for easier debugging.
      */
+    @Deprecated // use cleanup() with recordException()
     public void cleanupResult(Throwable e) {
+        cleanupInternal(e);
+    }
 
+    private void cleanupInternal(Throwable e) {
         if (!canBeCleanedUp()) {
             return; // TEMPORARY fixme
         }

infra/schema/src/main/java/com/evolveum/midpoint/schema/result/OperationResultStatus.java

@@ -6,7 +6,10 @@
  */
 package com.evolveum.midpoint.schema.result;
 
+import com.evolveum.midpoint.util.exception.CommonException;
+
 import org.jetbrains.annotations.Contract;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import com.evolveum.midpoint.xml.ns._public.common.common_3.OperationResultStatusType;
@@ -129,6 +132,31 @@ public enum OperationResultStatus {
         }
     }
 
+    static OperationResultStatus forThrowable(Throwable cause) {
+        if (cause instanceof CommonException) {
+            return forCommonExceptionStatus(((CommonException) cause).getSeverity());
+        } else {
+            return FATAL_ERROR;
+        }
+    }
+
+    private static OperationResultStatus forCommonExceptionStatus(@NotNull CommonException.Severity severity) {
+        switch (severity) {
+            case FATAL_ERROR:
+                return FATAL_ERROR;
+            case PARTIAL_ERROR:
+                return PARTIAL_ERROR;
+            case WARNING:
+                return WARNING;
+            case HANDLED_ERROR:
+                return HANDLED_ERROR;
+            case SUCCESS:
+                return SUCCESS;
+            default:
+                throw new AssertionError(severity);
+        }
+    }
+
     public OperationResultStatusType createStatusType() {
         return createStatusType(this);
     }

infra/schema/src/main/java/com/evolveum/midpoint/schema/util/PolicyRuleTypeUtil.java

@@ -508,7 +508,11 @@ public JAXBElement<? extends AbstractPolicyConstraintType> resolve(@NotNull Stri
                     return rv;
                 }
                 if (usedSuppliers >= constraintsSuppliers.size()) {
-                    throw new ObjectNotFoundException("No policy constraint named '" + name + "' could be found. Known constraints: " + constraintsMap.keySet());
+                    throw new ObjectNotFoundException(
+                            String.format(
+                                    "No policy constraint named '%s' could be found. Known constraints: %s",
+                                    name, constraintsMap.keySet()),
+                            AbstractPolicyConstraintType.class, name);
                 }
                 List<Map.Entry<String, JAXBElement<? extends AbstractPolicyConstraintType>>> newEntries =
                         constraintsSuppliers.get(usedSuppliers++).get();

model/certification-impl/src/main/java/com/evolveum/midpoint/certification/impl/AccCertCaseOperationsHelper.java

@@ -73,15 +73,21 @@ void recordDecision(String campaignOid, long caseId, long workItemId, AccessCert
             SchemaException, ObjectAlreadyExistsException {
         AccessCertificationCaseType acase = queryHelper.getCase(campaignOid, caseId, task, result);
         if (acase == null) {
-            throw new ObjectNotFoundException("Case " + caseId + " was not found in campaign " + campaignOid);
+            throw new ObjectNotFoundException(
+                    "Case " + caseId + " was not found in campaign " + campaignOid,
+                    AccessCertificationCaseType.class,
+                    campaignOid + ":" + caseId);
         }
         AccessCertificationCampaignType campaign = CertCampaignTypeUtil.getCampaign(acase);
         if (campaign == null) {
             throw new IllegalStateException("No owning campaign present in case " + acase);
         }
         AccessCertificationWorkItemType workItem = CertCampaignTypeUtil.findWorkItem(acase, workItemId);
         if (workItem == null) {
-            throw new ObjectNotFoundException("Work item " + workItemId + " was not found in campaign " + toShortString(campaign) + ", case " + caseId);
+            throw new ObjectNotFoundException(
+                    "Work item " + workItemId + " was not found in campaign " + toShortString(campaign) + ", case " + caseId,
+                    AccessCertificationWorkItemType.class,
+                    campaignOid + ":" + workItemId);
         }
 
         ObjectReferenceType responderRef = ObjectTypeUtil.createObjectRef(securityContextManager.getPrincipal().getFocus(), prismContext);

model/model-common/src/main/java/com/evolveum/midpoint/model/common/expression/evaluator/FunctionExpressionEvaluator.java

@@ -96,14 +96,14 @@ private ExpressionType getFunctionExpressionBean(ObjectReferenceType functionLib
 
             List<ExpressionType> allFunctions = functionLibrary.getFunction();
             if (CollectionUtils.isEmpty(allFunctions)) {
-                throw new ObjectNotFoundException(
+                throw new ConfigurationException(
                         "No functions defined in referenced function library: " + functionLibrary + " used in " +
                                 context.getContextDescription());
             }
 
             String functionName = expressionEvaluatorBean.getName();
             if (StringUtils.isEmpty(functionName)) {
-                throw new SchemaException(
+                throw new ConfigurationException(
                         "Missing function name in " + shortDebugDump() + " in " + context.getContextDescription());
             }
 
@@ -114,7 +114,7 @@ private ExpressionType getFunctionExpressionBean(ObjectReferenceType functionLib
                 String allFunctionNames = allFunctions.stream()
                         .map(ExpressionType::getName)
                         .collect(Collectors.joining(", "));
-                throw new ObjectNotFoundException("No function with name " + functionName + " found in " + shortDebugDump()
+                throw new ConfigurationException("No function with name " + functionName + " found in " + shortDebugDump()
                         + ". Function defined are: " + allFunctionNames + ". In " + context.getContextDescription());
             }
 
@@ -129,7 +129,7 @@ private ExpressionType getFunctionExpressionBean(ObjectReferenceType functionLib
 
     @NotNull
     private ExpressionType selectFromMatchingFunctions(List<ExpressionType> functionsMatchingName,
-            ExpressionEvaluationContext context) throws ObjectNotFoundException {
+            ExpressionEvaluationContext context) throws ConfigurationException {
 
         if (functionsMatchingName.size() == 1) {
             return functionsMatchingName.iterator().next();
@@ -148,7 +148,7 @@ private ExpressionType selectFromMatchingFunctions(List<ExpressionType> function
             return filteredExpression;
         }
         String functionName = expressionEvaluatorBean.getName();
-        throw new ObjectNotFoundException("No matching function with name " + functionName + " found in " + shortDebugDump()
+        throw new ConfigurationException("No matching function with name " + functionName + " found in " + shortDebugDump()
                 + ". " + functionsMatchingName.size() + " functions with this name found but none matches the parameters. In " +
                 context.getContextDescription());
     }

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelObjectResolver.java

@@ -147,8 +147,12 @@ public <T extends ObjectType> T getObject(
             objectType = object.asObjectable();
             if (!clazz.isInstance(objectType)) {
                 throw new ObjectNotFoundException(
-                        "Bad object type returned for referenced oid '" + oid + "'. Expected '" + clazz + "', but was '"
-                                + objectType.getClass() + "'.");
+                        String.format(
+                                "Bad object type returned for referenced oid '%s'. Expected '%s', but was '%s'.",
+                                oid, clazz, objectType.getClass()),
+                        clazz,
+                        oid,
+                        GetOperationOptions.isAllowNotFound(options));
             }
 
             if (hookRegistry != null) {

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/AuditController.java

@@ -150,7 +150,9 @@ private List<AuditEventRecordType> getChangeTrail(
         AuditEventRecordType finalEvent = findEvent(finalEventIdentifier, result);
         if (finalEvent == null) {
             throw new ObjectNotFoundException(
-                    "Audit event ID " + finalEventIdentifier + " was not found");
+                    "Audit event ID " + finalEventIdentifier + " was not found",
+                    AuditEventRecordType.class,
+                    finalEventIdentifier);
         }
 
         LOGGER.trace("Final event:\n{}", finalEvent.debugDumpLazily(1));

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java

@@ -227,13 +227,7 @@ public <T extends ObjectType> PrismObject<T> getObject(Class<T> clazz, String oi
             throw e;
         } catch (ObjectNotFoundException e) {
             OP_LOGGER.debug("MODEL OP error getObject({},{},{}): {}: {}", clazz.getSimpleName(), oid, rawOptions, e.getClass().getSimpleName(), e.getMessage());
-            if (GetOperationOptions.isAllowNotFound(rootOptions)) {
-                // TODO check if this is really needed (lower layers shouldn't produce FATAL_ERROR if "allow not found" is true)
-                // FIXME there is no "last subresult" if the called method throws this exception because of object type mismatch!
-                result.getLastSubresult().setStatus(OperationResultStatus.HANDLED_ERROR);
-            } else {
-                ModelImplUtils.recordFatalError(result, e);
-            }
+            ModelImplUtils.recordException(result, e);
             throw e;
         } finally {
             result.computeStatusIfUnknown();
@@ -2336,7 +2330,10 @@ private void authorizeNodeCollectionOperation(
             ObjectQuery q = ObjectQueryUtil.createNameQuery(NodeType.class, prismContext, identifier);
             List<PrismObject<NodeType>> nodes = cacheRepositoryService.searchObjects(NodeType.class, q, createReadOnlyCollection(), parentResult);
             if (nodes.isEmpty()) {
-                throw new ObjectNotFoundException("Node with identifier '" + identifier + "' couldn't be found.");
+                throw new ObjectNotFoundException(
+                        "Node with identifier '" + identifier + "' couldn't be found.",
+                        NodeType.class,
+                        identifier);
             } else if (nodes.size() > 1) {
                 throw new SystemException("Multiple nodes with identifier '" + identifier + "'");
             }