[API Design] Make LinkHandler public?

[stefanseifert]

#1818 rises the request to make LinkHandler public, and #1851 provides a PR for it. currently, the LinkHandler is not public for a good reason: it's API was a first draft, and was changed a lot during preparation of the new 3.x components which are not releases for production yet.

the question is: is the current API of the Core Components LinkHandler in a shape to make it public? keep in mind we cannot easily change it's API in a breaking way in the future once it's releases, so we should be quite sure on this. (i've already raised some design questions around the Link interface which also touches the LinkHandler #1853 #1855 #1857 #1859).

some thoughts on the current LinkHandler public API:

• property 'HTML_EXTENSION' - why is it there? for what should it by used from outside? currently it's used by DefaultPathProcessor to check if a path ends with it, which is not very elegant and will break once an URL parameter is added to it.
• properties 'PN_DISABLE_SHADOWING' and 'PROP_DISABLE_SHADOWING_DEFAULT' - does it make sense to have them in the LinkHandler API? probably they should be moved to a separate class for "cross-concern" properties which are used by content policies of multiple components.
• getLink methods return Optional of Link, which itself has a isValid method, discussed in [API Design] LinkHandler.getLink: Optional<Link> redundant to Link.isValid #1855
• currently there are already 5 overloads of getLink providing different variants by using different arguments, some of them providing varying number of string arguments. this type of API design is not very flexible when you add new feature in the future, you only can add even more arguments making code that interacts with it hard to read. probably a different approach should be used which is more flexible to be extended in the future with various features - e.g. a builder pattern?
• the resolveRedirects should not return a "Pair" object - this is very ugly as if you have the return object you always have to look up the API docs what is in left and what is in right. also to be checked
• also, `Pair' is a class of commons lang, this ties the API directly to a certain version (lang3). in general, it should be avoided to used 3rdparty classes in public APIs (except e.g. APIs from Sling, AEM itself, which are expected to be always there)
• i would recommend making an interface instead of a class. it can be backed by a model implementation separated from the interface.

[jckautzmann]

Thx @stefanseifert for your feedback about the link handler. We discussed with our team the conversations that you initiated:
#1855, #1857, #1859, #1853

We also considered the PR from @ky940819 to make the link handler an internal interface (before making it public):
#1864

We would like to proceed as follows:

1. Split the current link handler code between implementation and API as initiated by @ky940819
2. Re-define the link handler API. Check if it is possible to introduce the builder pattern and hook it into the PathProcessor API.
3. Get our Product Manager's buy-in (@gabrielwalt) to implement the changes and make it available in the 3.0.0 release.

[stefanseifert]

thanks for picking this up!

probably it would make sense to start with a PR (or discussion) for the API changes only, without the underlying implementation changes, this would make it easer to get feedback from the community focused on the API only (that will be public in future), and not mix in discussions about the implementation details.

[kwin]

Any progress here?

[jckautzmann]

Thx for your interest. We are planning to tackle this topic in the coming weeks, hopefully this month (April).

[Sai-1D]

I'd like to up vote for making LinkHandler public. There are multiple components that I'm currently developing and it would make our lives much easier if LinkHander is made a reusable service. Looking forward for this feature to be available in an upcoming release.

[jckautzmann]

We finally got some time to look into making the LinkHandler public. I came up with 2 drafts:

1. make link handler API public #2169
2. make link handler API public #2189

1. is very close to the existing LinkHandler. 2) adds a bit more flexibility to how a link is configured. Both keep the existing public Link and PathProcessor APIs unchanged.

@stefanseifert @kwin @Sai-1D @ky940819
It would be great to get your feedback, either here or directly in the PRs.

[kwin]

I am more in favour of #2169 but would get rid of Optional and also rename that to LinkFactory.
But best would be a proper LinkBuilder taking either URL, Resource or Page in the constructor and for all other optional elements have setter methods (fluent API, i.e. returning the builder) and a final build method returning the Link (never null but throw exceptions if for some reason the Link could not be constructed).

[stefanseifert]

feedback from my side:

• yes, please get rid of Optional in the LinkHandler interface because of [API Design] LinkHandler.getLink: Optional<Link> redundant to Link.isValid #1855
• make link handler API public #2169 looks simpler, but i fear it will quickly lead to much more overloads of getLink method when new features and variants are required. on the other hand, i think make link handler API public #2189 is too complicated with always having to instantiate a separate configuration object even for the simplest use case
• i would prefer to use a builder pattern instead - this keeps the code very simple for simple use cases, but allows to add any number of special use case parameters later. example: https://wcm.io/handler/link/apidocs/io/wcm/handler/link/LinkHandler.html and https://wcm.io/handler/link/apidocs/io/wcm/handler/link/LinkBuilder.html

[jckautzmann]

@kwin @stefanseifert Thx for your feedback. Based on your comments, I came up with another approach and created a dedicated PR at #2196 using the builder pattern.
cc: @vladbailescu @bpauli

[stefanseifert]

thanks!- i've added some review comments in the PR, but looks good overall

[kwin]

@stefanseifert I don’t see the comments, have you submitted the review?

[stefanseifert]

good point - submitted it now

[nehmalho]

Hi @jckautzmann - any idea by when this will be available core wcm package

[jckautzmann]

@nehmalho This is currently being worked on. I'll inform here when it becomes available.

[jckautzmann]

As it seems that we have some agreement on a new public LinkHandler (#2196), I created a pull request (#2224) with implementations for the new LinkHandler and LinkBuilder APIs.

@stefanseifert @kwin @ky940819 It would be great to get your feedback as you've been heavily involved in all the discussions about making the link handler part of the public API. Thx!