diff --git a/model/model-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignment.java b/model/model-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignment.java
index 3857d686309..4f0c2d42d29 100644
--- a/model/model-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignment.java
+++ b/model/model-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignment.java
@@ -94,6 +94,11 @@ public interface EvaluatedAssignment extends De
@NotNull
Collection 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 getPolicySituations();
diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentEvaluator.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentEvaluator.java
index 720797d9962..52b3b6e8832 100644
--- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentEvaluator.java
+++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentEvaluator.java
@@ -257,6 +257,8 @@ public EvaluatedAssignmentImpl 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
diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentTargetEvaluationInformation.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentTargetEvaluationInformation.java
new file mode 100644
index 00000000000..7a188f2aada
--- /dev/null
+++ b/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;
+ }
+}
diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentImpl.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentImpl.java
index 65a0a05f905..ab0be9c9ff5 100644
--- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentImpl.java
+++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentImpl.java
@@ -440,6 +440,11 @@ public Collection 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) {
diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentTargetCache.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentTargetCache.java
index 7c9788f597c..f2e30594969 100644
--- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentTargetCache.java
+++ b/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> processedKeys;
+ private DeltaTriple> processedKeys;
// Triple. Target processed for addition is not necessarily reusable for deletion
// This is indexed by OID, relation and order
- private DeltaTriple> processedOrderKeys;
+ private DeltaTriple> 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 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));
}
}
diff --git a/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/assignments/TestAssignmentsAdvanced.java b/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/assignments/TestAssignmentsAdvanced.java
index 5bd7d88ed76..8d7941150ef 100644
--- a/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/assignments/TestAssignmentsAdvanced.java
+++ b/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();