Moving an attachment deletes the attachment without creating the new one

This is a regression introduced by the fix of XWIKI-24555 (commit 2537fcdf6a73a3be4e7e59f1c3773afafd33b23b on master, backported as 874cdb541fa on stable-18.4.x and 30101ae8990 on stable-17.10.x).

Analysis

That commit changed MoveAttachmentJob#moveAttachment() to stop modifying the cached document:

-  XWikiDocument sourceDocument = wiki.getDocument(source.getParent(), this.xcontextProvider.get());
-  XWikiDocument targetDocument = wiki.getDocument(destination.getParent(), this.xcontextProvider.get());
+  XWikiDocument sourceDocument = wiki.getDocument(source.getParent(), context).clone();
+  XWikiDocument targetDocument = wiki.getDocument(destination.getParent(), context).clone();

XWikiCacheStore#loadXWikiDoc() returns the cached instance itself, so before that change, when the source and the destination are the same document (a pure rename), the two getDocument() calls returned the same object: sourceDocument == targetDocument. The rename then worked by aliasing:

• sourceDocument.removeAttachment(sourceAttachment) removes the old attachment,
• addAttachment(targetDocument, ...) adds the renamed one,
• since the source and destination documents are equal, only sourceDocument is saved.

Now that each call is cloned separately, the two variables reference two distinct instances. The renamed attachment is added to targetDocument, a clone that is never saved, while the removal performed on sourceDocument is saved. Hence the attachment is deleted and the new one is never created, which is exactly what this issue reports. As a side effect the auto-redirect object is created on the saved document but points to an attachment that does not exist, and removeExistingRedirection(destination.getName(), targetDocument) also operates on the discarded clone.

Moving an attachment to a different page is unaffected because that path goes through transactionalMove(), which saves both documents.

Affected versions

• 17.10.10 and later of the 17.10.x cycle,
• 18.4.3 and later of the 18.4.x cycle,
• 18.6.0RC1 and later.

Why our tests did not catch it

Two independent gaps:

1. The functional test never exercises the broken branch. MoveAttachmentIT has a single test, moveAttachment(), and it renames moveme.txt to newname.txt while also moving the attachment from page Source to page Target. That is the transactionalMove() path, which saves both documents and therefore still passes. The in-place rename branch has no functional test at all: rename and move are only ever tested together, never rename alone.

2. The unit test covering that branch was made blind to the change by the same commit. MoveAttachmentJobTest#processRename() does cover the in-place case, but the commit added to setUp():

when(this.sourceDocument.clone()).thenReturn(this.sourceDocument);
when(this.targetDocument.clone()).thenReturn(this.targetDocument);

This stubs clone() as an identity function, which reinstates in the test the very aliasing the production code had just lost. Both getDocument() calls returned the same mock, so verify(this.sourceDocument).setAttachment(targetAttachment) and verifyNoInteractions(this.targetDocument) still held, vacuously. The commit even strengthened the test with verify(this.sourceDocument, times(2)).clone(), so it looks like the new behaviour is asserted while the one property the code now depends on, namely that clone() returns a distinct instance, was stubbed away. More generally, because XWikiDocument is mocked in this test, document identity semantics are whatever the stubbing says, which makes the test structurally unable to catch aliasing bugs.

Fix

In the in-place case, use a single document instance for both the removal and the addition, and add the two missing tests: an in-place rename functional test in MoveAttachmentIT, and distinct clone mocks in MoveAttachmentJobTest so that the unit tests assert which instance is actually saved.

Using @OldcoreTest could also be interesting if that works with attachments as it would allow to more faithfully reproduce the failure in a unit test.