MID-1951 (checking for duplicate IDs on containers).

gui/admin-gui/src/main/java/com/evolveum/midpoint/web/component/prism/ObjectWrapper.java

@@ -667,7 +667,7 @@ private ObjectDelta createAddingObjectDelta() throws SchemaException {
         Collections.sort(containers, new ItemWrapperComparator());
 
         if (InternalsConfig.consistencyChecks) {
-        	delta.checkConsistence(true, true, true);
+        	delta.checkConsistence(true, true, true, ConsistencyCheckScope.THOROUGH);
         }
 
 		return delta;

infra/common/src/test/java/com/evolveum/midpoint/common/refinery/TestRefinedSchema.java

@@ -31,6 +31,7 @@
 import javax.xml.bind.JAXBException;
 import javax.xml.namespace.QName;
 
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import com.evolveum.midpoint.prism.util.PrismTestUtil;
 
 import org.testng.Assert;
@@ -356,7 +357,7 @@ private void assertAccountShadow(PrismObject<ShadowType> accObject, PrismObject<
         String accString = PrismTestUtil.serializeObjectToString(accObjectType.asPrismObject());
         System.out.println("Result of JAXB marshalling:\n"+accString);
 
-        accObject.checkConsistence(true, true);
+        accObject.checkConsistence(true, true, ConsistencyCheckScope.THOROUGH);
     }
 
 	private QName getAttrQName(PrismObject<ResourceType> resource, String localPart) {

infra/prism/src/main/java/com/evolveum/midpoint/prism/ConsistencyCheckScope.java

@@ -1,16 +1,40 @@
 package com.evolveum.midpoint.prism;
 
+import com.evolveum.midpoint.prism.delta.ObjectDelta;
+
+import java.util.Collection;
+
 /**
  * Determines the scope of consistency checks.
- * (Originally this was a boolean, but there are many methods with N boolean arguments, so it was
- * too easy to mix them up.)
+ *
+ * (Originally this was a boolean, but there are many 'checkConsistence'-style methods with a set of boolean arguments,
+ * so it was too easy to mix them up with this new one.)
  *
  * @author mederly
  */
 public enum ConsistencyCheckScope {
-    THOROUGH, MANDATORY_CHECKS_ONLY;
+    /**
+     * Full-scale checks.
+     */
+    THOROUGH,
+    /**
+     * Mandatory checks, e.g. checks for duplicate container IDs (see MID-1951).
+     * Should be rather quick.
+     *
+     * TODO Current solution is not that optimal. We should distinguish between checks that deal with midPoint internal workings
+     * (throwing IllegalArgumentException/IllegalStateException on failure), which can be turned off, as it is today. Another set
+     * of checks should be applied only on users' inputs (when importing data, when accepting inputs via SOAP/REST/whathever interfaces, ...)
+     * - and these should throw perhaps SchemaException that can be caught and handled appropriately.
+     *
+     * However, for the time being we consider this approach (i.e. that both kinds of checks are implemented the same way) to be an acceptable one.
+     */
+    MANDATORY_CHECKS_ONLY;
 
     public boolean isThorough() {
         return this == THOROUGH;
     }
+
+    public static ConsistencyCheckScope fromBoolean(boolean consistencyChecksSwitchValue) {
+        return consistencyChecksSwitchValue ? THOROUGH : MANDATORY_CHECKS_ONLY;
+    }
 }

infra/prism/src/main/java/com/evolveum/midpoint/prism/Item.java

@@ -620,7 +620,11 @@ public static <T extends Item> T createNewDefinitionlessItem(QName name, Class<T
     public void checkConsistence(boolean requireDefinitions, ConsistencyCheckScope scope) {
     	checkConsistenceInternal(this, requireDefinitions, false, scope);
     }
-
+
+    public void checkConsistence(boolean requireDefinitions, boolean prohibitRaw) {
+        checkConsistenceInternal(this, requireDefinitions, prohibitRaw, ConsistencyCheckScope.THOROUGH);
+    }
+
     public void checkConsistence(boolean requireDefinitions, boolean prohibitRaw, ConsistencyCheckScope scope) {
     	checkConsistenceInternal(this, requireDefinitions, prohibitRaw, scope);
     }
@@ -629,10 +633,11 @@ public void checkConsistence() {
     	checkConsistenceInternal(this, false, false, ConsistencyCheckScope.THOROUGH);
     }
 
-    public void checkConsistenceMandatory() {
-        checkConsistenceInternal(this, false, false, ConsistencyCheckScope.MANDATORY_CHECKS_ONLY);
+    public void checkConsistence(ConsistencyCheckScope scope) {
+        checkConsistenceInternal(this, false, false, scope);
     }
 
+
     public void checkConsistenceInternal(Itemable rootItem, boolean requireDefinitions, boolean prohibitRaw, ConsistencyCheckScope scope) {
     	ItemPath path = getPath();
     	if (elementName == null) {

infra/prism/src/main/java/com/evolveum/midpoint/prism/PrismContainer.java

@@ -168,6 +168,13 @@ public boolean add(PrismContainerValue newValue) throws SchemaException {
         if (newValue.getPrismContext() == null && this.prismContext != null) {
             prismContext.adopt(newValue);
         }
+        if (newValue.getId() != null) {
+            for (PrismContainerValue existingValue : getValues()) {
+                if (existingValue.getId() != null && existingValue.getId().equals(newValue.getId())) {
+                    throw new IllegalStateException("Attempt to add a container value with an id that already exists: " + newValue.getId());
+                }
+            }
+        }
         return super.add(newValue);
     }
 

infra/prism/src/main/java/com/evolveum/midpoint/prism/PrismPropertyValue.java

@@ -249,7 +249,11 @@ void checkValue() {
 	}
 
     @Override
-	public void checkConsistenceInternal(Itemable rootItem, boolean requireDefinitions, boolean prohibitRaw) {
+	public void checkConsistenceInternal(Itemable rootItem, boolean requireDefinitions, boolean prohibitRaw, ConsistencyCheckScope scope) {
+        if (!scope.isThorough()) {
+            return;
+        }
+
     	ItemPath myPath = getPath();
     	if (prohibitRaw && rawElement != null) {
     		throw new IllegalStateException("Raw element in property value "+this+" ("+myPath+" in "+rootItem+")");

infra/prism/src/main/java/com/evolveum/midpoint/prism/PrismReferenceValue.java

@@ -248,7 +248,11 @@ public void recompute(PrismContext prismContext) {
 	}
 
 	@Override
-	public void checkConsistenceInternal(Itemable rootItem, boolean requireDefinitions, boolean prohibitRaw) {
+	public void checkConsistenceInternal(Itemable rootItem, boolean requireDefinitions, boolean prohibitRaw, ConsistencyCheckScope scope) {
+        if (!scope.isThorough()) {
+            return;
+        }
+
 		ItemPath myPath = getPath();
 
 		if (oid == null && object == null && filter == null) {

infra/prism/src/main/java/com/evolveum/midpoint/prism/delta/ItemDelta.java

@@ -765,7 +765,11 @@ public void validate(String contextDescription) throws SchemaException {
     	}
     }
 
-	public static void checkConsistence(Collection<? extends ItemDelta> deltas, ConsistencyCheckScope scope) {
+    public static void checkConsistence(Collection<? extends ItemDelta> deltas) {
+        checkConsistence(deltas, ConsistencyCheckScope.THOROUGH);
+    }
+
+    public static void checkConsistence(Collection<? extends ItemDelta> deltas, ConsistencyCheckScope scope) {
 		checkConsistence(deltas, false, false, scope);
 	}
 

infra/prism/src/main/java/com/evolveum/midpoint/prism/delta/ObjectDelta.java

@@ -1145,7 +1145,15 @@ public static <O extends Objectable> ObjectDelta<O> createDeleteDelta(Class<O> t
     }
 
     public void checkConsistence() {
-    	checkConsistence(true, false, false, ConsistencyCheckScope.THOROUGH);
+    	checkConsistence(ConsistencyCheckScope.THOROUGH);
+    }
+
+    public void checkConsistence(ConsistencyCheckScope scope) {
+        checkConsistence(true, false, false, scope);
+    }
+
+    public void checkConsistence(boolean requireOid, boolean requireDefinition, boolean prohibitRaw) {
+        checkConsistence(requireOid, requireDefinition, prohibitRaw, ConsistencyCheckScope.THOROUGH);
     }
 
     public void checkConsistence(boolean requireOid, boolean requireDefinition, boolean prohibitRaw, ConsistencyCheckScope scope) {

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelCrudService.java

@@ -21,6 +21,7 @@
 
 import javax.xml.namespace.QName;
 
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import com.evolveum.midpoint.prism.crypto.Protector;
 
 import org.apache.commons.lang.StringUtils;
@@ -452,7 +453,7 @@ public <T extends ObjectType> void modifyObject(Class<T> type, String oid,
 			return;
 		}
 
-		ItemDelta.checkConsistence(modifications);
+		ItemDelta.checkConsistence(modifications, ConsistencyCheckScope.THOROUGH);
 		// TODO: check definitions, but tolerate missing definitions in <attributes>
 
 		OperationResult result = parentResult.createSubresult(MODIFY_OBJECT);

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java

@@ -35,6 +35,7 @@
 import com.evolveum.midpoint.model.api.hooks.ReadHook;
 import com.evolveum.midpoint.model.impl.scripting.ExecutionContext;
 import com.evolveum.midpoint.model.impl.scripting.ScriptingExpressionEvaluator;
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import com.evolveum.midpoint.prism.parser.XNodeSerializer;
 import com.evolveum.midpoint.prism.polystring.PolyString;
 import com.evolveum.midpoint.wf.api.WorkflowManager;
@@ -1622,7 +1623,7 @@ private <T extends ObjectType> void validateObject(PrismObject<T> object, GetOpe
 					tolerateRaw = true;
 				}
 			}
-			object.checkConsistence(true, !tolerateRaw);
+			object.checkConsistence(true, !tolerateRaw, ConsistencyCheckScope.THOROUGH);
 		} catch (RuntimeException e) {
 			result.recordFatalError(e);
 			throw e;

model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/importer/ObjectImporter.java

@@ -444,7 +444,7 @@ protected <T extends ObjectType> void validateWithDynamicSchemas(PrismObject<T>
 
             // now we check for raw data - their presence means e.g. that there is a connector property that is unknown in connector schema (applyDefinition does not scream in such a case!)
             try {
-                configurationContainer.checkConsistence(true, true);        // require definitions and prohibit raw
+                configurationContainer.checkConsistence(true, true, ConsistencyCheckScope.THOROUGH);        // require definitions and prohibit raw
             } catch (IllegalStateException e) {
                 // TODO do this error checking and reporting in a cleaner and more user-friendly way
                 result.recordFatalError("Configuration error in " + resource + " (probably incorrect connector property, see the following error): " + e.getMessage(), e);

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

@@ -31,6 +31,7 @@
 import com.evolveum.midpoint.model.common.expression.ExpressionFactory;
 import com.evolveum.midpoint.model.common.expression.ExpressionVariables;
 import com.evolveum.midpoint.model.impl.util.Utils;
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import com.evolveum.midpoint.prism.PrismContext;
 import com.evolveum.midpoint.prism.PrismObject;
 import com.evolveum.midpoint.prism.PrismObjectDefinition;
@@ -588,7 +589,7 @@ void executeDelta(ObjectDelta<T> objectDelta, LensElementContext<T> objectContex
         	return;
         }
 
-        if (consistencyChecks) objectDelta.checkConsistence();
+        objectDelta.checkConsistence(ConsistencyCheckScope.fromBoolean(consistencyChecks));
 
         // Other types than focus types may not be definition-complete (e.g. accounts and resources are completed in provisioning)
         if (FocusType.class.isAssignableFrom(objectDelta.getObjectTypeClass())) {

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

@@ -18,6 +18,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import org.apache.commons.lang.Validate;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
@@ -86,8 +87,10 @@ public <F extends ObjectType> LensContext<F> createContext(
 				}
 				if (InternalsConfig.consistencyChecks) {
 					// Focus delta has to be complete now with all the definition already in place
-					delta.checkConsistence(false, true, true);
-				}
+					delta.checkConsistence(false, true, true, ConsistencyCheckScope.THOROUGH);
+				} else {
+                    delta.checkConsistence(ConsistencyCheckScope.MANDATORY_CHECKS_ONLY);        // TODO is this necessary? Perhaps it would be sufficient to check on model/repo entry
+                }
                 if (focusDelta != null) {
                     throw new IllegalStateException("More than one focus delta used in model operation");
                 }

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

@@ -18,6 +18,7 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import com.evolveum.midpoint.prism.Objectable;
 import com.evolveum.midpoint.schema.DeltaConvertor;
 import com.evolveum.midpoint.schema.result.OperationResult;
@@ -414,7 +415,7 @@ public void checkConsistence(String contextDesc) {
 
 	private void checkConsistence(ObjectDelta<O> delta, boolean requireOid, String contextDesc) {
 		try {
-			delta.checkConsistence(requireOid, true, true);
+			delta.checkConsistence(requireOid, true, true, ConsistencyCheckScope.THOROUGH);
 		} catch (IllegalArgumentException e) {
 			throw new IllegalArgumentException(e.getMessage()+"; in "+contextDesc, e);
 		} catch (IllegalStateException e) {
@@ -432,7 +433,7 @@ protected boolean isRequireSecondardyDeltaOid() {
 	protected void checkConsistence(PrismObject<O> object, String elementDesc, String contextDesc) {
 		String desc = elementDesc+" in "+this + (contextDesc == null ? "" : " in " +contextDesc);
     	try {
-    		object.checkConsistence(true);
+    		object.checkConsistence(true, ConsistencyCheckScope.THOROUGH);
     	} catch (IllegalArgumentException e) {
 			throw new IllegalArgumentException(e.getMessage()+"; in "+desc, e);
 		} catch (IllegalStateException e) {

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

@@ -751,7 +751,7 @@ public void checkConsistence(String contextDesc, boolean fresh, boolean force) {
     	}
     	if (syncDelta != null) {
     		try {
-    			syncDelta.checkConsistence(true, true, true);
+    			syncDelta.checkConsistence(true, true, true, ConsistencyCheckScope.THOROUGH);
     		} catch (IllegalArgumentException e) {
 				throw new IllegalArgumentException(e.getMessage()+"; in "+getElementDesc()+" sync delta in "+this + (contextDesc == null ? "" : " in " +contextDesc), e);
 			} catch (IllegalStateException e) {

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

@@ -16,6 +16,7 @@
 package com.evolveum.midpoint.model.impl.lens;
 
 import com.evolveum.midpoint.common.crypto.CryptoUtil;
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import com.evolveum.midpoint.prism.PrismContext;
 import com.evolveum.midpoint.prism.delta.ObjectDelta;
 import com.evolveum.midpoint.schema.DeltaConvertor;
@@ -86,7 +87,7 @@ public void checkConsistence(boolean requireOid, String shortDesc) {
 				continue;
 			}
 			try {
-				delta.checkConsistence(requireOid, true, true);
+				delta.checkConsistence(requireOid, true, true, ConsistencyCheckScope.THOROUGH);
 			} catch (IllegalArgumentException e) {
 				throw new IllegalArgumentException(e.getMessage()+"; in "+shortDesc+", wave "+wave, e);
 			} catch (IllegalStateException e) {

model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/TestModelServiceContract.java

@@ -35,6 +35,7 @@
 
 import com.evolveum.icf.dummy.resource.BreakMode;
 import com.evolveum.midpoint.notifications.api.transports.Message;
+import com.evolveum.midpoint.prism.ConsistencyCheckScope;
 import com.evolveum.midpoint.prism.delta.PropertyDelta;
 import com.evolveum.midpoint.prism.match.PolyStringOrigMatchingRule;
 import com.evolveum.midpoint.prism.query.EqualFilter;
@@ -175,7 +176,7 @@ public void test051GetUserBarbossa() throws Exception {
         result.computeStatus();
         TestUtil.assertSuccess("getObject result", result);
 
-        userBarbossa.checkConsistence(true, true);
+        userBarbossa.checkConsistence(true, true, ConsistencyCheckScope.THOROUGH);
 
         assertSteadyResources();
 	}
@@ -361,7 +362,7 @@ public void test101GetAccount() throws Exception {
         result.computeStatus();
         TestUtil.assertSuccess("getObject result", result);
 
-        account.checkConsistence(true, true);
+        account.checkConsistence(true, true, ConsistencyCheckScope.THOROUGH);
 
         IntegrationTestTools.assertAttribute(account, getAttributeQName(resourceDummy, DummyResourceContoller.DUMMY_ACCOUNT_ATTRIBUTE_TITLE_NAME), 
         		"The best pirate captain ever");

repo/repo-sql-impl-test/src/test/java/com/evolveum/midpoint/repo/sql/AddGetObjectTest.java

@@ -575,19 +575,19 @@ public void test100AddUserWithoutAssignmentIds() throws Exception {
     @Test
     public void test110AddUserWithDuplicateAssignmentIds() throws Exception {
         OperationResult result = new OperationResult("test110AddUserWithDuplicateAssignmentIds");
-        PrismObject<UserType> user = PrismTestUtil.parseObject(new File(FOLDER_BASIC, "user-big.xml"));
+        PrismObject<UserType> user = PrismTestUtil.parseObject(new File(FOLDER_BASIC, "user-same-ids.xml"));
 
         //create duplicate ids in assignment values
         PrismContainer container = user.findContainer(UserType.F_ASSIGNMENT);
         List<PrismContainerValue> values = (List<PrismContainerValue>) container.getValues();
-        values.get(0).setId(999L);
-        values.get(0).setId(999L);
+        values.get(0).setId(999L);      // setting it manually because object with duplicate IDs could not be parsed at all
+        values.get(1).setId(999L);
         try {
             String OID = repositoryService.addObject(user, null, result);
             throw new AssertionError("Two container values with the same ID were accepted even they shouldn't be");
-        } catch (SchemaException e) {
+        } catch (RuntimeException e) {
             // this was expected
-        } catch (Throwable e) {
+        } catch (Exception e) {
             throw new AssertionError("Two container values with the same ID resulted in unexpected exception", e);
         }
     }