Stop visiting embedded objects by default

Visitor in prism module did NOT visit objects embedded in references for a very long time. Recently we changed this behavior and started visiting embedded objects - in order to assure encryption of protected values in such objects (see 60328c4). But this change broke other things, like resolution of references in ApprovalSchemaExecutionInformationType. Analysis of other uses indicate that visiting embedded objects can be wrong in other contexts as well. So now we are reverting to the old behavior and providing ConfigurableVisitor to allow controlling this feature. This resolves MID-6171.
Evolveum · Apr 6, 2020 · f9debb2 · f9debb2
1 parent 6da94cd
commit f9debb2
Show file tree

Hide file tree

Showing 10 changed files with 70 additions and 17 deletions.
diff --git a/infra/common/src/main/java/com/evolveum/midpoint/common/crypto/CryptoUtil.java b/infra/common/src/main/java/com/evolveum/midpoint/common/crypto/CryptoUtil.java
@@ -235,7 +235,7 @@ interface ProtectedStringProcessor {
         void apply(ProtectedStringType protectedString, String propertyName) throws EncryptionException;
     }
 
-    private static class CombinedVisitor implements Visitor, JaxbVisitor {
+    private static class CombinedVisitor implements ConfigurableVisitor, JaxbVisitor {
 
         private ProtectedStringProcessor processor;
         private String lastPropName = "?";
@@ -270,6 +270,11 @@ public void visit(Visitable visitable) {
                 }
             }
         }
+
+        @Override
+        public boolean shouldVisitEmbeddedObjects() {
+            return true; // Needed to encrypt secrets in embedded objects. See also 60328c40b2b99c6cf41ab6ce90145fae941d07bd.
+        }
     }
 
     @NotNull

diff --git a/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/ConfigurableVisitor.java b/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/ConfigurableVisitor.java
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2020 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.prism;
+
+import com.evolveum.midpoint.util.annotation.Experimental;
+
+/**
+ * Visitor with configurable behavior.
+ */
+@Experimental
+public interface ConfigurableVisitor<T extends Visitable<T>> extends Visitor<T> {
+
+    /**
+     * Should we visit also objects that are embedded in references?
+     */
+    boolean shouldVisitEmbeddedObjects();
+
+    /**
+     * Helper method.
+     */
+    static boolean shouldVisitEmbeddedObjects(Visitor<?> visitor) {
+        //noinspection SimplifiableIfStatement
+        if (visitor instanceof ConfigurableVisitor) {
+            return ((ConfigurableVisitor<?>) visitor).shouldVisitEmbeddedObjects();
+        } else {
+            return false; // This is the behavior before 60328c40b2b99c6cf41ab6ce90145fae941d07bd (March 24th, 2020)
+        }
+    }
+}
diff --git a/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/util/PrismAsserts.java b/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/util/PrismAsserts.java
@@ -1257,7 +1257,6 @@ public static <O extends Objectable> void assertEquivalent(String message, ItemP
 
     public static void assertImmutable(Definition definition) {
         definition.accept(def -> {
-            System.out.println("Checking " + def);
             if (!def.isImmutable()) {
                 fail("Definition is not immutable: " + def);
             }
@@ -1266,7 +1265,6 @@ public static void assertImmutable(Definition definition) {
 
     public static void assertMutable(Definition definition) {
         definition.accept(def -> {
-            System.out.println("Checking " + def);
             if (def.isImmutable()) {
                 fail("Definition is not mutable: " + def);
             }

diff --git a/infra/prism-impl/src/main/java/com/evolveum/midpoint/prism/impl/PrismReferenceValueImpl.java b/infra/prism-impl/src/main/java/com/evolveum/midpoint/prism/impl/PrismReferenceValueImpl.java
@@ -714,7 +714,7 @@ public void shortDump(StringBuilder sb) {
     @Override
     public void accept(Visitor visitor) {
         super.accept(visitor);
-        if (object != null) {
+        if (object != null && ConfigurableVisitor.shouldVisitEmbeddedObjects(visitor)) {
             //noinspection unchecked
             object.accept(visitor);
         }

diff --git a/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/PrismInternalTestUtil.java b/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/PrismInternalTestUtil.java
@@ -296,7 +296,7 @@ public static void assertUserJack(PrismObject<UserType> user, boolean expectRawI
         user.assertDefinitions("test");
         assertUserJackContent(user, expectRawInConstructions, expectFullPolyName, withIncomplete);
         assertUserJackExtension(user, withIncomplete);
-        assertVisitor(user, 75);
+        assertVisitor(user, 71);
 
         assertPathVisitor(user, UserType.F_ASSIGNMENT, true, 9);
         assertPathVisitor(user, ItemPath.create(UserType.F_ASSIGNMENT, USER_ASSIGNMENT_1_ID), true, 3);

diff --git a/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/TestPrismParsing.java b/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/TestPrismParsing.java
@@ -390,7 +390,7 @@ protected void assertUserAdhoc(PrismObject<UserType> user, boolean expectRawInCo
         user.checkConsistence();
         assertUserJackContent(user, expectRawInConstructions, true, withIncomplete);
         assertUserExtensionAdhoc(user);
-        assertVisitor(user, 62);
+        assertVisitor(user, 58);
     }
 
     private void assertUserExtensionAdhoc(PrismObject<UserType> user) {

diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelObjectResolver.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelObjectResolver.java
@@ -17,6 +17,7 @@
 import com.evolveum.midpoint.schema.SearchResultList;
 import com.evolveum.midpoint.task.api.TaskManager;
 
+import com.evolveum.midpoint.util.annotation.Experimental;
 import com.evolveum.midpoint.wf.api.WorkflowManager;
 import com.evolveum.midpoint.xml.ns._public.common.common_3.*;
 
@@ -325,13 +326,30 @@ public <R,O extends ObjectType> R searchOrgTreeWidthFirst(PrismObject<O> object,
         return null;
     }
 
+    @Experimental
     @Override
-    public void resolveAllReferences(Collection<PrismContainerValue> pcvs, Object taskObject, OperationResult result) {
+    public void resolveAllReferences(Collection<PrismContainerValue<?>> pcvs, Object taskObject, OperationResult result) {
         Session session = openResolutionSession(null);
         Task task = (Task) taskObject;
-        Visitor visitor = (o) -> {
-            if (o instanceof PrismReferenceValue) {
-                resolveReference((PrismReferenceValue) o, "resolving object reference", session, task, result);
+        ConfigurableVisitor<?> visitor = new ConfigurableVisitor() {
+            @Override
+            public boolean shouldVisitEmbeddedObjects() {
+                // This is to avoid endless recursion when resolving cases: A parent case has references to its children
+                // whereas child cases have references to their parent. We could deal with this using SmartVisitable
+                // but that would be overkill for the basic use - it would resolve much more than needed. So we simply stop
+                // visiting embedded objects because in the basic use case we are simply not interested in them.
+                // See also MID-6171.
+                //
+                // Should we need this feature we will add an option or create a separate method
+                // e.g. "resolveAllReferencesDeeply".
+                return false;
+            }
+
+            @Override
+            public void visit(Visitable visitable) {
+                if (visitable instanceof PrismReferenceValue) {
+                    resolveReference((PrismReferenceValue) visitable, "resolving object reference", session, task, result);
+                }
             }
         };
         pcvs.forEach(pcv -> pcv.accept(visitor));

diff --git a/...model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java b/...model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java
@@ -2297,10 +2297,10 @@ private void computePolyStringVisit(Visitable visitable) {
             PolyString polyString = pval.getValue();
             if (polyString.getOrig() == null) {
                 String orig = localizationService.translate(polyString);
-                LOGGER.info("PPPP1: Filling out orig value of polyString {}: {}", polyString, orig);
+                LOGGER.trace("PPPP1: Filling out orig value of polyString {}: {}", polyString, orig);
                 polyString.setComputedOrig(orig);
                 polyString.recompute(prismContext.getDefaultPolyStringNormalizer());
-                LOGGER.info("PPPP2: Resulting polyString: {}", polyString);
+                LOGGER.trace("PPPP2: Resulting polyString: {}", polyString);
             }
         }
     }

diff --git a/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/AbstractWfTestPolicy.java b/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/AbstractWfTestPolicy.java
@@ -17,10 +17,7 @@
 import static com.evolveum.midpoint.xml.ns._public.common.common_3.CaseWorkItemType.F_ORIGINAL_ASSIGNEE_REF;
 
 import java.io.File;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Comparator;
-import java.util.List;
+import java.util.*;
 
 import org.apache.commons.collections4.CollectionUtils;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -466,6 +463,7 @@ protected void afterFirstClockworkRun(CaseType rootCase,
                         if (subcase.getApprovalContext() != null) {
                             OperationResult opResult = new OperationResult("dummy");
                             ApprovalSchemaExecutionInformationType info = workflowManager.getApprovalSchemaExecutionInformation(subcase.getOid(), opTask, opResult);
+                            modelObjectResolver.resolveAllReferences(Collections.singleton(info.asPrismContainerValue()), opTask, result); // MID-6171
                             display("Execution info for " + subcase, info);
                             opResult.computeStatus();
                             assertSuccess("Unexpected problem when looking at getApprovalSchemaExecutionInformation result", opResult);

diff --git a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/ObjectResolver.java b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/ObjectResolver.java
@@ -73,7 +73,7 @@ <O extends ObjectType> SearchResultList<PrismObject<O>> searchObjects(Class<O> t
 
     // EXPERIMENTAL (implemented only for ModelObjectResolver)
     // TODO clean up this mess
-    default void resolveAllReferences(Collection<PrismContainerValue> pcvs, Object taskObject, OperationResult result) {
+    default void resolveAllReferences(Collection<PrismContainerValue<?>> pcvs, Object taskObject, OperationResult result) {
         throw new UnsupportedOperationException();
     }