search

adobe / aem-core-wcm-components

Standardized components to build websites with AEM.
https://docs.adobe.com/content/help/en/experience-manager-core-components/using/introduction.html
Apache License 2.0
740 stars 747 forks source link

[Architecture] Use newest model implementation by default #546

Closed kwin closed 3 years ago

kwin commented 5 years ago

Feature Request

If I want to reuse a model from core wcm components in another context (i.e. not in a resource using or inheriting from core wcm component's resource types) Sling Models will pick the first implementation by default. I.e. if you use the following code snippet in HTL

<sly data-sly-use.page="com.adobe.cq.wcm.core.components.models.Page"/>
<sly data-sly-test="${page.clientLibCategoriesJsHead}"
         data-sly-call="${clientLib.js @ categories=page.clientLibJsHead}">
</sly>

which is not bound to resource type core/wcm/components/page/v2/page by default the implementation from com.adobe.cq.wcm.core.components.internal.models.v1.PageImpl is picked instead of com.adobe.cq.wcm.core.components.internal.models.v2.PageImpl

In this particular case this leads to an UnsupportedOperationException as the v1 PageImpl does not override the default impl of getClientLibCategoriesJsHead

Describe the solution you'd like I would be nice if the packages are ordered in a way so that always the newest (not the oldest) version is being picked by default by the org.apache.sling.models.impl.FirstImplementationPicker

Are there alternatives? You cannot reference the model with the implementation name i.e. com.adobe.cq.wcm.core.components.internal.models.v2.PageImpl as that is not exported.

vladbailescu commented 5 years ago

I'm assuming you have valid reasons to not want to use the v2 supertype for your component. In this case, the default implementation picker will choose the first implementation in alphabetical order (https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/FirstImplementationPicker.java#L30).

We could try to make sure all our components have a getter for the version and we could even provide our own picker that returns the implementation with highest version.

Considering a common getter for the version and a common getter for the element id (see https://github.com/adobe/aem-core-wcm-components/issues/535#issuecomment-483297494) would probably mean that we will need to define a base CoreComponent interface and have all models inherit from it.

@kwin, Are you planning a contribution for this issue?

kwin commented 5 years ago

We could try to make sure all our components have a getter for the version

That would not actually help because I would still not be able to call v2 methods!

... we could even provide our own picker that returns the implementation with highest version.

Since ImplementationPickers are global, I wouldn't really recommend that because this might break other code.

Actually I think you should rather change your package naming for the internal implementations, so that by picking the first one in alphabetical order will always be the latest version, so e.g. something like com.adobe.cq.wcm.core.components.internal.models.v1.PageImpl and com.adobe.cq.wcm.core.components.internal.models.current.v2.PageImpl (for the latest version).

vladbailescu commented 5 years ago

Having the implementation return the version number would allow reordering properly; alphabetical ordering is not a good fit (think v2 vs v10).

As for the pickers being global, we could make sure the picker falls back to the default (next service ranking) when picking implementations for classes that do not extend CoreComponent.

Your workaround would be effective too, assuming the default picker behaviour never changes.

jckautzmann commented 5 years ago

@kwin How would your work around look like when introducing 'v3.PageImpl'? If it would be as follows, it would mean renaming the 'v2.PageImpl' package, which is not ideal: com.adobe.cq.wcm.core.components.internal.models.v1.PageImpl com.adobe.cq.wcm.core.components.internal.models.v2.PageImpl com.adobe.cq.wcm.core.components.internal.models.current.v3.PageImpl

kwin commented 5 years ago

Right, when you introduce a new version, you would need to rename the old package. Hopefully that isn't an issue, because IMHO there should be no direct references to com.adobe.cq.wcm.core.components.internal.models.v1..... anywhere in your source code, or am I wrong?

Regarding:

As for the pickers being global, we could make sure the picker falls back to the default (next service ranking) when picking implementations for classes that do not extend CoreComponent

Sounds good as well.

vladbailescu commented 5 years ago

There would be some refactoring involved but it should be easily doable with modern IDEs. I would rather we first try to fix it properly and, only if not possible, keep the workaround in mind.

vladbailescu commented 5 years ago

Hm, had a look, how do you stick a new picker between https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ResourceTypeBasedResourcePicker.java#L34 and https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/FirstImplementationPicker.java#L36 ? 🙄

kwin commented 5 years ago

You could try to modify the service.ranking of the ResourceTypeBasedResourcePicker with a custom OSGi component config. I created https://issues.apache.org/jira/browse/SLING-8356 to improve the situation in the future.

vladbailescu commented 4 years ago

Doing this the proper way, with a custom picker based on component versions, would require a new version for each component.