Add per-assignment cleanup of target eval cache

Here we introduce per-assignment selective cleanup of assignment
target evaluation cache. It is needed because some of the assignments
can bring target-related policy rules. Although such assignment targets
can be idempotent, their idempotency is limited to a single assignment
evaluation. See also MID-5827.

infra/schema/src/main/resources/xml/ns/public/common/common-core-3.xsd

@@ -12433,9 +12433,9 @@
                             <xsd:documentation>
                                 <p>
                                     <p>
-                                        This value indicates, whether the evaluation of this role gives the
+                                        This value indicates whether the evaluation of this role gives the
                                         same results regardless of its position in the assignment/inducement
-                                        hierarchy. I.e. evaluation of this roles does not depend on the assignment
+                                        hierarchy. I.e. evaluation of this role does not depend on the assignment
                                         parameters of focus or any of the preceding roles. This flag is used
                                         to enable aggressive caching of role evaluation, so idempotent roles
                                         are only evaluated once regardless of their position in the hierarchy
@@ -12465,15 +12465,15 @@
                                         focus then their outcome does not depend on their position in
                                         assignment/inducement hierarchy and these roles can be made idempotent.
                                     </p>
-                                </p>
-                            </xsd:documentation>
-                             <xsd:appinfo>
-                                <a:displayName>AbstractRoleType.idempotent</a:displayName>
-                                <a:since>3.6</a:since>
-                            </xsd:appinfo>
-                        </xsd:annotation>
-                    </xsd:element>
-                    <xsd:element name="riskLevel" type="xsd:string" minOccurs="0">
+								</p>
+							</xsd:documentation>
+							<xsd:appinfo>
+								<a:displayName>AbstractRoleType.idempotent</a:displayName>
+								<a:since>3.6</a:since>
+							</xsd:appinfo>
+						</xsd:annotation>
+					</xsd:element>
+					<xsd:element name="riskLevel" type="xsd:string" minOccurs="0">
                         <xsd:annotation>
                             <xsd:documentation>
                                 <p>

model/model-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignment.java

@@ -94,6 +94,11 @@ public interface EvaluatedAssignment<AH extends AssignmentHolderType> extends De
     @NotNull
     Collection<EvaluatedPolicyRule> getAllTargetsPolicyRules();
 
+    /**
+     * How many target policy rules are there. This is more efficient than getAllTargetsPolicyRules().size(), as the
+     * collection of all targets policy rules is computed on demand.
+     */
+    int getAllTargetsPolicyRulesCount();
 
     Collection<String> getPolicySituations();
 

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentEvaluator.java

@@ -257,6 +257,8 @@ public EvaluatedAssignmentImpl<AH> evaluate(
                     new AssignmentPathImpl(prismContext),
                     primaryAssignmentMode, evaluateOld, task);
 
+            evaluatedAssignmentTargetCache.resetForNextAssignment();
+
             AssignmentPathSegmentImpl segment = new AssignmentPathSegmentImpl(source, sourceDescription, assignmentIdi, true,
                     evaluateOld, relationRegistry, prismContext);
             segment.setEvaluationOrder(getInitialEvaluationOrder(assignmentIdi, ctx));
@@ -816,6 +818,7 @@ private ExpressionVariables getAssignmentEvaluationVariables() {
         return variables;
     }
 
+    // Note: This method must be single-return after targetEvaluationInformation is established.
     private void evaluateSegmentTarget(AssignmentPathSegmentImpl segment, PlusMinusZero relativeMode, boolean isValid,
             AssignmentHolderType targetType, QName relation, EvaluationContext ctx,
             OperationResult result)
@@ -846,11 +849,21 @@ private void evaluateSegmentTarget(AssignmentPathSegmentImpl segment, PlusMinusZ
         LOGGER.debug("Evaluating RBAC [{}]", ctx.assignmentPath.shortDumpLazily());
         InternalMonitor.recordRoleEvaluation(targetType, true);
 
+        AssignmentTargetEvaluationInformation targetEvaluationInformation;
         if (isValid) {
             // Cache it immediately, even before evaluation. So if there is a cycle in the role path
             // then we can detect it and skip re-evaluation of aggressively idempotent roles.
-            evaluatedAssignmentTargetCache.recordProcessing(segment, ctx.primaryAssignmentMode);
+            //
+            // !!! Ensure we will not return without updating this object (except for exceptions). So please keep this
+            //     method a single-return one after this point. We did not want to complicate things using try...finally.
+            //
+            targetEvaluationInformation = evaluatedAssignmentTargetCache.recordProcessing(segment, ctx.primaryAssignmentMode);
+        } else {
+            targetEvaluationInformation = null;
         }
+        int targetPolicyRulesOnEntry = ctx.evalAssignment.getAllTargetsPolicyRulesCount();
+
+        boolean skipOnConditionResult = false;
 
         if (isTargetValid && targetType instanceof AbstractRoleType) {
             MappingType roleCondition = ((AbstractRoleType)targetType).getCondition();
@@ -862,71 +875,78 @@ private void evaluateSegmentTarget(AssignmentPathSegmentImpl segment, PlusMinusZ
                 boolean condOld = ExpressionUtil.computeConditionResult(conditionTriple.getNonPositiveValues());
                 boolean condNew = ExpressionUtil.computeConditionResult(conditionTriple.getNonNegativeValues());
                 PlusMinusZero modeFromCondition = ExpressionUtil.computeConditionResultMode(condOld, condNew);
-                if (modeFromCondition == null) {        // removed "|| (condMode == PlusMinusZero.ZERO && !condNew)" because it's always false
+                if (modeFromCondition == null) {
+                    skipOnConditionResult = true;
                     LOGGER.trace("Skipping evaluation of {} because of condition result ({} -> {}: null)",
                             targetType, condOld, condNew);
-                    return;
+                } else {
+                    PlusMinusZero origMode = relativeMode;
+                    relativeMode = PlusMinusZero.compute(relativeMode, modeFromCondition);
+                    LOGGER.trace("Evaluated condition in {}: {} -> {}: {} + {} = {}", targetType, condOld, condNew,
+                            origMode, modeFromCondition, relativeMode);
                 }
-                PlusMinusZero origMode = relativeMode;
-                relativeMode = PlusMinusZero.compute(relativeMode, modeFromCondition);
-                LOGGER.trace("Evaluated condition in {}: {} -> {}: {} + {} = {}", targetType, condOld, condNew,
-                        origMode, modeFromCondition, relativeMode);
             }
         }
 
-        EvaluatedAssignmentTargetImpl evalAssignmentTarget = new EvaluatedAssignmentTargetImpl(
-                targetType.asPrismObject(),
-                segment.isMatchingOrder(),    // evaluateConstructions: exact meaning of this is to be revised
-                ctx.assignmentPath.clone(),
-                getAssignmentType(segment, ctx),
-                isValid);
-        ctx.evalAssignment.addRole(evalAssignmentTarget, relativeMode);
-
-        // we need to evaluate assignments also for disabled targets, because of target policy rules
-        // ... but only for direct ones!
-        if (isTargetValid || ctx.assignmentPath.size() == 1) {
-            for (AssignmentType roleAssignment : targetType.getAssignment()) {
-                evaluateAssignment(segment, relativeMode, isValid, ctx, targetType, relation, roleAssignment, result);
+        if (!skipOnConditionResult) {
+            EvaluatedAssignmentTargetImpl evalAssignmentTarget = new EvaluatedAssignmentTargetImpl(
+                    targetType.asPrismObject(),
+                    segment.isMatchingOrder(),    // evaluateConstructions: exact meaning of this is to be revised
+                    ctx.assignmentPath.clone(),
+                    getAssignmentType(segment, ctx),
+                    isValid);
+            ctx.evalAssignment.addRole(evalAssignmentTarget, relativeMode);
+
+            // we need to evaluate assignments also for disabled targets, because of target policy rules
+            // ... but only for direct ones!
+            if (isTargetValid || ctx.assignmentPath.size() == 1) {
+                for (AssignmentType roleAssignment : targetType.getAssignment()) {
+                    evaluateAssignment(segment, relativeMode, isValid, ctx, targetType, relation, roleAssignment, result);
+                }
             }
-        }
 
-        // we need to collect membership also for disabled targets (provided the assignment itself is enabled): MID-4127
-        if ((isNonNegative(relativeMode)) && segment.isProcessMembership()) {
-            collectMembership(targetType, relation, ctx);
-        }
-
-        if (!isTargetValid) {
-            return;
-        }
+            // we need to collect membership also for disabled targets (provided the assignment itself is enabled): MID-4127
+            if ((isNonNegative(relativeMode)) && segment.isProcessMembership()) {
+                collectMembership(targetType, relation, ctx);
+            }
 
-        if ((isNonNegative(relativeMode))) {
-            collectTenantRef(targetType, ctx);
-        }
+            if (isTargetValid) {
+                if ((isNonNegative(relativeMode))) {
+                    collectTenantRef(targetType, ctx);
+                }
 
-        // We continue evaluation even if the relation is non-membership and non-delegation.
-        // Computation of isMatchingOrder will ensure that we won't collect any unwanted content.
+                // We continue evaluation even if the relation is non-membership and non-delegation.
+                // Computation of isMatchingOrder will ensure that we won't collect any unwanted content.
 
-        if (targetType instanceof AbstractRoleType) {
-            for (AssignmentType roleInducement : ((AbstractRoleType)targetType).getInducement()) {
-                evaluateInducement(segment, relativeMode, isValid, ctx, targetType, roleInducement, result);
-            }
-        }
+                if (targetType instanceof AbstractRoleType) {
+                    for (AssignmentType roleInducement : ((AbstractRoleType) targetType).getInducement()) {
+                        evaluateInducement(segment, relativeMode, isValid, ctx, targetType, roleInducement, result);
+                    }
+                }
 
-        //boolean matchesOrder = AssignmentPathSegmentImpl.computeMatchingOrder(segment.getEvaluationOrder(), 1, Collections.emptyList());
-        if (segment.isMatchingOrder() && targetType instanceof AbstractRoleType && isNonNegative(relativeMode)) {
-            for (AuthorizationType authorizationType: ((AbstractRoleType)targetType).getAuthorization()) {
-                Authorization authorization = createAuthorization(authorizationType, targetType.toString());
-                if (!ctx.evalAssignment.getAuthorizations().contains(authorization)) {
-                    ctx.evalAssignment.addAuthorization(authorization);
+                //boolean matchesOrder = AssignmentPathSegmentImpl.computeMatchingOrder(segment.getEvaluationOrder(), 1, Collections.emptyList());
+                if (segment.isMatchingOrder() && targetType instanceof AbstractRoleType && isNonNegative(relativeMode)) {
+                    for (AuthorizationType authorizationType : ((AbstractRoleType) targetType).getAuthorization()) {
+                        Authorization authorization = createAuthorization(authorizationType, targetType.toString());
+                        if (!ctx.evalAssignment.getAuthorizations().contains(authorization)) {
+                            ctx.evalAssignment.addAuthorization(authorization);
+                        }
+                    }
+                    AdminGuiConfigurationType adminGuiConfiguration = ((AbstractRoleType) targetType).getAdminGuiConfiguration();
+                    if (adminGuiConfiguration != null && !ctx.evalAssignment.getAdminGuiConfigurations()
+                            .contains(adminGuiConfiguration)) {
+                        ctx.evalAssignment.addAdminGuiConfiguration(adminGuiConfiguration);
+                    }
                 }
             }
-            AdminGuiConfigurationType adminGuiConfiguration = ((AbstractRoleType) targetType).getAdminGuiConfiguration();
-            if (adminGuiConfiguration != null && !ctx.evalAssignment.getAdminGuiConfigurations().contains(adminGuiConfiguration)) {
-                ctx.evalAssignment.addAdminGuiConfiguration(adminGuiConfiguration);
-            }
         }
+        int targetPolicyRulesOnExit = ctx.evalAssignment.getAllTargetsPolicyRulesCount();
 
-        LOGGER.trace("Evaluating segment target DONE for {}", segment);
+        LOGGER.trace("Evaluating segment target DONE for {}; target policy rules: {} -> {}", segment, targetPolicyRulesOnEntry,
+                targetPolicyRulesOnExit);
+        if (targetEvaluationInformation != null) {
+            targetEvaluationInformation.setBringsTargetPolicyRules(targetPolicyRulesOnExit > targetPolicyRulesOnEntry);
+        }
     }
 
     // TODO revisit this

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentTargetEvaluationInformation.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright (c) 2019 Evolveum and contributors
+ *
+ * This work is dual-licensed under the Apache License 2.0
+ * and European Union Public License. See LICENSE file for details.
+ */
+
+package com.evolveum.midpoint.model.impl.lens;
+
+/**
+ * Information about assignment target evaluation that is stored in the evaluated assignment target cache.
+ */
+
+class AssignmentTargetEvaluationInformation {
+
+    /**
+     * True if the assignment target contributes (provides) some "target" policy rules. This means that we should not
+     * cache its evaluation for the whole focus processing, because the policy rules should be applied separately to all
+     * the evaluated focus assignments.
+     *
+     * Therefore, when starting evaluation of next focus assignment, we should remove all existing cache entries that have
+     * this value set to true.
+     *
+     * See also MID-5827.
+     *
+     * The default of "true" is safer -- it is better to errorneously evict an entry from the cache than to omit some
+     * necessary target policy rules.
+     */
+    private boolean bringsTargetPolicyRules = true;
+
+    void setBringsTargetPolicyRules(boolean bringsTargetPolicyRules) {
+        this.bringsTargetPolicyRules = bringsTargetPolicyRules;
+    }
+
+    /**
+     * @return true if this target evaluation should be cached for single assignment evaluation only.
+     *
+     * Currently we decide only based on whether it brings any target policy rules.
+     */
+    boolean isAssignmentScoped() {
+        return bringsTargetPolicyRules;
+    }
+}

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentImpl.java

@@ -440,6 +440,11 @@ public Collection<EvaluatedPolicyRule> getAllTargetsPolicyRules() {
         return Stream.concat(thisTargetPolicyRules.stream(), otherTargetsPolicyRules.stream()).collect(Collectors.toList());
     }
 
+    @Override
+    public int getAllTargetsPolicyRulesCount() {
+        return thisTargetPolicyRules.size() + otherTargetsPolicyRules.size();
+    }
+
     @NotNull
     private EvaluatedPolicyRule toEvaluatedPolicyRule(PolicyConstraintsType constraints, AssignmentPath assignmentPath,
             AssignmentHolderType directOwner, PrismContext prismContext) {

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentTargetCache.java

@@ -6,11 +6,6 @@
  */
 package com.evolveum.midpoint.model.impl.lens;
 
-import java.util.HashSet;
-import java.util.Set;
-
-import javax.xml.namespace.QName;
-
 import com.evolveum.midpoint.model.api.context.EvaluationOrder;
 import com.evolveum.midpoint.prism.delta.DeltaTriple;
 import com.evolveum.midpoint.prism.delta.PlusMinusZero;
@@ -22,6 +17,10 @@
 import com.evolveum.midpoint.xml.ns._public.common.common_3.IdempotenceType;
 import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectType;
 
+import javax.xml.namespace.QName;
+import java.util.HashMap;
+import java.util.Map;
+
 /**
  * @author semancik
  *
@@ -32,36 +31,53 @@ public class EvaluatedAssignmentTargetCache implements DebugDumpable {
 
     // Triple. Target processed for addition is not necessarily reusable for deletion
     // This is indexed by OID and relation only
-    private DeltaTriple<Set<Key>> processedKeys;
+    private DeltaTriple<Map<Key, AssignmentTargetEvaluationInformation>> processedKeys;
 
     // Triple. Target processed for addition is not necessarily reusable for deletion
     // This is indexed by OID, relation and order
-    private DeltaTriple<Set<OrderKey>> processedOrderKeys;
+    private DeltaTriple<Map<OrderKey, AssignmentTargetEvaluationInformation>> processedOrderKeys;
 
 
-    public EvaluatedAssignmentTargetCache() {
-        processedOrderKeys = new DeltaTriple<>(() -> new HashSet<>());
-        processedKeys = new DeltaTriple<>(() -> new HashSet<>());
+    EvaluatedAssignmentTargetCache() {
+        processedOrderKeys = new DeltaTriple<>(HashMap::new);
+        processedKeys = new DeltaTriple<>(HashMap::new);
     }
 
     public void reset() {
-        processedOrderKeys.foreach(set -> set.clear());
-        processedKeys.foreach(set -> set.clear());
+        processedOrderKeys.foreach(Map::clear);
+        processedKeys.foreach(Map::clear);
     }
 
-    public void recordProcessing(AssignmentPathSegmentImpl segment, PlusMinusZero mode) {
+    void resetForNextAssignment() {
+        processedOrderKeys.foreach(map -> removeAssignmentScoped(map, "conservative assignment target evaluation cache"));
+        processedKeys.foreach(map -> removeAssignmentScoped(map, "aggressive assignment target evaluation cache"));
+    }
+
+    private void removeAssignmentScoped(Map<? extends Key, AssignmentTargetEvaluationInformation> map, String desc) {
+        int before = map.size();
+        map.entrySet().removeIf(e -> e.getValue().isAssignmentScoped());
+        int after = map.size();
+        if (after < before) {
+            LOGGER.trace("Removed {} entries from {}", before - after, desc);
+        }
+    }
+
+    AssignmentTargetEvaluationInformation recordProcessing(AssignmentPathSegmentImpl segment, PlusMinusZero mode) {
         ObjectType targetType = segment.getTarget();
         if (!(targetType instanceof AbstractRoleType)) {
-            return;
+            return null;
         }
         if (!isCacheable((AbstractRoleType)targetType)) {
-            return;
+            return null;
         }
-        processedOrderKeys.get(mode).add(new OrderKey(segment));
+        AssignmentTargetEvaluationInformation targetEvaluationInformation = new AssignmentTargetEvaluationInformation();
 
+        processedOrderKeys.get(mode).put(new OrderKey(segment), targetEvaluationInformation);
         if (targetType.getOid() != null) {
-            processedKeys.get(mode).add(new Key(segment));
+            processedKeys.get(mode).put(new Key(segment), targetEvaluationInformation);
         }
+
+        return targetEvaluationInformation;
     }
 
     private boolean isCacheable(AbstractRoleType targetType) {
@@ -97,9 +113,9 @@ public boolean canSkip(AssignmentPathSegmentImpl segment, PlusMinusZero mode) {
                 LOGGER.trace("Aggressive idempotent and non-one order: {}: {}", segment.getEvaluationOrder(), target);
                 return true;
             }
-            return processedKeys.get(mode).contains(new Key(segment));
+            return processedKeys.get(mode).containsKey(new Key(segment));
         } else {
-            return processedOrderKeys.get(mode).contains(new OrderKey(segment));
+            return processedOrderKeys.get(mode).containsKey(new OrderKey(segment));
         }
     }
 

model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/assignments/TestAssignmentsAdvanced.java

@@ -1000,7 +1000,7 @@ public void test820UnassignRole27() throws Exception {
         assertNotAssignedRole(jack, roleRole27Oid);
     }
 
-    @Test(enabled = false)      // MID-5827
+    @Test      // MID-5827
     public void test900AssignIdempotentRole() throws Exception {
         login(userAdministrator);
         Task task = getTask();