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

[maximthomas]

XMLHandlerImpl.update now refuses to rename an entry to a __NAME__ that another entry of the same class has, and it refuses a blank __NAME__ even where the XSD makes __NAME__ optional. When the class has no __UID__, so the uid is the name, it returns the new uid.

Before, create refused a name that already existed or was blank, but update accepted any new __NAME__, a blank one too when the XSD made __NAME__ optional, and it always returned the uid it was given.

call	before	after
update(ACCOUNT, <uid of n2>, {__NAME__=n1}) while another account is n1	accepted; a search for n1 returns two objects, and authenticate as n1 with n2's password fails with InvalidPasswordException (#159)	AlreadyExistsException: Could not update entry. An entry with the __NAME__ of n1 already exists.; entry and file unchanged
update(GROUP, Rebels, {__NAME__=Alliance}) (__GROUP__ has no __UID__)	returns Rebels; getObject(GROUP, Rebels) is null	returns Alliance
update(ACCOUNT, <uid>, {__NAME__=""}) where a user XSD makes __ACCOUNT__'s __NAME__ optional	accepted; a search of the whole class, or by that entry's __UID__, then fails with IllegalArgumentException: The Attribute set must contain a 'Name'. " " is accepted as well and fails the same way once the file is saved and loaded again	IllegalArgumentException: Parameter '__NAME__' must not be blank.; entry unchanged
update(GROUP, dup, {__NAME__=""}) where a user XSD makes __GROUP__'s __NAME__ optional	accepted; a search of the whole class then fails with IllegalArgumentException: Uid value must not be blank!	IllegalArgumentException: Parameter '__NAME__' must not be blank.; entry unchanged

• The check runs in update's check phase (XML connector: a failed update is saved half-applied #158), after the attribute's own checks and before the entry changes. A refused rename leaves the document and the file as they were. A blank __NAME__ is still reported as Parameter '__NAME__' must not be blank.
• It uses the same index lookup as create's duplicate check: names are looked up within the entry's object class, so an account may still have a group's name, and values are compared as they are (" padded " is not "padded").
• An entry may send the name it already has, even when another entry has it too (duplicates already in a file). update(u2, {__NAME__=dup, lastname=L2b}) still succeeds when u1 is dup as well.
• __NAME__ is checked as a required attribute in every class, even where a user XSD makes it optional: create requires a name whatever the XSD says, and the framework builds no object without a Name. One cleared entry made every search of its class fail, reconciliation included, and it could not be read by its __UID__ to be repaired. The blank check runs before the duplicate check, so a blank name is reported as blank, not as taken.
• The message names __NAME__. create's duplicate message still names __UID__; it is not changed here.
• For a class with __UID__, the returned uid is unchanged.

Two existing tests renamed an entry into a name another entry had, and they now use other names. XMLHandlerIndexTests#renamedEntryIsFoundByItsNewNameOnly renames u1 to renamed-again instead of back to dup, which u2 has. XMLHandlerIndexTests#indexAgreesWithXQueryAfterChanges no longer renames u2 to renamed and back to dup; instead u1 re-sends dup, its own name. Re-linking u1 in front of u2 also pins the ordered insert of EntryIndex.link (EntryIndex.java:225). The old step re-linked u2 at the end of the list, which is also its place in document order, so it could not catch an insert at the end.

Not fixed here:

• A hand-edited file can nest an entry inside another entry's __NAME__ element (XML connector: update, delete and authenticate break on an entry below another element #182). Changing the inner entry leaves the outer entry's index key stale until the next reload (EntryIndex.java:49-51). Until then, the check can accept a rename into the outer entry's current name, and refuse the outer entry re-sending its current name when another entry has it too. The connector never writes that shape.
• A DOM failure after the rename is applied (XML connector: update, delete and authenticate break on an entry below another element #182) still leaves the entry renamed. For a class without __UID__, the caller gets the exception and keeps the old uid.
• An entry whose __NAME__ is already blank in the file (hand-edited, or cleared by update before this change) still makes every search of its class fail. The read side is not changed.

Test strength

arm	case	mutant killed	red at
XMLHandlerImpl.java:357-362 refuse a __NAME__ another entry has	XMLHandlerIndexTests#renameIntoANameAnotherEntryHasIsRefused step 1 (the message)	no check; __UID__ in the message	1eeff90
same, before the entry changes	XMLHandlerReloadTests#renameIntoATakenNameLeavesTheEntryAndTheFileAlone (firstname first, then the taken name: the message, the file's mtime and bytes, then both entries)	check moved into the apply loop	1eeff90
:756-757 looked up within the entry's class	renameIntoANameAnotherEntryHasIsRefused step 2 (group dup to The Empire, a group's name); keepingANameAnotherEntryAlsoHasIsAllowed step 3 (group dup to no-uid, an account's name)	ObjectClass.ACCOUNT constant; both classes looked up	those mutants only
same	renameIntoATakenNameLeavesTheEntryAndTheFileAlone (accounts only), keepingANameAnotherEntryAlsoHasIsAllowed step 1, indexAgreesWithXQueryAfterChanges	ObjectClass.GROUP constant (step 1 of renameIntoANameAnotherEntryHasIsRefused does not kill it: a group is named dup too)	that mutant only
:758 an entry that has the name may keep it	keepingANameAnotherEntryAlsoHasIsAllowed step 1 (u2 re-sends dup, which u1 has first); indexAgreesWithXQueryAfterChanges ("first duplicate re-keyed": u1 re-sends dup)	refused whenever the name has a holder; refused unless the entry is the only holder; refused unless it is the first holder	those mutants only
:357 only __NAME__ is checked	keepingANameAnotherEntryAlsoHasIsAllowed step 2 (lastname = dup)	every attribute checked	that mutant only
:328 __NAME__ checked as required in every class, so a blank one is refused	clearingAnOptionalNameIsRefused (a copy of the test XSD with an optional account __NAME__; "" and " " refused with the message, then a search of the whole class returns both accounts); clearingTheOptionalNameOfAnEntryWithoutUidIsRefused (a copy with an optional group __NAME__; the same refusals, then the group is still found as dup)	__NAME__ checked only when required (the account's clear accepted; the group's fails on the framework's Uid value must not be blank!)	1eeff90
:334 the blank check before the duplicate check	renameIntoANameAnotherEntryHasIsRefused step 3 (u1 to "" while u10's name is "" in the index: the blank message, not the duplicate one)	duplicate check first	that mutant only
EntryIndex.java:225 a re-keyed entry re-linked in document order	indexAgreesWithXQueryAfterChanges ("first duplicate re-keyed")	appended at the end	that mutant only (green at 1eeff90)
:364, :365 the new name as uid when the class has no __UID__	keepingANameAnotherEntryAlsoHasIsAllowed step 3 (dup to no-uid returns no-uid); XMLConnectorTests#renamingAGroupReturnsItsNewNameAsUid (through the facade: Rebels to Alliance returns Alliance, getObject(Rebels) is null, a rename of Alliance to The Empire is refused)	input uid returned	1eeff90
same, only then	renamedEntryIsFoundByItsNewNameOnly (a renamed account keeps u1)	isSupportUid ignored; inverted	those mutants only (green at 1eeff90)
:401 the input uid otherwise	keepingANameAnotherEntryAlsoHasIsAllowed step 3 (a __DESCRIPTION__ update of no-uid returns no-uid)	newUid returned without the fallback	that mutant only

mvn -pl OpenICF-xml-connector verify: 248 unit tests (242 + 6) and 1 IT, green on JDK 26 and JDK 11.

Fixes #159

[vharseko]

Checked at 9e7d4dc. The duplicate check and the returned uid work as described: a refused rename leaves the entry and the file alone, an entry may re-send its own name, and a class without __UID__ gets its new uid back. One point, about the blank-name branch.

A cleared __NAME__ in a class with __UID__ leaves an entry that can no longer be read (XMLHandlerImpl.java:757-761, clearingAnOptionalNameOfTwoEntriesIsAllowed)

The description refuses a blank name in a class without __UID__ because "a search of the whole class then fails". The same happens in a class with __UID__, only with a different message. I ran it on this branch with the optional-name XSD from clearingAnOptionalNameOfTwoEntriesIsAllowed, accounts alice and bob:

step	result
update(ACCOUNT, uid-alice, {__NAME__=""}), then the same for uid-bob	both accepted, as the test asserts
search of the whole __ACCOUNT__ class	IllegalArgumentException: The Attribute set must contain a 'Name'.
search by __UID__ = uid-alice	the same
update(ACCOUNT, uid-carol, {__NAME__=" "}), then a search	uid-carol| : readable until the save
dispose(), a new handler on the file, a search	The Attribute set must contain a 'Name'.

setTextContent("") leaves the __NAME__ element without a text node (normalizeText does the same to " " on save or load). ConnectorObjectCreator.addAllAttributesToBuilder skips such an element, so the object has no Name, and ConnectorObject's constructor refuses it. One cleared entry is enough to make every search of the class fail, reconciliation included. getObject on that entry fails too, so it cannot even be read in order to repair it.

This is not new: on master, update accepted a blank optional __NAME__ in any class. But this PR now decides that case on purpose, and the test pins it as allowed. Two things suggest refusing it in every class, as the PR already does without __UID__:

• create never makes a nameless entry. It requires __NAME__ whatever the XSD says (__NAME__ must be defined.), and new Uid(name) refuses a blank one.
• The framework requires a Name on every object.

Doing so would make the blank branch in nameTakenByAnother unnecessary, and clearingAnOptionalNameOfTwoEntriesIsAllowed would turn into a refusal test. Its current form only checks that the two updates do not throw, which is why it stays green. If clearing has to stay, the read side has to accept a nameless entry instead, and that is a larger change than this PR.

(The fixture's u10, with __NAME__ = " ", is equally unreadable: a search by __UID__ = u10 fails the same way. That is a hand-written file and outside this PR; I mention it only because it shows the read side does not tolerate such entries.)

[maximthomas]

Fixed in 36def07: update now refuses a blank __NAME__ in every class.

• XMLHandlerImpl.java:328 checks __NAME__ as a required attribute even where the XSD makes it optional. "" and " " get Parameter '__NAME__' must not be blank. in the check phase, before the entry changes. The blank branch in nameTakenByAnother and the separate check for a class without __UID__ are gone.
• clearingAnOptionalNameOfTwoEntriesIsAllowed is now clearingAnOptionalNameIsRefused. It sends "" and " " to uid-alice, expects the refusal for both, and then expects a search of the whole class to return both accounts. It is red at 1eeff90 and at 9e7d4dc. It kills the mutant that checks __NAME__ only when it is required, and so does clearingTheOptionalNameOfAnEntryWithoutUidIsRefused (it then fails on the framework's Uid value must not be blank!).
• With the blank branch gone, the order of the two checks can be observed: if the duplicate check runs first, renameIntoANameAnotherEntryHasIsRefused step 3 (u1 to "" while u10 is "" in the index) gets AlreadyExistsException. The description's "Not pinned" note is now a row of the test-strength table.
• Your table reproduces on master 1eeff90 too: the whole-class search, the search by uid-alice, and the search for uid-carol after a reload all fail with The Attribute set must contain a 'Name'. The before/after table has a new __ACCOUNT__ row for it.
• An entry whose name is already blank in the file, like the fixture's u10, still breaks a search of its class. The read side is unchanged; this is listed under "Not fixed here".

The earlier mutants were re-run on the new code and are still killed. mvn -pl OpenICF-xml-connector verify: 248 unit tests and 1 IT, green on JDK 26 and JDK 11.

[vharseko]

Checked at 36def07. The blank-name point is fixed: update now checks __NAME__ as required in every class (XMLHandlerImpl.java:328), so "" and " " are refused with Parameter '__NAME__' must not be blank. before the entry changes, and a search of the class still returns both accounts.

Two mutants on this head: checking __NAME__ only when the XSD requires it fails clearingAnOptionalNameIsRefused and clearingTheOptionalNameOfAnEntryWithoutUidIsRefused; running the duplicate check before the blank check fails step 3 of renameIntoANameAnotherEntryHasIsRefused. mvn -pl OpenICF-xml-connector -am verify on JDK 21: 248 unit tests and 1 IT, green.

Nothing further from me.