[Link Handling] API Design

[Stefan Seifert]

thanks for picking up the link handling and finally bringing it into the next versions of components! - unfortunately I had very little time the last months and could not help much. looking at what is currently in develop branch I've some remarks on the current API design:

1. LinkHandler.getLink: Optional<Link> redundant to Link.isValid

that is redundant and we should use only one of the concepts:
- having the linkhandler returning an empty optional element to signal an invalid link, removing the isValid method
- or having the linkhandler return and link without optional that is never null, and that can be check via isValid method
- furthermore the model interfaces all just define a @Nullable Link - unclear whether invalid means null, or not null with isValid=false
with the current state it's quite confusing. there is already code where links are consumed always checking both !=null && isValid which is very ugly.


2. Link<?> generic signature

i currently see no benefit with the concept of having a generic signature like Link<Page> or Link<Asset>. in theory it makes sense, in practice not.
whenever you consume a link object from link handler or from an model interface, you have no idea on what item the link really points to when you declare your variable, and you can only use Link<?> or Link. same applies in model interfaces that return links. you only know what's in it after you actually got the link instance. this makes using link objects very clumsy, and you already see code scattered with @SuppressWarnings("rawtypes") to get rid of related warnings.
the only place where you are sure what the link is pointing at is during link building/handling, but this piece of code is inside the link handling and not relevant for the users of the API.
i would propose to drop this generic concept completely.


3. Download component without getLink method

the download component model seems to be the only model that was not switched to the new Link concept. is this component obsolete, or is it an oversight?


4. Link with separate getURL/getMappedURL/getExternalizedURL methods

i find it a bit confusing as well what's to do with those three methods. the javadocs are clear, but I still suppose most users who "just want a link" will be puzzled which is the correct one for their HTL script, and in a big project with lots of developers every developer will use a different method leading to much inconsistency. in wcm.io Link handler/URL handler for comparison we have also concepts for influencing if links should be externalized or not (avoiding the ill-designed com.day.cq.commons.Externalizer altogether), but this are only optional aspects which can be activated during link resolution with special builder methods, and there is always a sensible default handling which is the right thing to use in 95% of the cases.
providing access to these special case methods in this three variants of URLs may lead to confusion and inconsistencies.


sorry for bringing this up quite late in the game - although 3.0.0 is not release officially most of it is already baked in the public APIs that are released and probably cannot be changed any more.

stefan

[kyl...@gmail.com]

Hello, 

I also share most of these concerns. 

1) 

I think the problem with the @Nullable annotations is because most model interfaces return null in the default implementation, which then requires @Nullable to be used. 
I haven't looked at this specific code in awhile, but any method that returns an Optional should be annotated @NotNull, and a Optional.empty() should be returned it default implementations. 
Sorry if that isn't relevant - again, I haven't spent much time digging into this lately. 

2) 
My interpretation for the generic is so that the link can, optionally, provide a back-reference to the thing it is linking to. 
I'm not really in favour of this design because I feel like a link is a link - if you want a Page, Asset, or, Resource, then there should be a getter in the model to provide that. 
As it stands now, since that field is entirely optional, it may be used in ways that are inconsistent. I would drop this concept completely as well and just make links links, and what they link to is irrelevant once the link is created, all necessary link information is part of the link object, there is no need to keep references to these other things - other than trying to overload the concept.
This also causes confusion for front-end developers because they would need to know implementation specifics to be able to make use that.

3)
Download seems like exactly the sort of thing that is a link to me?

4)

I would drop "externalizer" entirely and rely on mapping. 
If there is some super important use-case for retaining the ability to externalize the link using the externalizer then you _could_ do something like 
#getExternalURL() -> use mapping

#getExternalURL(EXTERNALIZER_FUNCTION)

where EXTERNALIZER_FUNCTION could be any function that can be applied to a path to externalize it. For convenience there could be static functions like "Externalizers.MAPPING" and "Externalizers.EXTERNALIZER", such that:

getMappedURL() ==  getExternalURL(Externalizers.MAPPING)

getExternalURL() ==  getExternalURL(Externalizers.EXTERNALIZER). 

but in practice I would actually remove these get* methods and just replace it with one that uses sling mappings by default. This way it's clean, not confusing, and if you go out of your way to override the externalizing behaviour then it is very clear that you have done so - it's not the result of an accident. 

Additionally, I recently tried to update some custom models that meet the model interfaces but do not use model delegation because they share nothing in common with the CC model implementations other than the same interface. 
I have noticed that a lot of the link builder utilities are not exported from the bundle, which makes it very difficult to create custom models that are compliant with the interfaces.

Awhile ago I created a branch that made the LinkHandler a public interface and then reworked the model to implement that interface such that it could could be used outside of the CC project. It's outdated now so I never sent a PR. 
If it's something that is of interest then I would be happy to make a ticket and submit a PR:
https://github.com/ky940819/aem-core-wcm-components/commit/39efe45d6f667d68eeddac5533efa5bdb4187578

I also question if the LinkHandler is actually a model? I am aware that it _is_ implemented as a sling model, It just feels like a service to me. 

If you look at the above commit, you will notice that I changed the LinkHandlerImpl model to use Constructor Injection. 
The intention behind that was to test out creating a LinkHandlerFactory service from which you could acquire link handlers -- Not being *required* to follow the model injection makes that totally possible.

It would end up as LinkHandler linkHandler = LinkHandlerFactory.getLinkHandler(currentRequest).

This has the side benefit of being able to be used in other services without a model. 

Kyle

[Stefan Seifert]

it seems this mailing list is no longer in use - i've created 4 items in GitHub discussions instead:

1. -> https://github.com/adobe/aem-core-wcm-components/discussions/1855
2. -> https://github.com/adobe/aem-core-wcm-components/discussions/1853
3. -> https://github.com/adobe/aem-core-wcm-components/discussions/1859
4. -> https://github.com/adobe/aem-core-wcm-components/discussions/1857

stefan

>--
>You received this message because you are subscribed to the Google Groups
>"AEM Core Components Developer Community" group.
>To unsubscribe from this group and stop receiving emails from it, send an
>email to aem-core-componen...@googlegroups.com.
>To view this discussion on the web visit
>https://groups.google.com/d/msgid/aem-core-components-
>dev/AS8PR07MB7238137D48504B293171B7F1B8C89%40AS8PR07MB7238.eurprd07.prod.ou
>tlook.com.