-
-
Notifications
You must be signed in to change notification settings - Fork 550
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 #3634
Merged
+934
−225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
surli
requested review from
tmortagne and
michitux
and removed request for
tmortagne
November 8, 2024 14:09
Original PR created in #3553 and recreated here after renaming the branch for CI. |
surli
force-pushed
the
feature-deploy-refactor-links
branch
from
November 8, 2024 14:56
1d8b51b
to
7a9cdfe
Compare
…ving a page WIP The idea of this work is to: 1. Provide a way to access all documents that are moved as part of a move job 2. Use that information when performing a call to ReferenceRenamer to define if a relative untyped link should be handled or not On top of it, the idea is also to check if the doc exists in case of refactoring of a link to avoid refactoring unexisting relative links. One problem is remaining about relative link pointing to sibling pages (e.g. the link to Alice in Bob page in the ticket): we rely apparently to an old mechanism for backward compatibility reason for this to work in the UI, we might need same thing in the check, or to decide to ignore that UC. I started to add an integration tests but for some reason it's not passing, though it seemed to be working locally for the scenario described in the ticket (except for the link in Bob page).
…ving a page * Fix integration test setup * Fix some signatures * Work on the conditions for performing link update: WIP
…ving a page * Fix conditions to make all RenamePageIT passing * WIP: need to double check that some conditions are not redundant and double check side effects
…ving a page * Simplify a bit the conditions in ResourceReferenceRenamer and ensure all unit tests are passing in refactoring module
…ving a page * Fix checkstyle * WIP: try to find proper oracle for renaming absolute references, without success so far.
…ving a page * Find proper conditions to perform or not link renames * Fix unit tests to add missing conditions * WIP: need to fix coverage and check on subwikis / with more conditions (e.g. with holes in hierarchy)
…ving a page * Fix a regression and provide a test to cover it
…ving a page * Provide subwiki integration tests * Minor improvment in RenamePageIT
…ving a page * Improve SubWikiIT to add more checks
…ving a page * Few improvments following review
…ving a page * Change APIs to use a Map<EntityReference, EntityReference> corresponding to the source and target of refactorings in renamers * Change some logic of AbstractCopyOrMoveJob to compute the actual couple source/destination before performing any operation and store the info in EntitySelection * Add a log in RenameJob if it's not executed because of the number of entities (not needed for this issue, but felt better to understand what's happening)
…ving a page * Fix remaining coverage problems
surli
force-pushed
the
feature-deploy-refactor-links
branch
from
November 12, 2024 07:31
7a9cdfe
to
c9be265
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira URL
https://jira.xwiki.org/browse/XWIKI-12987
TODO
Changes
Description
The idea of this work is to:
AbstractCopyOrMoveJob
works to perform computation of couple source/target documents before processing themReferenceRenamer
to define if a relative untyped link should be handled or notThe PR provides mainly:
ReferenceRenamer
andMacroRefactoring
to integrate the map of references that have been moved as part of same jobAbstractCopyOrMoveJob
:getEntities
to actually visit the hierarchy and populate the entities with the couple of source/target documentsResourceReferenceRenamer
to decide if a link should be renamed or not: most of the logic of the fix is encoded there (see also clarifications)XWiki#updateLinksForRename
andBackLinkUpdaterListener#updateBackLinks
to give the map of source/target when calling the rename of referencesSubWikiIT
Clarifications
The refactoring of references is currently called at two places:
BackLinkUpdaterListener
for all backlinks after a document has been renamed (triggered by a document event)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
andupdateResourceReferenceAbsolute
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 toXWiki#updateLinksForRename
on that document to update the link, we only get the call fromBackLinkUpdaterListener
.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.
Screenshots & Video
Tested and supported UCs
In RenamePageIT:
[[1.2.WebHome]]
and1.2.WebHome
full hierarchy is renamed toA.B
.[[Alice]]
) we check that those links are not updated if the whole hierarchy is moved. Test also performed when moving on a subwiki.TODO:
Executed Tests
Run of tests on following modules / integration test:
Expected merging strategy
*