Hi, On Tue, Feb 2, 2016 at 3:06 PM, vinc...@massol.net <vinc...@massol.net> wrote:
> Hi Edy, > > On 2 Feb 2016 at 13:34:49, Eduard Moraru (enygma2...@gmail.com(mailto: > enygma2...@gmail.com)) wrote: > > > Hi, > > > > So after an initial implementation we realized there are some issues with > > the original approach. > > > > Basically, it is not such a good idea to handle the resolving of untyped > > links at the parser level because this is not compatible with the XDOM > > caching that we have for XWikiDocuments, i.e. it`s not OK to cache a > > document's XDOM that was resolved with links in the context of the > *first* > > document that resolved them and then to reuse that cached XDOM later or > > from a different calling document when/where those links no longer make > > sense. We would need to remove the XDOM caching and parse the XDOM every > > time we need it in order to make sure that the links are resolved in the > > context of the current calling document and that they are the correct > > links. Obviously, we don`t want that and we want to keep the XDOM cache > > which is vital for performance. > > > > In order to address this issue and to still be able to do the fallback > > mechanism (the one that allows us to be backwards compatible at the > syntax > > level when resolving an untyped link), Thomas suggested that we always > > produce a static (cacheable/reusable) parsing result (i.e. URL or > DOCUMENT > > types, with the DOCUMENT ResourceReference having the isTyped flag set to > > false) for untyped links and that the we introduce a new set of > > EntityReferenceResolver API to be used on the client > > side (i.e. when consuming the XDOM) to get the actual XWiki entities. > This > > would be used, for example, by the XWikiWikiModel when producing the URLs > > and any other places that use DOCUMENT type ResourceReferences. So we > pass > > the resolving responsibility to this new API and expect the client to use > > it. > > > > This second proposal resolves the caching issue but introduces new > > requirements on the clients of the rendering result, so in terms of > > backwards compatibility, if the clients don`t use this new resolvers API, > > some links might be interpreted incorrectly (i.e. assuming it's a > document > > instead of a space reference). This adds to the backwards compatibility > > issues caused by the addition of the new SPACE link type, but there is > not > > much way around it. > > I agree that the XDOM returned by XWikiDocument.getXDOM() should contain > exactly what’s been parsed from the syntax without any interpretation. > > We could imagine caching the XDOM along with the Context but it doesn’t > make much sense IMO. I find it better that it’s evaluated at the time it’s > needed with the context at that time. > Yes, and it also increases the potential size of the cache to all possible combinations of (caller, cached XDOM) pairs. > You propose to provide an Entity Reference Resolver to allow users of > getXDOM() to parse references. So I guess the users would check if the > ResourceReference is of type Attachment, Document or Space and then use > that resolver to convert the String into an EntityResourceReference. > We were going directly for EntityResource and want to reuse the EntityResourceResolver<ResourceReference> API. Do you see any issue with that? > > Another idea (don’t know if you thought about it) would be to implement a > Transformation that would take that XDOM and generate a XDOM’ that is > resolved against the current Context (with all the links resolved). I guess > there could be use cases for both needs but having the fine-grained > solution is better to start with. > Interesting idea. Haven`t thought about it, but, as you also said, I find the fine grained solution to be better since it also helps with attachments, not only space/document links, since for attachments you would still have to apply the manual/fine-grained solution. > > I`d also like to mention that we are already doing something similar to > > this when resolving attachment/image references (i.e. the caller needs to > > resolve the reference since the rendering does not distinguish between > > "space attachments" or "document attachments") so this new resolver API > is > > a good/needed addition anyway and expanding this approach to links makes > > sense, IMO. > > I don’t understand "since the rendering does not distinguish between > "space attachments" or "document attachments””, could you provide some > example in wiki syntax? > Yes, I should have given some examples here. What I call "space attachment" is "sp...@file.ext" (without "WebHome"), while a "document attachment" would be "space....@file.ext" or even "space.webh...@file.ext". We have no explicit syntax to differentiate between them (like we have for doc: and space:) so a client would have to resolve the reference by himself. Currently, XWikiWikiModel resolves it an produces the correct attachment URL. This is where the above resolvers come very useful. > > > Additional note: typed links ([[doc:Doc]], [[space:Space]] etc.) are > still > > supported and not affected by this change of direction, since this is > only > > about (untyped [[Doc]]) links. > > > > We`re starting work on this direction. > > > > Please let us know what you think about this new approach and if you see > > any use cases where it creates problems. > > Sounds good to me. > > Of course we’ll need to document this in the release notes. > > I’ve done a quick check of usages of LinkBlock.getReference() since this > is where the problem could happen. I’ve found the following places to check: > - XWikiDocument.getUniqueLinkedPages() > - DefaultLinkedResourceHelper > - PlainTextBlockFilter > > Note that the LinkCheckerTransformation is not affected since it only > checks URL-type links. > > There are also some matches in xwiki-contrib to check: > > https://github.com/search?q=LinkBlock+user%3Axwiki-contrib&ref=searchresults&type=Code&utf8=%E2%9C%93 > > Cool, I`ll look there too. Thanks, Eduard > Thanks! > -Vincent > > PS: Good catch Thomas about this! > > > > Thanks, > > Eduard > > > > On Thu, Jan 7, 2016 at 2:03 PM, Eduard Moraru wrote: > > > > > Hi Thomas, > > > > > > On Wed, Jan 6, 2016 at 7:42 PM, Thomas Mortagne > > > wrote: > > > > > >> On Wed, Jan 6, 2016 at 5:57 PM, Eduard Moraru > > >> wrote: > > >> > Hi devs, > > >> > > > >> > As XWIKI-12920 [1] mentions, we need to hide "WebHome" references > in the > > >> > wiki syntax for links and images (for now) in order to be consistent > > >> with > > >> > what we did in the UI. > > >> > > > >> > What this means is that [[label>>Page.WebHome]] should be > equivalent to > > >> > [[label>>Page]]. > > >> > > > >> > The plan is to apply the same logic as we did for URLs and detailed > by > > >> > Vincent in "Solution 2" in his comment on the issue [2]. > > >> > > > >> > In order to achieve this, Vincent proposed to introduce a new > "space:" > > >> > resource type and make this new type the default instead of "doc:" > > >> which is > > >> > the current default. > > >> > > > >> > Before: > > >> > [[label>>Page]] => [[label>>doc:Page]] > > >> > After: > > >> > [[label>>Page]] => [[label>>space:Page]] if "Page.WebHome" exists, > > >> > or => [[label>>doc:Page]] if > > >> ".Page" > > >> > exists, > > >> > or => wanted link to create "Page.WebHome" when > > >> > clicked. > > >> > > > >> > Using either the "doc:" or the "space:" versions manually will > resolve > > >> the > > >> > requested resource, without doing any fallback resolution (which is > > >> applied > > >> > only for the no-prefix version). > > >> > > > >> > > > >> > For the same consistency reason, we need to apply the same fallback > to > > >> the > > >> > "attach:" resource type, e.g: > > >> > [[attach:p...@file.ext]] => [[label>>attach:space:p...@file.ext]] > || > > >> > [[label>>attach:doc:p...@file.ext]] > > >> > > > >> > However, a resource is defined as ((type:) resource) so for > attachments > > >> we > > >> > would need to extend the type's definition to allow it to contain > ":" in > > >> > the type name (i.e. "attach:doc" and "attach:space") so that more > > >> > variations are tested when resolving a link reference before > treating > > >> it as > > >> > a generic/untyped reference (this is where the fallback mechanism > kicks > > >> in). > > >> > > > >> > > > >> > There is also the image syntax that needs to be extended to support > the > > >> > same fallback logic, so something like: > > >> > image:Page => image:space:Page || image:doc:Page > > >> > > > >> > > > >> > In all these cases: > > >> > * link space: prefix, > > >> > * link attach:doc and attach:space prefixes and > > >> > * image space: and doc: prefix > > >> > ... we would be breaking backwards compatibility in the sense that > if a > > >> > wiki with the name "space" (and/or "doc", depending on which case > of the > > >> > above you are) already existed in the wiki farm, any links pointing > to > > >> > documents in that wiki from other wikis will be broken, because, for > > >> > example [[label>>space:Page]] no longer points to the document > > >> > "space:Page.WebHome" (from wiki "space"), but to the document > > >> > "Page.WebHome" from the current wiki (where the link is made). To > fix > > >> this, > > >> > one would have to write [[label>>space:space:Page]] instead. > > >> > > >> "[[label>>space:Page]]" does not mean "space:Page.WebHome" right now > > >> but ":.space:Page" > > >> > > > > > > You are right. Bad example (though I find that behavior a bit odd). It > > > should have been: > > > > > > [[label>>space:Space.Page]] no longer points to the document > > > "space:Space.Page" (from wiki "space"), but to the document > > > "Space.Page.WebHome" from the current wiki (where the link is made). > To fix > > > this, one would have to write [[label>>space:space:Space.Page]] > instead. > > > > > > Thanks, > > > Eduard > > > > > > > > >> > > > >> > I guess we could/should write a migrator to try to fix most of these > > >> cases > > >> > automatically (like we fix links to a document that was renamed), > but a > > >> > couple of links will be unfixable if they are built programatically > > >> (e.g. > > >> > by velocity scripts) and the process could prove to be extremely > lengthy > > >> > one (due to the parsing, processing, serialization and resaving of > each > > >> > document). > > >> > > > >> > > > >> > Please feel free to comment on the above described approach. > > >> > > > >> > Thanks, > > >> > Eduard > > >> > > > >> > P.S.: I did not mention macro parameter references which would also > > >> need a > > >> > solution for hiding the "WebHome" part, but I`d prefer it if we > handle > > >> that > > >> > another time / in another thread. > > >> > > > >> > ---------- > > >> > [1] http://jira.xwiki.org/browse/XWIKI-12920 > > >> > [2] > > >> > > > >> > http://jira.xwiki.org/browse/XWIKI-12920?focusedCommentId=89129&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-89129 > _______________________________________________ > devs mailing list > devs@xwiki.org > http://lists.xwiki.org/mailman/listinfo/devs > _______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
