XWIKI-22681: Provide a relative ResourceReferenceEntityReferenceResolver #3673
Conversation
| // TODO: check that this assert is ok, we don't get the default page since we don't have any info to properly | ||
| // resolve the absolute ref (no wiki, no base reference) | ||
| assertEquals(PAGE_ENTITY_REFERENCE, | ||
| this.resolver.resolve(documentResource(SPACE + '.' + PAGE, false), null)); |
There was a problem hiding this comment.
This assertion is probably the first one we should ensure we agree on.
There was a problem hiding this comment.
This does not feel right: when no specific target can be found, an untyped reference is supposed to target a WebHome by default. You don't really need a base reference to know that.
Alternatively, you could decide to return an entity reference of type PAGE, in which case you don't need to worry about WebHome. Unfortunately, it would probably be a bit dangerous to change this behavior to have the same logic in the absolute resolver.
There was a problem hiding this comment.
This does feel right: when no specific target can be found, an untyped reference is supposed to target a WebHome by default. You don't really need a base reference to know that.
So it does not feel right?
There was a problem hiding this comment.
So it does not feel right?
Yes, sorry, I fixed the comment.
| } | ||
|
|
||
| @Override | ||
| protected EntityReference resolveUntyped(ResourceReference resourceReference, EntityReference baseReference) |
There was a problem hiding this comment.
Looks a lot like DocumentResourceReferenceEntityReferenceResolver#resolveUntyped. It would be better to improve code reuse.
| } | ||
|
|
||
| @Override | ||
| public EntityReference resolve(ResourceReference resourceReference, EntityType entityType, Object... parameters) |
There was a problem hiding this comment.
This seems to be almost an exact duplicate of the AbstractResourceReferenceEntityReferenceResolver version. I'm sure code reuse can be improved.
The point of convert (which removing it seems to be the reason why you did this whole duplicate) is not to resolve an absolute reference, it's to convert a reference of the DOCUMENT world from/into a reference of the PAGE world, depending of which type was passed to #resolve.
|
The relations between the relative and absolute versions of |
| // resolve the absolute ref (no wiki, no base reference) | ||
| assertEquals(PAGE_ENTITY_REFERENCE, | ||
| this.resolver.resolve(documentResource(SPACE + '.' + PAGE, false), null)); | ||
| assertEquals(PAGE_ALONE_ENTITY_REFERENCE, |
There was a problem hiding this comment.
Now, one problem is that adding a WebHome in the result of that example where you have no space is that it completely change the behavior of the relative -> absolute resolution. As soon as a reference contains a space, it's considered "absolute" in a given wiki, which is not the intent here since the "Page" reference is supposed to be relative to the current space.
There was a problem hiding this comment.
it completely change the behavior of the relative -> absolute resolution
I'm trying to understand what you mean exactly here. My understanding is that you mean there's an inconsistency between:
- Resolving a ResourceReference
Space.WebHometo an absolute EntityReference which would result incurrentwiki:Space.WebHome - Resolving a ResourceReference
Space.WebHometo a relative EntityReference which would result inSpace.WebHomeand then resolving that one to an absolute EntityReference which would result indefaultwiki:Space.WebHome
Am I understanding it correctly?
There was a problem hiding this comment.
I think you understood, but your examples are not accurate since the ResourceReference is Page (Space.WebHome would not be resolved as currentwiki.CurrentSpace.Space.WebHome).
There was a problem hiding this comment.
I edited my comment to fix the example.
So ok now I think I understand what you were saying last time. We have two solutions here:
- We always perform the "absolute" resolution, meaning what I'm doing here is useless basically: the resolver would never return a relative reference for Document type at least
- We put an info in the EntityReference that it's relative and that's consumer job of the API to check if the EntityReference is relative or not and to use the proper resolver (current or default), we could probably directly also implement the logic of checking that info.
I think I prefer option 2 even if it's more work, as we provide a true meaning of relative entity reference. If we go for 1 IMO it's almost as if we would never really consider relative references...
There was a problem hiding this comment.
There's actually a 3rd solution: we accept the inconsistency of getting different results, depending if you resolved directly the ResourceReference to absolute or if you resolved it first to relative and then the EntityReference to absolute
Jira URL
https://jira.xwiki.org/browse/XWIKI-22681
TODO
Changes
Description
Clarifications
Screenshots & Video
Executed Tests
Run
mvn clean install -Pqualityon modulexwiki-platform-rendering-xwiki. Now this component implem is used for #3654 so integration tests are executed in that one.Expected merging strategy