XWIKI-12987: Relative links are made absolute or even broken after mo…

…ving a page (#3634)

The idea of this work is to:

    * Change the way AbstractCopyOrMoveJob works to perform computation of couple source/target documents before processing them
    * Provide a way to access that map source/target documents
    * Use that information when performing a call to ReferenceRenamer to define if a relative untyped link should be handled or not

The PR provides mainly:

    - new APIs in ReferenceRenamer and MacroRefactoring to integrate the map of references that have been moved as part of same job
   - refactorings of AbstractCopyOrMoveJob:
      *  specific computation of getEntities to actually visit the hierarchy and populate the entities with the couple of source/target documents
      * new abstract methods to avoid duplications (not strictly needed for this work)
      * new method to retrieve the map of source/target documents
    - new conditions in ResourceReferenceRenamer to decide if a link should be renamed or not: most of the logic of the fix is encoded there (see also clarifications)
    - new calls in XWiki#updateLinksForRename and BackLinkUpdaterListener#updateBackLinks to give the map of source/target when calling the rename of references
    - new integration test simulating the scenario indicated in the ticket and also performing a supplementary check related to a regression found afterwards
    - same integration test also performed on a subwiki in SubWikiIT

The refactoring of references is currently called at two places:

    by the BackLinkUpdaterListener for all backlinks after a document has been renamed (triggered by a document event)
    by XWiki#updateLinksForRename to rename the internal links of the current document (which is always a call to updateResourceReferenceRelative, see below)

The problem of XWIKI-12987 is that XWiki#updateLinksForRename is called first and does perform an absolute rename of the relative links.
Now ResourceReferenceRenamer APIs names might be misleading: updateResourceReferenceRelative and updateResourceReferenceAbsolute are not about the references being absolute or relative: it's about the renamed references being absolute or relative respectively to the current document. It took me a while to integrate this, and I'm still struggling a bit with it.

So the problem was to find a proper condition to decide when to not refactor links, for this I'm performing a check for assessing if a link is absolute or not, by trying to resolve the ResourceReference without any parameter: if the result equals the reference with parameter then it was absolute.

Then for the updateResourceReferenceAbsolute the idea is to only perform update of the links, if the provided link is absolute, or if it's relative but the current document hasn't been moved as part of same job: in such case we do need to update the relative link, because there won't be a call to XWiki#updateLinksForRename on that document to update the link, we only get the call from BackLinkUpdaterListener.

For the updateResourceReferenceRelative the check is a bit more complex.
We only update links that are relative here, we don't want to update absolute references (is that correct? Can't find a counter example right now).
Then since we only perform refactoring of links relative to current document, we also check that the link about to be refactored is not related to pages that are part of the moved document in the same job: if those are also moved in the same job, then they're moved using same "direction", they're part of same hierarchy and we don't want to change the relative links wrt to them. This check is the main part of avoiding to update the relative links.

And finally we perform the update of the link only if the doc actually exists: we would create absolute links for those not existing doc, which doesn't make sense, we should keep the relative link we don't really know what the user wanted to do with those. Note that we could do the same check in updateResourceReferenceAbsolute but we don't really have the need since this is only called from the BackLinkUpdaterListener and if I'm correct we'll never have registered backlinks for a not existing doc.

Note that initially we discussed about using untyped link as a condition to perform or not the refactoring: I dropped the idea because we currently always create image resource references as untyped references from the WYSIWYG editor.

Note that this work currently includes a known regression when renaming links on the form `page:../SubSpace/Page`, this is going to be handled in further work.

xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-test/xwiki-platform-flamingo-skin-test-docker/src/test/it/org/xwiki/flamingo/test/docker/RenamePageIT.java

@@ -366,18 +366,16 @@ void renamePageUsedInMacroContentAndParameters(TestUtils setup, TestReference te
 
         setup.rest().delete(testReference);
 
-        ViewPage standardLinkPage = setup.createPage(sourcePageReference1, "Some content to be linked. number 1");
-        ViewPage standardMacroLinkPage =
-            setup.createPage(sourcePageReference2, "Some content to be linked in macro. number 2");
-        ViewPage nestedMacroLinkPage =
-            setup.createPage(sourcePageReference3, "Some content to be linked in nested macro. number 3");
-        setup.createPage(sourcePageReference4, "A page with image to be linked. number 4");
+        setup.rest().savePage(sourcePageReference1, "Some content to be linked. number 1", "sourcePage1");
+        setup.rest().savePage(sourcePageReference2, "Some content to be linked in macro. number 2", "sourcePage2");
+        setup.rest().savePage(sourcePageReference3, "Some content to be linked in nested macro. number 3",
+            "sourcePage3");
+        setup.rest().savePage(sourcePageReference4, "A page with image to be linked. number 4", "sourcePage4");
         AttachmentsPane attachmentsPane = new AttachmentsViewPage().openAttachmentsDocExtraPane();
         File image = new File(testConfiguration.getBrowser().getTestResourcesPath(), "AttachmentIT/image.gif");
         attachmentsPane.setFileToUpload(image.getAbsolutePath());
         attachmentsPane.waitForUploadToFinish("image.gif");
-
-        ViewPage includeLinkPage = setup.createPage(sourcePageReference5, "A page to be included. number 5");
+        setup.rest().savePage(sourcePageReference5, "A page to be included. number 5", "sourcePage5");
 
         String testPageContent = "Check out this page: [[type the link label>>doc:%1$s]]\n" + "\n" + "{{warning}}\n"
             + "Withing a macro: Check out this page: [[type the link label>>doc:%2$s]]\n" + "\n" + "{{error}}\n"
@@ -390,8 +388,9 @@ void renamePageUsedInMacroContentAndParameters(TestUtils setup, TestReference te
             + "Withing a macro: Check out this page: [[type the link label>>doc:%1$s]]\n"
             + "{{/warning}}\n\n"
             + "Final line.";
-        setup.createPage(testReference,
-            String.format(testPageContent, sourcePage1, sourcePage2, sourcePage3, sourcePage4, sourcePage5));
+        setup.rest().savePage(testReference,
+            String.format(testPageContent, sourcePage1, sourcePage2, sourcePage3, sourcePage4, sourcePage5),
+            "testPage");
 
         // Wait for the solr indexing to be completed before doing any rename
         new SolrTestUtils(setup, testConfiguration.getServletEngine()).waitEmptyQueue();
@@ -511,10 +510,10 @@ void renamePageRelativeLinkPageAndTranslation(TestUtils setup, TestReference tes
 
         // Make sure the link was refactored in both the page and its translation
         Page newPage = setup.rest().get(new LocalDocumentReference(newSpace, newName));
-        assertEquals("[[" + parent + ".OtherPage.WebHome]]", newPage.getContent());
+        assertEquals("[[OtherPage]]", newPage.getContent());
 
         newPage = setup.rest().get(new LocalDocumentReference(newSpace, newName, Locale.FRENCH));
-        assertEquals("fr [[" + parent + ".OtherPage.WebHome]]", newPage.getContent());
+        assertEquals("fr [[OtherPage]]", newPage.getContent());
     }
 
     @Order(6)
@@ -668,4 +667,54 @@ void renameWithIndexingWaiting(String strategy, TestUtils setup, TestReference t
             setup.rest().delete(otherReference);
         }
     }
+
+    @Order(9)
+    @Test
+    void renameWithRelativeLinks(TestUtils testUtils, TestReference testReference, TestConfiguration testConfiguration)
+        throws Exception
+    {
+        testUtils.rest().savePage(testReference, "[[Alice]]\n[[Bob]]\n[[Eve]]", "Test relative links");
+        SpaceReference rootSpaceReference = testReference.getLastSpaceReference();
+        SpaceReference aliceSpace = new SpaceReference("Alice", rootSpaceReference);
+        testUtils.rest().savePage(new DocumentReference("WebHome", aliceSpace), "Alice page", "Alice");
+        SpaceReference bobSpace = new SpaceReference("Bob", rootSpaceReference);
+        testUtils.rest().savePage(new DocumentReference("WebHome", bobSpace), "[[Alice]]", "Bob");
+
+        // Wait for an empty queue here to ensure that the deleted page has been removed from the index and links
+        // won't be updated just because the page is still in the index.
+        new SolrTestUtils(testUtils, testConfiguration.getServletEngine()).waitEmptyQueue();
+
+        ViewPage viewPage = testUtils.gotoPage(testReference);
+        RenamePage rename = viewPage.rename();
+        rename.getDocumentPicker().setName(rootSpaceReference.getName() + "Foo");
+        CopyOrRenameOrDeleteStatusPage statusPage = rename.clickRenameButton().waitUntilFinished();
+        assertEquals("Done.", statusPage.getInfoMessage());
+
+        WikiEditPage wikiEditPage = statusPage.gotoNewPage().editWiki();
+        assertEquals("[[Alice]]\n[[Bob]]\n[[Eve]]", wikiEditPage.getContent());
+
+        SpaceReference newRootSpace =
+            new SpaceReference(rootSpaceReference.getName() + "Foo", rootSpaceReference.getParent());
+        SpaceReference newBobSpace = new SpaceReference("Bob", newRootSpace);
+        wikiEditPage = WikiEditPage.gotoPage(new DocumentReference("WebHome", newBobSpace));
+        assertEquals("[[Alice]]", wikiEditPage.getContent());
+
+        SpaceReference newAliceSpace = new SpaceReference("Alice", newRootSpace);
+        DocumentReference newAliceReference = new DocumentReference("WebHome", newAliceSpace);
+        viewPage = testUtils.gotoPage(newAliceReference);
+        rename = viewPage.rename();
+        rename.getDocumentPicker().setName("Alice2");
+        statusPage = rename.clickRenameButton().waitUntilFinished();
+        assertEquals("Done.", statusPage.getInfoMessage());
+
+        SpaceReference alice2Space = new SpaceReference("Alice2", newRootSpace);
+        DocumentReference alice2Reference = new DocumentReference("WebHome", alice2Space);
+        wikiEditPage = WikiEditPage.gotoPage(new DocumentReference("WebHome", newRootSpace));
+        String serializedAlice2Reference = testUtils.serializeLocalReference(alice2Reference);
+        assertEquals(String.format("[[%s]]%n[[Bob]]%n[[Eve]]", serializedAlice2Reference), wikiEditPage.getContent());
+
+        // FIXME: ideally this one should be refactored too, however it's not a regression.
+        //wikiEditPage = WikiEditPage.gotoPage(new DocumentReference("WebHome", newBobSpace));
+        //assertEquals(String.format("[[%s]]", serializedAlice2Reference), wikiEditPage.getContent());
+    }
 }

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/XWiki.java

@@ -115,6 +115,7 @@
 import org.xwiki.extension.job.internal.UninstallJob;
 import org.xwiki.extension.repository.CoreExtensionRepository;
 import org.xwiki.job.Job;
+import org.xwiki.job.JobContext;
 import org.xwiki.job.JobException;
 import org.xwiki.job.JobExecutor;
 import org.xwiki.job.annotation.Serializable;
@@ -151,6 +152,7 @@
 import org.xwiki.query.QueryFilter;
 import org.xwiki.refactoring.batch.BatchOperationExecutor;
 import org.xwiki.refactoring.internal.ReferenceUpdater;
+import org.xwiki.refactoring.internal.job.AbstractCopyOrMoveJob;
 import org.xwiki.rendering.async.AsyncContext;
 import org.xwiki.rendering.block.Block;
 import org.xwiki.rendering.internal.transformation.MutableRenderingContext;
@@ -4968,9 +4970,19 @@ private void updateLinksForRename(XWikiDocument sourceDoc, DocumentReference new
         List<DocumentReference> backlinkDocumentReferences, List<DocumentReference> childDocumentReferences,
         XWikiContext context) throws XWikiException
     {
+        // FIXME: that's ugly we should use something else.
+        JobContext jobContext = Utils.getComponent(JobContext.class);
+        Job currentJob = jobContext.getCurrentJob();
+
+        Map<EntityReference, EntityReference> updatedReferences =
+            Map.of(sourceDoc.getDocumentReference(), newDocumentReference);
+        if (currentJob instanceof AbstractCopyOrMoveJob) {
+            updatedReferences = ((AbstractCopyOrMoveJob) currentJob).getSelectedEntities();
+        }
         // Step 1: Refactor the relative links contained in the document to make sure they are relative to the new
         // document's location.
-        getReferenceUpdater().update(newDocumentReference, sourceDoc.getDocumentReference(), newDocumentReference);
+        getReferenceUpdater().update(newDocumentReference, sourceDoc.getDocumentReference(), newDocumentReference,
+            updatedReferences);
 
         // Step 2: For each child document, update its parent reference.
         if (childDocumentReferences != null) {

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/render/DefaultOldRendering.java

@@ -20,6 +20,7 @@
 package com.xpn.xwiki.internal.render;
 
 import java.io.StringWriter;
+import java.util.Map;
 import java.util.Set;
 
 import javax.inject.Inject;
@@ -117,7 +118,8 @@ private void refactorDocumentLinks(XWikiDocument document, DocumentReference old
         DocumentReference newDocumentReference, XWikiContext context) throws XWikiException
     {
         this.referenceRenamer.renameReferences(document.getXDOM(), document.getDocumentReference(),
-            oldDocumentReference, newDocumentReference, false);
+            oldDocumentReference, newDocumentReference, false,
+            Map.of(oldDocumentReference, newDocumentReference));
     }
 
     @Override

xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/XWikiTest.java

@@ -1093,13 +1093,14 @@ void atomicRename() throws Exception
             new DocumentReference(targetReference, Locale.GERMAN), xWikiContext);
 
         // Test links
-        verify(this.referenceUpdater).update(targetReference, sourceReference, targetReference);
+        verify(this.referenceUpdater).update(targetReference, sourceReference, targetReference,
+            Map.of(sourceReference, targetReference));
         verify(this.referenceRenamer).renameReferences(doc1.getXDOM(), reference1, sourceReference,
-            targetReference, false);
+            targetReference, false, Map.of(sourceReference, targetReference));
         verify(this.referenceRenamer).renameReferences(doc2.getXDOM(), reference2, sourceReference,
-            targetReference, false);
+            targetReference, false, Map.of(sourceReference, targetReference));
         verify(this.referenceRenamer).renameReferences(doc3.getXDOM(), reference3, sourceReference,
-            targetReference, false);
+            targetReference, false, Map.of(sourceReference, targetReference));
 
         assertTrue(this.xwiki
             .getDocument(new DocumentReference(DOCWIKI, DOCSPACE, DOCNAME), this.oldcore.getXWikiContext()).isNew());

xwiki-platform-core/xwiki-platform-refactoring/xwiki-platform-refactoring-api/src/checkstyle/checkstyle-suppressions.xml

@@ -25,5 +25,6 @@
     "http://www.puppycrawl.com/dtds/suppressions_1_0.dtd">
 
 <suppressions>
+  <!-- Fan out of 22 on a max of 20 -->
   <suppress checks="ClassFanOutComplexity" files="DefaultDocumentSplitter.java"/>
 </suppressions>

xwiki-platform-core/xwiki-platform-refactoring/xwiki-platform-refactoring-api/src/main/java/org/xwiki/refactoring/ReferenceRenamer.java

@@ -19,10 +19,14 @@
  */
 package org.xwiki.refactoring;
 
+import java.util.Map;
+
 import org.xwiki.component.annotation.Role;
 import org.xwiki.model.reference.AttachmentReference;
 import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.model.reference.EntityReference;
 import org.xwiki.rendering.block.Block;
+import org.xwiki.stability.Unstable;
 
 /**
  * Allow to replace references during rename/move refactoring operations.
@@ -46,6 +50,27 @@ public interface ReferenceRenamer
     boolean renameReferences(Block block, DocumentReference currentDocumentReference, DocumentReference oldTarget,
         DocumentReference newTarget, boolean relative);
 
+    /**
+     * Change references of the given block so that the references pointing to the old target points to the new target.
+     *
+     * @param block the {@link Block} to modify
+     * @param currentDocumentReference the current document reference
+     * @param oldTarget the previous reference of the renamed entity (attachment or document)
+     * @param newTarget the new reference of the renamed entity (attachment or document)
+     * @param relative {@code true} if the link should be serialized relatively to the current document
+     * @param updatedEntities the map of entities that are or are going to be updated: the map contains the source
+     *      and target destination.
+     * @return {@code true} if the given {@link Block} was modified
+     * @since 16.10.0RC1
+     */
+    @Unstable
+    default boolean renameReferences(Block block, DocumentReference currentDocumentReference,
+        DocumentReference oldTarget, DocumentReference newTarget, boolean relative,
+        Map<EntityReference, EntityReference> updatedEntities)
+    {
+        return renameReferences(block, currentDocumentReference, oldTarget, newTarget, relative);
+    }
+
     /**
      * Change references of the given block so that the references pointing to the old target points to the new target.
      *
@@ -63,4 +88,25 @@ default boolean renameReferences(Block block, DocumentReference currentDocumentR
         return false;
     }
 
+    /**
+     * Change references of the given block so that the references pointing to the old target points to the new target.
+     *
+     * @param block the {@link Block} to modify
+     * @param currentDocumentReference the current document reference
+     * @param oldTarget the previous reference of the renamed entity (attachment or document)
+     * @param newTarget the new reference of the renamed entity (attachment or document)
+     * @param relative {@code true} if the link should be serialized relatively to the current document
+     * @param updatedEntities the map of entities that are or are going to be updated: the map contains the source
+     *      and target destination.
+     * @return {@code true} if the given {@link Block} was modified
+     * @since 16.10.0RC1
+     */
+    @Unstable
+    default boolean renameReferences(Block block, DocumentReference currentDocumentReference,
+        AttachmentReference oldTarget, AttachmentReference newTarget, boolean relative,
+        Map<EntityReference, EntityReference> updatedEntities)
+    {
+        return renameReferences(block, currentDocumentReference, oldTarget, newTarget, relative);
+    }
+
 }

xwiki-platform-core/xwiki-platform-refactoring/xwiki-platform-refactoring-api/src/main/java/org/xwiki/refactoring/internal/ReferenceUpdater.java

@@ -19,6 +19,8 @@
  */
 package org.xwiki.refactoring.internal;
 
+import java.util.Map;
+
 import org.xwiki.component.annotation.Role;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.model.reference.EntityReference;
@@ -32,6 +34,20 @@
 @Role
 public interface ReferenceUpdater
 {
+    /**
+     * @param documentReference the reference of the document in which to update the references
+     * @param oldTargetReference the previous reference of the renamed entity
+     * @param newTargetReference the new reference of the renamed entity
+     * @param updatedEntities the map of entities that are or are going to be updated: the map contains the source
+     * and target destination.
+     * @since 16.10.0RC1
+     */
+    default void update(DocumentReference documentReference, EntityReference oldTargetReference,
+        EntityReference newTargetReference, Map<EntityReference, EntityReference> updatedEntities)
+    {
+        update(documentReference, oldTargetReference, newTargetReference);
+    }
+
     /**
      * @param documentReference the reference of the document in which to update the references
      * @param oldTargetReference the previous reference of the renamed entity