Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWIKI-12987: Relative links are made absolute or even broken after moving a page #3553

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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();
Expand Down Expand Up @@ -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());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not good anymore: the OtherPage doesn't exist so we just check that the unexisting relative link is not updated. But we don't check anymore that link are refactored in a translation page. We need to improve it.

}

@Order(6)
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed anymore with XWIKI-22323.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually call it to avoid getting a question when performing the rename operation.

Copy link
Member

@tmortagne tmortagne Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is supposed to automatically vanish (it's not a blocker question) as soon as the job is done AFAIU.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well what's sure is that I'm getting a timeout when I remove this and the screenshot shows the question...

Copy link
Member

@tmortagne tmortagne Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to update the PO to support non-blocking questions (maybe it does not find what it expects to see to check if the job is still running when a question is displayed).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is more that indexing takes quite some time, in particular with the frequent committing of the index with the default parameters, and the page object most likely just doesn't wait long enough. I have an integration test that explicitly triggers the question and there it works to just continue waiting without answering the question.


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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmortagne for that one you suggested:

But I'm not sure it's a good thing to do, might be cleaner to introduce something more genereric in the ExecutionContext that would be populated by jobs like the move job, etc.

wdyt, the PR is already quite complex, isn't it something we can address later?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it's not a blocker, just a cleaner thing to do eventually for the long run. As long as we don't introduce public API for the current version, it's not adding too much debt.

// 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
*
Expand All @@ -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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
Loading