Skip to content

Commit

Permalink
XWIKI-12987: Relative links are made absolute or even broken after mo…
Browse files Browse the repository at this point in the history 
…ving a page

  * Fix integration test setup
  * Fix some signatures
  * Work on the conditions for performing link update: WIP
  • Loading branch information
@surli
surli committed Oct 23, 2024
1 parent fa0a4f9 commit 12a6e52
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 33 deletions.
37 changes: 24 additions & 13 deletions ...rm-flamingo-skin-test-docker/src/test/it/org/xwiki/flamingo/test/docker/RenamePageIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,18 +366,15 @@ 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(sourcePageReference1, "Some content to be linked. number 1");
setup.createPage(sourcePageReference2, "Some content to be linked in macro. number 2");
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");
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.createPage(sourcePageReference5, "A page to be included. number 5");

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 Down Expand Up @@ -511,10 +508,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)
Expand Down Expand Up @@ -671,21 +668,35 @@ void renameWithIndexingWaiting(String strategy, TestUtils setup, TestReference t

@Order(9)
@Test
void renameWithRelativeLinks(TestUtils testUtils, TestReference testReference)
void renameWithRelativeLinks(TestUtils testUtils, TestReference testReference, TestConfiguration testConfiguration)
throws Exception
{
testUtils.createPage(testReference, "[[Alice]]\n[[Bob]]\n[[Eve]]", "Test relative links");
testUtils.createPage(new DocumentReference("Alice", testReference.getLastSpaceReference()), "Alice page",
SpaceReference rootSpaceReference = testReference.getLastSpaceReference();
SpaceReference aliceSpace = new SpaceReference("Alice", rootSpaceReference);
testUtils.createPage(new DocumentReference("WebHome", aliceSpace), "Alice page",
"Alice");
testUtils.createPage(new DocumentReference("Bob", testReference.getLastSpaceReference()), "[[Alice]]",
SpaceReference bobSpace = new SpaceReference("Bob", rootSpaceReference);
testUtils.createPage(new DocumentReference("WebHome", bobSpace), "[[Alice]]",
"Alice");

// 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("Foo");
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());
}
}
19 changes: 15 additions & 4 deletions ...toring-default/src/main/java/org/xwiki/refactoring/internal/ResourceReferenceRenamer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ private boolean updateAbsoluteResourceReference(ResourceReference resourceRefere
linkTargetDocumentReference.equals(oldReference)
&& !(!resourceReference.isTyped() && movedDocuments.contains(linkEntityReference)
&& movedDocuments.contains(currentDocumentReference))
&& docExists;
&& (docExists || movedDocuments.contains(linkEntityReference));

// If the link targets the old (renamed) document reference, we must update it.
if (shouldBeUpdated) {
Expand Down Expand Up @@ -195,7 +195,9 @@ private <T extends EntityReference> boolean updateRelativeResourceReference(Reso
EntityReference linkEntityReference = this.entityReferenceResolver.resolve(resourceReference, null,
newReference);

if (newReference.equals(linkEntityReference.extractReference(EntityType.DOCUMENT))) {
DocumentReference documentReference =
(DocumentReference) linkEntityReference.extractReference(EntityType.DOCUMENT);
if (newReference.equals(documentReference)) {
// If the link is relative to the containing document we don't modify it
return false;
}
Expand All @@ -204,10 +206,19 @@ private <T extends EntityReference> boolean updateRelativeResourceReference(Reso
EntityReference oldLinkReference =
this.entityReferenceResolver.resolve(resourceReference, null, oldReference);

XWikiContext context = contextProvider.get();
boolean docExists = false;
try {
docExists = context.getWiki().exists(documentReference, context);
} catch (XWikiException e) {
this.logger.error("Error while checking if [{}] exists for link refactoring.", documentReference,
e);
}

boolean shouldBeUpdated =
!linkEntityReference.equals(oldLinkReference)
&& !(!resourceReference.isTyped() && movedDocuments.contains(oldLinkReference)
&& movedDocuments.contains(currentDocumentReference));
&& !(!resourceReference.isTyped() && movedDocuments.contains(documentReference))
&& (docExists || movedDocuments.contains(documentReference));

// If the new and old link references don`t match, then we must update the relative link.
if (shouldBeUpdated) {
Expand Down
15 changes: 10 additions & 5 deletions ...ude/src/main/java/org/xwiki/rendering/internal/macro/include/IncludeMacroRefactoring.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,22 +85,27 @@ public class IncludeMacroRefactoring implements MacroRefactoring

@Override
public Optional<MacroBlock> replaceReference(MacroBlock macroBlock, DocumentReference currentDocumentReference,
DocumentReference sourceReference, DocumentReference targetReference, boolean relative)
DocumentReference sourceReference, DocumentReference targetReference, boolean relative,
Set<DocumentReference> updatedDocuments)
throws MacroRefactoringException
{
return getMacroBlock(macroBlock, currentDocumentReference, sourceReference, targetReference, relative);
return getMacroBlock(macroBlock, currentDocumentReference, sourceReference, targetReference, relative,
updatedDocuments);
}

@Override
public Optional<MacroBlock> replaceReference(MacroBlock macroBlock, DocumentReference currentDocumentReference,
AttachmentReference sourceReference, AttachmentReference targetReference, boolean relative)
AttachmentReference sourceReference, AttachmentReference targetReference, boolean relative,
Set<DocumentReference> updatedDocuments)
throws MacroRefactoringException
{
return getMacroBlock(macroBlock, currentDocumentReference, sourceReference, targetReference, relative);
return getMacroBlock(macroBlock, currentDocumentReference, sourceReference, targetReference, relative,
updatedDocuments);
}

private <T extends EntityReference> Optional<MacroBlock> getMacroBlock(MacroBlock macroBlock,
DocumentReference currentDocumentReference, T sourceReference, T targetReference, boolean relative)
DocumentReference currentDocumentReference, T sourceReference, T targetReference, boolean relative,
Set<DocumentReference> updatedDocuments)
throws MacroRefactoringException
{
Optional<MacroBlock> result;
Expand Down
23 changes: 12 additions & 11 deletions ...src/test/java/org/xwiki/rendering/internal/macro/include/IncludeMacroRefactoringTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ void replaceDocumentReferenceWhenNoReferenceParameterSet() throws Exception
{
MacroBlock block = new MacroBlock("include", Collections.emptyMap(), false);
assertEquals(Optional.empty(), this.includeMacroRefactoring.replaceReference(block, null, null,
(DocumentReference) null, false));
(DocumentReference) null, false, Set.of()));
}

@Test
Expand All @@ -119,7 +119,7 @@ void replaceDocumentReferenceWhenEmptyReferenceParameterSet() throws Exception
MacroBlock block = new MacroBlock("include", Collections.emptyMap(), false);
block.setParameter("reference", "");
assertEquals(Optional.empty(), this.includeMacroRefactoring.replaceReference(block, null, null,
(DocumentReference) null, false));
(DocumentReference) null, false, Set.of()));
}

@Test
Expand All @@ -128,7 +128,7 @@ void replaceDocumentReferenceWhenEmptyPageParameterSet() throws Exception
MacroBlock block = new MacroBlock("include", Collections.emptyMap(), false);
block.setParameter("page", "");
assertEquals(Optional.empty(), this.includeMacroRefactoring.replaceReference(block, null, null,
(DocumentReference) null, false));
(DocumentReference) null, false, Set.of()));
}

@Test
Expand All @@ -153,7 +153,7 @@ void replaceDocumentReferenceWhenIncludedReferenceIsRenamedUsingReferenceParamet
new EntityReference("sourcewiki", EntityType.WIKI))).thenReturn("sourcespace.foo");

Optional<MacroBlock> result = this.includeMacroRefactoring.replaceReference(block, currentReference,
sourceReference, targetReference, false);
sourceReference, targetReference, false, Set.of());
assertFalse(result.isEmpty());
assertEquals("targetwiki:targetspace.targetfoo", result.get().getParameter("reference"));
}
Expand Down Expand Up @@ -189,7 +189,7 @@ void replaceDocumentReferenceWhenIncludingDocumentRenamedUsingReferenceParameter
new EntityReference("sourcewiki", EntityType.WIKI))).thenReturn("sourcespace.foo");

Optional<MacroBlock> result = this.includeMacroRefactoring.replaceReference(block, null,
sourceReference, targetReference, false);
sourceReference, targetReference, false, Set.of());
assertFalse(result.isEmpty());
assertEquals("sourcewiki:sourcespace.foo", result.get().getParameter("reference"));
}
Expand Down Expand Up @@ -225,7 +225,7 @@ void replaceDocumentReferenceWhenIncludingDocumentRenamedUsingPageParameterAndNo
new EntityReference("sourcewiki", EntityType.WIKI))).thenReturn("sourcespace/foo");

Optional<MacroBlock> result = this.includeMacroRefactoring.replaceReference(block, null,
sourceReference, targetReference, false);
sourceReference, targetReference, false, Set.of());
assertFalse(result.isEmpty());
assertEquals("sourcewiki:sourcespace.foo", result.get().getParameter("page"));
}
Expand Down Expand Up @@ -254,7 +254,7 @@ void replaceDocumentReferenceWhenIncludedReferenceIsRenamedUsingPageParameterAnd
new EntityReference("sourcewiki", EntityType.WIKI))).thenReturn("sourcespace/foo");

Optional<MacroBlock> result = this.includeMacroRefactoring.replaceReference(block, currentReference,
sourceReference, targetReference, false);
sourceReference, targetReference, false, Set.of());
assertFalse(result.isEmpty());
assertEquals("targetwiki:targetspace.targetfoo", result.get().getParameter("page"));
}
Expand Down Expand Up @@ -282,7 +282,7 @@ void replaceDocumentReferenceWhenIncludingDocumentIsRenamedToSameReference() thr
new EntityReference("sourcewiki", EntityType.WIKI))).thenReturn("sourcespace.foo");

Optional<MacroBlock> result = this.includeMacroRefactoring.replaceReference(block, null, sourceReference,
targetReference, false);
targetReference, false, Set.of());
assertTrue(result.isEmpty());
}

Expand Down Expand Up @@ -312,7 +312,7 @@ void replaceAttachmentReferenceWhenIncludedAttachmentReferenceIsRenamedUsingRefe
new EntityReference("sourcewiki", EntityType.WIKI))).thenReturn("sourcespace.foo");

Optional<MacroBlock> result = this.includeMacroRefactoring.replaceReference(block, currentReference,
sourceReference, targetAttachmentReference, false);
sourceReference, targetAttachmentReference, false, Set.of());
assertFalse(result.isEmpty());
assertEquals("targetwiki:targetspace.targetfoo@targetfile", result.get().getParameter("reference"));
}
Expand Down Expand Up @@ -355,7 +355,7 @@ void replaceDocumentReferenceWhenIncludingDocumentIsRenamedUsingAttachmentRefere
new EntityReference("sourcewiki", EntityType.WIKI))).thenReturn("sourcespace.foopage@foofile");

Optional<MacroBlock> result = this.includeMacroRefactoring.replaceReference(block, null,
sourceReference, targetReference, false);
sourceReference, targetReference, false, Set.of());
assertFalse(result.isEmpty());
assertEquals("sourcewiki:sourcespace.foopage@foofile", result.get().getParameter("reference"));
}
Expand All @@ -369,7 +369,8 @@ void replaceDocumentReferenceWhenParametersPopulateError() throws Exception
block.setParameter("type", "invalid");

Throwable exception = assertThrows(MacroRefactoringException.class,
() -> this.includeMacroRefactoring.replaceReference(block, null, null, (DocumentReference) null, false));
() -> this.includeMacroRefactoring.replaceReference(block, null, null, (DocumentReference) null, false,
Set.of()));
assertEquals("There's one or several invalid parameters for an [include] macro with parameters "
+ "[{type=invalid}]", exception.getMessage());
}
Expand Down

0 comments on commit 12a6e52

Please sign in to comment.