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)
Evolveum · Sep 4, 2018 · 2c8bc04 · 2c8bc04
1 parent 616570a
commit 2c8bc04
Show file tree

Hide file tree

Showing 11 changed files with 298 additions and 25 deletions.
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
@@ -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 <IV extends PrismValue, ID extends ItemDefinition> Source<IV,ID> 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<IV,ID> itemOld = null;
 		ItemDelta<IV,ID> delta = null;
 		Item<IV,ID> itemNew = null;

diff --git a/...ommon/src/test/java/com/evolveum/midpoint/model/common/expression/TestExpressionUtil.java b/...ommon/src/test/java/com/evolveum/midpoint/model/common/expression/TestExpressionUtil.java
@@ -206,7 +206,7 @@ private <T> 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
@@ -120,7 +120,7 @@ private static Object variableFromPathExpression(HashMap<String, Object> 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<String, Object> variableMap) {

diff --git a/.../test/java/com/evolveum/midpoint/model/intest/AbstractConfiguredModelIntegrationTest.java b/.../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/...test/java/com/evolveum/midpoint/model/intest/AbstractInitializedModelIntegrationTest.java b/...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
@@ -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<UserType> 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<UserType> 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<UserType> 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<UserType> 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<Containerable> orgAssignmentPcv = new PrismContainerValue<>(prismContext);
+		orgAssignmentPcv.setId(orgAssignment.getId());
+		ObjectDelta<UserType> 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<UserType> 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
@@ -0,0 +1,105 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ Copyright (c) 2010-2018 Evolveum
+  ~
+  ~ Licensed under the Apache License, Version 2.0 (the "License");
+  ~ you may not use this file except in compliance with the License.
+  ~ You may obtain a copy of the License at
+  ~
+  ~     http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing, software
+  ~ distributed under the License is distributed on an "AS IS" BASIS,
+  ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+<objectTemplate oid="8e47c2b2-dde6-44a9-a7c0-de21a14cb70d"
+				xmlns='http://midpoint.evolveum.com/xml/ns/public/common/common-3'
+				xmlns:q="http://prism.evolveum.com/xml/ns/public/query-3">
+	<name>User template for cartesian assignments</name>
+	<mapping>
+		<strength>strong</strength>
+		<source>
+			<name>x</name>
+			<path>assignment</path>
+			<set>
+				<condition>
+					<script>
+						<code>
+							x != null &amp;&amp; x.targetRef?.type?.localPart == 'RoleType'
+						</code>
+					</script>
+				</condition>
+			</set>
+		</source>
+		<source>
+			<name>y</name>
+			<path>assignment</path>
+			<set>
+				<condition>
+					<script>
+						<code>
+							y != null &amp;&amp; y.targetRef?.type?.localPart == 'OrgType' &amp;&amp; !y.subtype.contains('combined')
+						</code>
+					</script>
+				</condition>
+			</set>
+		</source>
+		<expression>
+			<assignmentTargetSearch>
+				<condition>
+					<script>
+						<code>
+							log.info('condition: x={}, y={}', x?.targetRef?.oid, y?.targetRef?.oid)
+							x != null &amp;&amp; y != null
+						</code>
+					</script>
+				</condition>
+				<targetType>OrgType</targetType>
+				<filter>
+					<q:equal>
+						<q:path>name</q:path>
+						<expression>
+							<script>
+								<code>
+									midpoint.resolveReference(x?.targetRef)?.name + '-' + midpoint.resolveReference(y?.targetRef)?.name
+								</code>
+							</script>
+						</expression>
+					</q:equal>
+				</filter>
+				<assignmentProperties>
+					<subtype>combined</subtype>
+				</assignmentProperties>
+				<createOnDemand>true</createOnDemand>
+				<populateObject>
+					<populateItem>
+						<target>
+							<path>name</path>
+						</target>
+						<expression>
+							<script>
+								<code>
+									midpoint.resolveReference(x?.targetRef)?.name + '-' + midpoint.resolveReference(y?.targetRef)?.name
+								</code>
+							</script>
+						</expression>
+					</populateItem>
+				</populateObject>
+			</assignmentTargetSearch>
+		</expression>
+		<target>
+			<path>assignment</path>
+			<set>
+				<condition>
+					<script>
+						<code>
+							assignment?.subtype.contains('combined')
+						</code>
+					</script>
+				</condition>
+			</set>
+		</target>
+	</mapping>
+</objectTemplate>
diff --git a/repo/repo-common/pom.xml b/repo/repo-common/pom.xml
@@ -95,6 +95,10 @@
 			<groupId>commons-lang</groupId>
 			<artifactId>commons-lang</artifactId>
 		</dependency>
+		<dependency>
+			<groupId>org.apache.commons</groupId>
+			<artifactId>commons-collections4</artifactId>
+		</dependency>
         <dependency>
             <groupId>org.springframework</groupId>
             <artifactId>spring-context</artifactId>

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
@@ -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/...epo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ExpressionUtil.java b/...epo-common/src/main/java/com/evolveum/midpoint/repo/common/expression/ExpressionUtil.java
@@ -170,8 +170,15 @@ public static <I, O> O convertValue(Class<O> finalExpectedJavaType, Function<Obj
 		return convertedVal;
 	}
 
-	public static Object resolvePath(ItemPath path, ExpressionVariables variables, Object defaultContext,
-			ObjectResolver objectResolver, String shortDesc, Task task, OperationResult result)
+	/**
+	 * normalizeValuesToDelete: Whether to normalize container values that are to be deleted, i.e. convert them
+	 * from id-only to full data (MID-4863).
+	 * TODO:
+	 * 1. consider setting this parameter to true at some other places where it might be relevant
+	 * 2. consider normalizing delete deltas earlier in the clockwork, probably at the very beginning of the operation
+	 */
+	public static Object resolvePath(ItemPath path, ExpressionVariables variables, boolean normalizeValuesToDelete,
+			Object defaultContext, ObjectResolver objectResolver, String shortDesc, Task task, OperationResult result)
 					throws SchemaException, ObjectNotFoundException, CommunicationException, ConfigurationException, SecurityViolationException, ExpressionEvaluationException {
 
 		Object root = defaultContext;
@@ -195,6 +202,10 @@ public static Object resolvePath(ItemPath path, ExpressionVariables variables, O
 			return root;
 		}
 
+		if (normalizeValuesToDelete) {
+			root = normalizeValuesToDelete(root);
+		}
+
 		if (root instanceof ObjectReferenceType) {
 			root = resolveReference((ObjectReferenceType) root, objectResolver, varDesc, shortDesc, task,
 					result);
@@ -222,7 +233,18 @@ public static Object resolvePath(ItemPath path, ExpressionVariables variables, O
 					"Unexpected root " + root + " (relative path:" + relativePath + ") in " + shortDesc);
 		}
 	}
-
+
+	private static Object normalizeValuesToDelete(Object root) {
+		if (root instanceof ObjectDeltaObject<?>) {
+			return ((ObjectDeltaObject<?>) root).normalizeValuesToDelete(true);
+		} else if (root instanceof ItemDeltaItem<?, ?>) {
+			// TODO normalize as well
+			return root;
+		} else {
+			return root;
+		}
+	}
+
 	public static <V extends PrismValue, F extends FocusType> Collection<V> 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 <V extends PrismValue, F extends FocusType> Collection<V> 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) {