From 2c8bc04d5b72ca104d715dbc5cb48d08411c4a97 Mon Sep 17 00:00:00 2001 From: Pavol Mederly Date: Tue, 4 Sep 2018 10:02:44 +0200 Subject: [PATCH] Fix id-only ODOs in expression sources (MID-4863) When e.g. assignment used as expression source and a value of the assignment is being deleted, the ODO contains an id-only value. This commit conservatively fixes (normalizes) such values just before expression evaluation. TODO: consider more ambitious approach and normalize these values e.g. before the delta execution in model (also a test for MID-4862 is added here) --- .../model/common/mapping/MappingImpl.java | 4 +- .../common/expression/TestExpressionUtil.java | 2 +- .../model/impl/scripting/VariablesUtil.java | 2 +- ...bstractConfiguredModelIntegrationTest.java | 3 + ...stractInitializedModelIntegrationTest.java | 1 + .../model/intest/mapping/TestMapping.java | 109 ++++++++++++++++-- .../common/user-template-carthesian.xml | 105 +++++++++++++++++ repo/repo-common/pom.xml | 4 + .../repo/common/expression/Expression.java | 2 +- .../common/expression/ExpressionUtil.java | 30 ++++- .../common/expression/ObjectDeltaObject.java | 61 ++++++++-- 11 files changed, 298 insertions(+), 25 deletions(-) create mode 100644 model/model-intest/src/test/resources/common/user-template-carthesian.xml diff --git a/model/model-common/src/main/java/com/evolveum/midpoint/model/common/mapping/MappingImpl.java b/model/model-common/src/main/java/com/evolveum/midpoint/model/common/mapping/MappingImpl.java index aa71cce90d3..70fef8cfca2 100644 --- a/model/model-common/src/main/java/com/evolveum/midpoint/model/common/mapping/MappingImpl.java +++ b/model/model-common/src/main/java/com/evolveum/midpoint/model/common/mapping/MappingImpl.java @@ -815,7 +815,7 @@ private XMLGregorianCalendar parseTimeSource(VariableBindingDefinitionType sourc throw new SchemaException("Empty source path in "+getMappingContextDescription()); } - Object sourceObject = ExpressionUtil.resolvePath(path, variables, sourceContext, objectResolver, "reference time definition in "+getMappingContextDescription(), task, result); + Object sourceObject = ExpressionUtil.resolvePath(path, variables, false, sourceContext, objectResolver, "reference time definition in "+getMappingContextDescription(), task, result); if (sourceObject == null) { return null; } @@ -871,7 +871,7 @@ private Source parseSo name = ItemPath.getName(path.last()); } ItemPath resolvePath = path; - Object sourceObject = ExpressionUtil.resolvePath(path, variables, sourceContext, objectResolver, "source definition in "+getMappingContextDescription(), task, result); + Object sourceObject = ExpressionUtil.resolvePath(path, variables, true, sourceContext, objectResolver, "source definition in "+getMappingContextDescription(), task, result); Item itemOld = null; ItemDelta delta = null; Item itemNew = null; diff --git a/model/model-common/src/test/java/com/evolveum/midpoint/model/common/expression/TestExpressionUtil.java b/model/model-common/src/test/java/com/evolveum/midpoint/model/common/expression/TestExpressionUtil.java index c27cdc5c5ac..53d32f7cee0 100644 --- a/model/model-common/src/test/java/com/evolveum/midpoint/model/common/expression/TestExpressionUtil.java +++ b/model/model-common/src/test/java/com/evolveum/midpoint/model/common/expression/TestExpressionUtil.java @@ -206,7 +206,7 @@ private T resolvePath(String path, ExpressionVariables variables, final Stri ItemPath itemPath = toItemPath(path); // WHEN - Object resolved = ExpressionUtil.resolvePath(itemPath, variables, null, null, TEST_NAME, null, result); + Object resolved = ExpressionUtil.resolvePath(itemPath, variables, false, null, null, TEST_NAME, null, result); // THEN System.out.println("Resolved:"); diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java index 62c297fdc41..9993b18b6f2 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java @@ -120,7 +120,7 @@ private static Object variableFromPathExpression(HashMap resulti throw new IllegalArgumentException("Path expression: expected ItemPathType but got " + expressionEvaluator.getValue()); } ItemPath itemPath = ((ItemPathType) expressionEvaluator.getValue()).getItemPath(); - return ExpressionUtil.resolvePath(itemPath, createVariables(resultingVariables), null, ctx.objectResolver, shortDesc, ctx.task, result); + return ExpressionUtil.resolvePath(itemPath, createVariables(resultingVariables), false, null, ctx.objectResolver, shortDesc, ctx.task, result); } private static ExpressionVariables createVariables(HashMap variableMap) { diff --git a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractConfiguredModelIntegrationTest.java b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractConfiguredModelIntegrationTest.java index 3ddd13b7dbd..13bb1d23e5a 100644 --- a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractConfiguredModelIntegrationTest.java +++ b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractConfiguredModelIntegrationTest.java @@ -102,6 +102,9 @@ public class AbstractConfiguredModelIntegrationTest extends AbstractModelIntegra protected static final String USER_TEMPLATE_ORG_ASSIGNMENT_FILENAME = COMMON_DIR + "/user-template-org-assignment.xml"; protected static final String USER_TEMPLATE_ORG_ASSIGNMENT_OID = "10000000-0000-0000-0000-000000000444"; + protected static final String USER_TEMPLATE_CARTHESIAN_FILENAME = COMMON_DIR + "/user-template-carthesian.xml"; + protected static final String USER_TEMPLATE_CARTHESIAN_OID = "8e47c2b2-dde6-44a9-a7c0-de21a14cb70d"; + protected static final File OBJECT_TEMPLATE_PERSONA_ADMIN_FILE = new File(COMMON_DIR, "object-template-persona-admin.xml"); protected static final String OBJECT_TEMPLATE_PERSONA_ADMIN_OID = "894ea1a8-2c0a-11e7-a950-ff2047b0c053"; diff --git a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractInitializedModelIntegrationTest.java b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractInitializedModelIntegrationTest.java index 821b8ea60a9..8508ba33fa2 100644 --- a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractInitializedModelIntegrationTest.java +++ b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/AbstractInitializedModelIntegrationTest.java @@ -216,6 +216,7 @@ public void initSystem(Task initTask, OperationResult initResult) throws Excepti repoAddObjectFromFile(USER_TEMPLATE_INBOUNDS_FILENAME, initResult); repoAddObjectFromFile(USER_TEMPLATE_COMPLEX_INCLUDE_FILENAME, initResult); repoAddObjectFromFile(USER_TEMPLATE_ORG_ASSIGNMENT_FILENAME, initResult); + repoAddObjectFromFile(USER_TEMPLATE_CARTHESIAN_FILENAME, initResult); // Shadows repoAddObjectFromFile(ACCOUNT_SHADOW_GUYBRUSH_DUMMY_FILE, initResult); diff --git a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/mapping/TestMapping.java b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/mapping/TestMapping.java index 558d1a8530b..63bbce02650 100644 --- a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/mapping/TestMapping.java +++ b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/mapping/TestMapping.java @@ -24,6 +24,11 @@ import javax.xml.datatype.XMLGregorianCalendar; +import com.evolveum.midpoint.prism.Containerable; +import com.evolveum.midpoint.prism.PrismContainerValue; +import com.evolveum.midpoint.prism.delta.builder.DeltaBuilder; +import com.evolveum.midpoint.schema.constants.SchemaConstants; +import com.evolveum.midpoint.xml.ns._public.common.common_3.*; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.annotation.DirtiesContext.ClassMode; import org.springframework.test.context.ContextConfiguration; @@ -60,14 +65,6 @@ import com.evolveum.midpoint.util.exception.PolicyViolationException; import com.evolveum.midpoint.util.exception.SchemaException; import com.evolveum.midpoint.util.exception.SecurityViolationException; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ActivationStatusType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ActivationType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.AssignmentPolicyEnforcementType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.AssignmentType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.FocusType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ShadowType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.UserType; /** * @author semancik @@ -133,8 +130,9 @@ public class TestMapping extends AbstractMappingTest { protected static final String DRINK_GRAPPA = "grappa"; protected static final String DRINK_GIN = "gin"; protected static final String DRINK_MEZCAL = "mezcal"; - + private static final String USER_JIM_NAME = "jim"; + private static final String USER_TYPE_CARTHESIAN = "carthesian"; @Override public void initSystem(Task initTask, OperationResult initResult) throws Exception { @@ -155,6 +153,8 @@ public void initSystem(Task initTask, OperationResult initResult) throws Excepti repoAddObjectFromFile(ROLE_COBALT_NEVERLAND_FILE, initResult); assumeAssignmentPolicy(AssignmentPolicyEnforcementType.FULL); + + setDefaultObjectTemplate(UserType.COMPLEX_TYPE, USER_TYPE_CARTHESIAN, USER_TEMPLATE_CARTHESIAN_OID, initResult); } /** @@ -3441,6 +3441,97 @@ public void test427UnassignAccountFromJack() throws Exception { assertLinks(userAfter, 0); } + /** + * MID-4862 + */ + @Test + public void test500AssignmentsCombinationSingle() throws Exception { + final String TEST_NAME = "test500AssignmentsCombinationSingle"; + displayTestTitle(TEST_NAME); + + // GIVEN + Task task = createTask(TEST_NAME); + OperationResult result = task.getResult(); + + UserType jim = prismContext.createKnownObjectable(UserType.class) + .name(USER_JIM_NAME) + .subtype(USER_TYPE_CARTHESIAN) + .beginAssignment() + .targetRef(ROLE_SUPERUSER_OID, RoleType.COMPLEX_TYPE) + .end(); + + // WHEN + displayWhen(TEST_NAME); + addObject(jim.asPrismObject()); + + // THEN + displayThen(TEST_NAME); + + PrismObject userAfter = getUser(jim.getOid()); + display("User after", userAfter); + assertAssignments(userAfter, 1); + } + + /** + * MID-4862 + */ + @Test + public void test510AssignmentsCombinationCouple() throws Exception { + final String TEST_NAME = "test500AssignmentsCombinationCouple"; + displayTestTitle(TEST_NAME); + + // GIVEN + Task task = createTask(TEST_NAME); + OperationResult result = task.getResult(); + + PrismObject jim = findUserByUsername(USER_JIM_NAME); + + // WHEN + displayWhen(TEST_NAME); + assignOrg(jim.getOid(), ORG_SAVE_ELAINE_OID, task, result); + + // THEN + displayThen(TEST_NAME); + assertSuccess(result); + + PrismObject userAfter = getUser(jim.getOid()); + display("User after", userAfter); + assertAssignments(userAfter, 3); + } + + /** + * MID-4863 + */ + @Test + public void test520DeleteUserAssignment() throws Exception { + final String TEST_NAME = "test520DeleteUserAssignment"; + displayTestTitle(TEST_NAME); + + // GIVEN + Task task = createTask(TEST_NAME); + OperationResult result = task.getResult(); + + PrismObject jim = findUserByUsername(USER_JIM_NAME); + + // WHEN + displayWhen(TEST_NAME); + AssignmentType orgAssignment = findAssignment(jim, ORG_SAVE_ELAINE_OID, SchemaConstants.ORG_DEFAULT); + assertNotNull("org assignment not found", orgAssignment); + PrismContainerValue orgAssignmentPcv = new PrismContainerValue<>(prismContext); + orgAssignmentPcv.setId(orgAssignment.getId()); + ObjectDelta delta = DeltaBuilder.deltaFor(UserType.class, prismContext) + .item(UserType.F_ASSIGNMENT).delete(orgAssignmentPcv) + .asObjectDeltaCast(jim.getOid()); + executeChanges(delta, null, task, result); + + // THEN + displayThen(TEST_NAME); + assertSuccess(result); + + PrismObject userAfter = getUser(jim.getOid()); + display("User after", userAfter); + assertAssignments(userAfter, 1); + } private String rumFrom(String locality) { return "rum from " + locality; diff --git a/model/model-intest/src/test/resources/common/user-template-carthesian.xml b/model/model-intest/src/test/resources/common/user-template-carthesian.xml new file mode 100644 index 00000000000..efdc9e774fc --- /dev/null +++ b/model/model-intest/src/test/resources/common/user-template-carthesian.xml @@ -0,0 +1,105 @@ + + + + User template for cartesian assignments + + strong + + x + assignment + + + + + + + + y + assignment + + + + + + + + + + + + OrgType + + + name + + + + + + + combined + + true + + + + name + + + + + + + + + + assignment + + + + + + + + diff --git a/repo/repo-common/pom.xml b/repo/repo-common/pom.xml index c48721386d5..b9b599e3e31 100644 --- a/repo/repo-common/pom.xml +++ b/repo/repo-common/pom.xml @@ -95,6 +95,10 @@ commons-lang commons-lang + + org.apache.commons + commons-collections4 + org.springframework spring-context diff --git a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/Expression.java b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/Expression.java index 06fc7d8de94..c1b7a0eaddf 100644 --- a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/Expression.java +++ b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/Expression.java @@ -347,7 +347,7 @@ private ExpressionVariables processInnerVariables(ExpressionVariables variables, } } else if (variableDefType.getPath() != null) { ItemPath itemPath = variableDefType.getPath().getItemPath(); - Object resolvedValue = ExpressionUtil.resolvePath(itemPath, variables, null, objectResolver, contextDescription, task, result); + Object resolvedValue = ExpressionUtil.resolvePath(itemPath, variables, false, null, objectResolver, contextDescription, task, result); newVariables.addVariableDefinition(varName, resolvedValue); } else { throw new SchemaException("No value for variable "+varName+" in "+contextDescription); diff --git a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ExpressionUtil.java b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ExpressionUtil.java index c651c74ec0d..b6f69789b0f 100644 --- a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ExpressionUtil.java +++ b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ExpressionUtil.java @@ -170,8 +170,15 @@ public static O convertValue(Class finalExpectedJavaType, Function) { + return ((ObjectDeltaObject) root).normalizeValuesToDelete(true); + } else if (root instanceof ItemDeltaItem) { + // TODO normalize as well + return root; + } else { + return root; + } + } + public static Collection computeTargetValues(VariableBindingDefinitionType target, Object defaultTargetContext, ExpressionVariables variables, ObjectResolver objectResolver, String contextDesc, Task task, OperationResult result) throws SchemaException, ObjectNotFoundException, CommunicationException, ConfigurationException, SecurityViolationException, ExpressionEvaluationException { @@ -238,7 +260,7 @@ public static Collection computeT } ItemPath path = itemPathType.getItemPath(); - Object object = resolvePath(path, variables, defaultTargetContext, objectResolver, contextDesc, task, result); + Object object = resolvePath(path, variables, false, defaultTargetContext, objectResolver, contextDesc, task, result); if (object == null) { return new ArrayList<>(); } else if (object instanceof Item) { diff --git a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ObjectDeltaObject.java b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ObjectDeltaObject.java index 762853c515a..03a941091a6 100644 --- a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ObjectDeltaObject.java +++ b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ObjectDeltaObject.java @@ -17,14 +17,9 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.List; -import com.evolveum.midpoint.prism.Item; -import com.evolveum.midpoint.prism.ItemDefinition; -import com.evolveum.midpoint.prism.PartiallyResolvedItem; -import com.evolveum.midpoint.prism.PrismContainerValue; -import com.evolveum.midpoint.prism.PrismObject; -import com.evolveum.midpoint.prism.PrismObjectDefinition; -import com.evolveum.midpoint.prism.PrismValue; +import com.evolveum.midpoint.prism.*; import com.evolveum.midpoint.prism.delta.ChangeType; import com.evolveum.midpoint.prism.delta.ItemDelta; import com.evolveum.midpoint.prism.delta.ObjectDelta; @@ -36,6 +31,8 @@ import com.evolveum.midpoint.util.exception.SchemaException; import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectType; +import static org.apache.commons.collections4.CollectionUtils.emptyIfNull; + /** * A DTO class defining old object state (before change), delta (change) and new object state (after change). * @@ -306,4 +303,54 @@ public ObjectDeltaObject clone() { return clone; } + public ObjectDeltaObject normalizeValuesToDelete(boolean doClone) { + if (delta == null || delta.getChangeType() != ChangeType.MODIFY) { + return this; + } + boolean foundIdOnlyDeletion = false; + main: for (ItemDelta itemDelta : delta.getModifications()) { + for (PrismValue valueToDelete : emptyIfNull(itemDelta.getValuesToDelete())) { + if (valueToDelete instanceof PrismContainerValue && ((PrismContainerValue) valueToDelete).isIdOnly()) { + foundIdOnlyDeletion = true; + break main; + } + } + } + if (!foundIdOnlyDeletion) { + return this; + } + ObjectDeltaObject object = doClone ? this.clone() : this; + + boolean anyRealChange = false; + for (ItemDelta itemDelta : object.delta.getModifications()) { + if (itemDelta.getValuesToDelete() == null) { + continue; + } + boolean itemDeltaChanged = false; + List newValuesToDelete = new ArrayList<>(); + for (PrismValue valueToDelete : itemDelta.getValuesToDelete()) { + if (valueToDelete instanceof PrismContainerValue && ((PrismContainerValue) valueToDelete).isIdOnly() + && object.oldObject != null /* should always be */) { + Object oldItem = object.oldObject.find(itemDelta.getPath()); + if (oldItem instanceof PrismContainer) { + PrismContainerValue oldValue = ((PrismContainer) oldItem) + .getValue(((PrismContainerValue) valueToDelete).getId()); + if (oldValue != null) { + newValuesToDelete.add(oldValue.clone()); + itemDeltaChanged = true; + continue; + } + } + } + newValuesToDelete.add(valueToDelete); + } + if (itemDeltaChanged) { + itemDelta.resetValuesToDelete(); + //noinspection unchecked + ((ItemDelta) itemDelta).addValuesToDelete(newValuesToDelete); + anyRealChange = true; + } + } + return anyRealChange ? object : this; + } }