[#159] Refuse a duplicate name and return the new uid in an XML update

OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java

@@ -308,6 +308,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set<Attribute> replaceAttribute
 
         // Every attribute is checked before the entry changes, so a rejected update leaves the document as it was (#158).
         List<CheckedAttribute> checkedAttributes = new ArrayList<CheckedAttribute>();
+        Uid newUid = null;
         for (Attribute attribute : replaceAttributes) {
 
             if (!objAttributes.containsKey(attribute.getName())) {
@@ -322,7 +323,9 @@ public Uid update(ObjectClass objClass, Uid uid, Set<Attribute> replaceAttribute
             }
 
             List<String> values = null;
-            if (attributeInfo.isRequired()) {
+            // create() requires __NAME__ whatever the XSD says, and an object without a name cannot be built, so a
+            // cleared __NAME__ would leave an entry that no search of the class can read back.
+            if (attributeInfo.isRequired() || Name.NAME.equals(attributeName)) {
                 values = AttributeTypeUtil.findAttributeValue(attribute, attributeInfo);
                 if (values.isEmpty()) {
                     throw new IllegalArgumentException("No values provided for required attribute: " + attributeName);
@@ -342,7 +345,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set<Attribute> replaceAttribute
                 }
             }
 
-            // A required attribute's values were read above; reading them again would decrypt a password twice.
+            // Values read above are not read again: that would decrypt a password twice.
             if (values == null) {
                 values = AttributeTypeUtil.findAttributeValue(attribute, attributeInfo);
             }
@@ -351,6 +354,19 @@ public Uid update(ObjectClass objClass, Uid uid, Set<Attribute> replaceAttribute
                 throw new IllegalArgumentException("Data field: " + attributeName + " is not multivalued  can not have more than one value");
             }
 
+            if (Name.NAME.equals(attributeName)) {
+                for (String value : values) {
+                    if (nameTakenByAnother(objClass, entry, value)) {
+                        throw new AlreadyExistsException("Could not update entry. An entry with the " + Name.NAME + " of "
+                                + value + " already exists.");
+                    }
+                    // Without a __UID__ element the uid is the name, as create() makes it.
+                    if (!isSupportUid(objClass)) {
+                        newUid = new Uid(value);
+                    }
+                }
+            }
+
             checkedAttributes.add(new CheckedAttribute(attributeName, values));
         }
 
@@ -382,7 +398,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set<Attribute> replaceAttribute
 
         log.info("Exit {0}", method);
 
-        return uid;
+        return newUid != null ? newUid : uid;
     }
 
     @Override
@@ -735,6 +751,13 @@ private boolean entryExists(ObjectClass objClass, Uid uid, ElementIdentifierFiel
         return getEntry(objClass, uid, identifierField) != null;
     }
 
+    /** An entry that already has {@code name} keeps it, even when another entry has it too. */
+    private boolean nameTakenByAnother(ObjectClass objClass, Element entry, String name) {
+        List<Element> holders = entryIndex.find(objClass.getObjectClassValue(),
+                identifierElement(objClass, ElementIdentifierFieldType.BY_NAME), name);
+        return !holders.isEmpty() && !holders.contains(entry);
+    }
+
     private String getElementIdentifierField(ObjectClass objClass, ElementIdentifierFieldType identifierField) {
         String elementField = "";
 

OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java

@@ -50,6 +50,7 @@
 import org.identityconnectors.framework.api.APIConfiguration;
 import org.identityconnectors.framework.api.ConnectorFacade;
 import org.identityconnectors.framework.api.ConnectorFacadeFactory;
+import org.identityconnectors.framework.common.exceptions.AlreadyExistsException;
 import org.identityconnectors.framework.common.exceptions.ConnectorException;
 import org.identityconnectors.framework.common.objects.Attribute;
 import org.identityconnectors.framework.common.objects.AttributeBuilder;
@@ -197,6 +198,27 @@ public void updateShouldReturnUidWhenGivenValidParameters() {
         AssertJUnit.assertEquals(insertedUid.getUidValue(), updatedUid.getUidValue());
     }
 
+    @Test
+    public void renamingAGroupReturnsItsNewNameAsUid() {
+        // __GROUP__ has no __UID__: its uid is the name.
+        facade.create(ObjectClass.GROUP, getRequiredGroupAttributes(), null);
+        Set<Attribute> rebels = new HashSet<Attribute>();
+        rebels.add(AttributeBuilder.build(ATTR_NAME, "Rebels"));
+        rebels.add(AttributeBuilder.build(ATTR_DESCRIPTION, ATTR_GROUP_VALUE_DESCRIPTION));
+        rebels.add(AttributeBuilder.build(ATTR_SHORT_NAME, ATTR_GROUP_VALUE_SHORT_NAME));
+        facade.create(ObjectClass.GROUP, rebels, null);
+
+        Uid uid = facade.update(ObjectClass.GROUP, new Uid("Rebels"),
+                Collections.singleton(AttributeBuilder.build(ATTR_NAME, "Alliance")), null);
+
+        AssertJUnit.assertEquals("Alliance", uid.getUidValue());
+        AssertJUnit.assertNull(facade.getObject(ObjectClass.GROUP, new Uid("Rebels"), null));
+        AssertJUnit.assertEquals("Alliance",
+                facade.getObject(ObjectClass.GROUP, new Uid("Alliance"), null).getName().getNameValue());
+        expectThrows(AlreadyExistsException.class, () -> facade.update(ObjectClass.GROUP, new Uid("Alliance"),
+                Collections.singleton(AttributeBuilder.build(ATTR_NAME, ATTR_GROUP_VALUE_NAME)), null));
+    }
+
     @Test
     public void failedAddAttributeValuesKeepsTheOldValue() {
         Set<Attribute> attributes = getRequiredAccountAttributes();

OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java

@@ -89,12 +89,13 @@ public void deleteRemovesTheFirstDuplicateAndExposesTheNext() {
 
     @Test
     public void renamedEntryIsFoundByItsNewNameOnly() {
-        handler.update(ObjectClass.ACCOUNT, new Uid("u1"),
+        Uid uid = handler.update(ObjectClass.ACCOUNT, new Uid("u1"),
                 Collections.singleton(AttributeBuilder.build(Name.NAME, "renamed")));
+        assertEquals(uid.getUidValue(), "u1"); // __ACCOUNT__ has a __UID__: the rename keeps it
         expectThrows(AlreadyExistsException.class, () -> handler.create(ObjectClass.ACCOUNT, account("renamed")));
 
         handler.update(ObjectClass.ACCOUNT, new Uid("u1"),
-                Collections.singleton(AttributeBuilder.build(Name.NAME, "dup")));
+                Collections.singleton(AttributeBuilder.build(Name.NAME, "renamed-again")));
         handler.create(ObjectClass.ACCOUNT, account("renamed"));
     }
 
@@ -122,6 +123,110 @@ public void rejectedRenameKeepsTheOldName() {
                 Arrays.asList("u1|dup", "u2|dup"));
     }
 
+    @Test
+    public void renameIntoANameAnotherEntryHasIsRefused() {
+        Uid fresh = handler.create(ObjectClass.ACCOUNT, account("fresh"));
+        AlreadyExistsException e = expectThrows(AlreadyExistsException.class, () -> handler.update(ObjectClass.ACCOUNT,
+                fresh, Collections.singleton(AttributeBuilder.build(Name.NAME, "dup"))));
+        assertEquals(e.getMessage(), "Could not update entry. An entry with the __NAME__ of dup already exists.");
+
+        // Only a GROUP has the name The Empire.
+        handler.create(ObjectClass.GROUP, getRequiredGroupAttributes());
+        expectThrows(AlreadyExistsException.class, () -> handler.update(ObjectClass.GROUP,
+                new Uid("dup"), Collections.singleton(AttributeBuilder.build(Name.NAME, ATTR_GROUP_VALUE_NAME))));
+
+        // u10's name is "" in the index: a blank name is refused as blank, not as taken.
+        IllegalArgumentException blank = expectThrows(IllegalArgumentException.class, () -> handler.update(
+                ObjectClass.ACCOUNT, new Uid("u1"), Collections.singleton(AttributeBuilder.build(Name.NAME, ""))));
+        assertEquals(blank.getMessage(), "Parameter '__NAME__' must not be blank.");
+    }
+
+    @Test
+    public void keepingANameAnotherEntryAlsoHasIsAllowed() {
+        Set<Attribute> keepName = new LinkedHashSet<Attribute>();
+        keepName.add(AttributeBuilder.build(Name.NAME, "dup"));
+        keepName.add(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "L2b"));
+        handler.update(ObjectClass.ACCOUNT, new Uid("u2"), keepName);
+        assertEquals(describe(handler.search(equalsQuery(Name.NAME, "dup"), ObjectClass.ACCOUNT)),
+                Arrays.asList("u1|dup", "u2|dup"));
+
+        // Only __NAME__ is checked: another attribute may have a value that is someone's name.
+        handler.update(ObjectClass.ACCOUNT, new Uid("u4"),
+                Collections.singleton(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "dup")));
+
+        // Only an ACCOUNT has the name no-uid. __GROUP__ has no __UID__: its uid is the name, so the rename changes it.
+        Uid renamed = handler.update(ObjectClass.GROUP, new Uid("dup"),
+                Collections.singleton(AttributeBuilder.build(Name.NAME, "no-uid")));
+        assertEquals(renamed.getUidValue(), "no-uid");
+        Uid kept = handler.update(ObjectClass.GROUP, new Uid("no-uid"),
+                Collections.singleton(AttributeBuilder.build(ATTR_DESCRIPTION, "D2")));
+        assertEquals(kept.getUidValue(), "no-uid");
+    }
+
+    @Test
+    public void clearingAnOptionalNameIsRefused() throws Exception {
+        // Next to the original, so that its import of resource-schema-1.xsd resolves.
+        File optionalNameXsd = getTestFile("optional-name.xsd");
+        String xsd = new String(Files.readAllBytes(XSD_SCHEMA_FILEPATH.toPath()), StandardCharsets.UTF_8);
+        // The first __NAME__ reference is __ACCOUNT__'s.
+        Files.write(optionalNameXsd.toPath(), xsd.replaceFirst("<xsd:element ref=\"icf:__NAME__\"/>",
+                "<xsd:element ref=\"icf:__NAME__\" minOccurs=\"0\"/>").getBytes(StandardCharsets.UTF_8));
+        try {
+            File file = getRandomXMLFile();
+            writeAccounts(file, "alice", "bob");
+            XMLConfiguration config = new XMLConfiguration();
+            config.setXmlFilePath(file);
+            config.setXsdFilePath(optionalNameXsd);
+            SchemaParser parser = new SchemaParser(XMLConnector.class, optionalNameXsd);
+            XMLHandlerImpl optional = new XMLHandlerImpl(config, parser.parseSchema(), parser.getXsdSchema());
+            optional.init();
+
+            // create() requires a name whatever the XSD says, and an object without one cannot be built:
+            // a cleared entry would fail every search of the class.
+            for (String blank : Arrays.asList("", "   ")) {
+                IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> optional.update(
+                        ObjectClass.ACCOUNT, new Uid("uid-alice"), Collections.singleton(AttributeBuilder.build(Name.NAME, blank))));
+                assertEquals(e.getMessage(), "Parameter '__NAME__' must not be blank.");
+            }
+            assertEquals(describe(optional.search((Query) null, ObjectClass.ACCOUNT)),
+                    Arrays.asList("uid-alice|alice", "uid-bob|bob"));
+        } finally {
+            Files.delete(optionalNameXsd.toPath());
+        }
+    }
+
+    @Test
+    public void clearingTheOptionalNameOfAnEntryWithoutUidIsRefused() throws Exception {
+        File optionalNameXsd = getTestFile("optional-group-name.xsd");
+        String xsd = new String(Files.readAllBytes(XSD_SCHEMA_FILEPATH.toPath()), StandardCharsets.UTF_8);
+        String ref = "<xsd:element ref=\"icf:__NAME__\"/>";
+        // The second __NAME__ reference is __GROUP__'s, a class without __UID__.
+        int group = xsd.indexOf(ref, xsd.indexOf(ref) + 1);
+        Files.write(optionalNameXsd.toPath(), (xsd.substring(0, group) + "<xsd:element ref=\"icf:__NAME__\" minOccurs=\"0\"/>"
+                + xsd.substring(group + ref.length())).getBytes(StandardCharsets.UTF_8));
+        try {
+            File file = getRandomXMLFile();
+            Files.copy(getTestFile("index-fixture.xml").toPath(), file.toPath());
+            XMLConfiguration config = new XMLConfiguration();
+            config.setXmlFilePath(file);
+            config.setXsdFilePath(optionalNameXsd);
+            SchemaParser parser = new SchemaParser(XMLConnector.class, optionalNameXsd);
+            XMLHandlerImpl optional = new XMLHandlerImpl(config, parser.parseSchema(), parser.getXsdSchema());
+            optional.init();
+
+            // The uid is the name, so a cleared name would leave an entry that no search could read back.
+            for (String blank : Arrays.asList("", "   ")) {
+                IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> optional.update(
+                        ObjectClass.GROUP, new Uid("dup"), Collections.singleton(AttributeBuilder.build(Name.NAME, blank))));
+                assertEquals(e.getMessage(), "Parameter '__NAME__' must not be blank.");
+            }
+            assertEquals(describe(optional.search(equalsQuery(Name.NAME, "dup"), ObjectClass.GROUP)),
+                    Collections.singletonList("dup|dup"));
+        } finally {
+            Files.delete(optionalNameXsd.toPath());
+        }
+    }
+
     /**
      * Renames to {@code newName} along with an unknown attribute. update() rejects the unknown attribute
      * before it changes the entry, so the rename is not applied. A two-valued __NAME__ cannot be used:
@@ -281,9 +386,8 @@ public void indexAgreesWithXQueryAfterChanges() throws Exception {
         assertIndexAgreesWithXQuery("loaded");
         handler.create(ObjectClass.ACCOUNT, account("dup-new"));
         assertIndexAgreesWithXQuery("created");
-        handler.update(ObjectClass.ACCOUNT, new Uid("u2"), Collections.singleton(AttributeBuilder.build(Name.NAME, "renamed")));
-        handler.update(ObjectClass.ACCOUNT, new Uid("u2"), Collections.singleton(AttributeBuilder.build(Name.NAME, "dup")));
-        assertIndexAgreesWithXQuery("renamed back to a duplicate");
+        handler.update(ObjectClass.ACCOUNT, new Uid("u1"), Collections.singleton(AttributeBuilder.build(Name.NAME, "dup")));
+        assertIndexAgreesWithXQuery("first duplicate re-keyed");
         handler.delete(ObjectClass.ACCOUNT, new Uid("u1"));
         assertIndexAgreesWithXQuery("first duplicate deleted");
         handler.update(ObjectClass.ACCOUNT, new Uid("u11"), Collections.singleton(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "L11")));

OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java

@@ -52,6 +52,7 @@
 import org.forgerock.openicf.connectors.xml.query.QueryBuilder;
 import org.forgerock.openicf.connectors.xml.xsdparser.SchemaParser;
 import org.identityconnectors.common.security.GuardedString;
+import org.identityconnectors.framework.common.exceptions.AlreadyExistsException;
 import org.identityconnectors.framework.common.exceptions.ConnectorException;
 import org.identityconnectors.framework.common.objects.Attribute;
 import org.identityconnectors.framework.common.objects.AttributeBuilder;
@@ -186,6 +187,25 @@ public void rejectedUpdateLeavesTheEntryAndTheFileAlone(Set<Attribute> attribute
         assertEquals(call(h -> firstAndLastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), List.of("null|Last-alice"));
     }
 
+    @Test
+    public void renameIntoATakenNameLeavesTheEntryAndTheFileAlone() throws Exception {
+        writeAccounts(file, "alice", "bob");
+        FileTime past = FileTime.fromMillis(now() - HOUR);
+        Files.setLastModifiedTime(file.toPath(), past);
+        byte[] before = Files.readAllBytes(file.toPath());
+
+        // The name comes after an attribute that would be applied first.
+        AlreadyExistsException e = expectThrows(AlreadyExistsException.class, () -> call(h -> h.update(ObjectClass.ACCOUNT,
+                new Uid("uid-alice"), attributes(AttributeBuilder.build(ATTR_ACCOUNT_FIRST_NAME, "CHANGED"),
+                        AttributeBuilder.build(Name.NAME, "bob")))));
+
+        assertEquals(e.getMessage(), "Could not update entry. An entry with the __NAME__ of bob already exists.");
+        assertEquals(Files.getLastModifiedTime(file.toPath()), past);
+        assertEquals(Files.readAllBytes(file.toPath()), before);
+        assertEquals(call(h -> firstAndLastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))),
+                List.of("null|Last-alice", "null|Last-bob"));
+    }
+
     @Test
     public void updateIsInTheFileWhenTheLastUserLeaves() throws Exception {
         writeAccounts(file, "alice");