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
744 stars 752 forks source link

[Architecture] Model classes should support adaptation from Resource #467

Open HitmanInWis opened 5 years ago

HitmanInWis commented 5 years ago

Feature Request

Looking through WCM Core components, I see that almost all of them are coded to support adaptation from only a SlingHttpServletRequest.class, and not a Resource.class. This means that in the context of an OSGi service that runs outside the context of a sling request (event listeners, scheduled jobs, data pollers, etc.), these model classes cannot be used.

I understand that model classes often have some functionality that depends on a sling request, but often much of the functionality including reading properties from the JCR does not. As long as a model is coded to allow an injected request object to be optional, those models can still be very useful in back end OSGi code.

Can someone explain to me why we are limiting WCM Core components to only be used in the context of a web request? Or would people be open to the enhancement of allowing WCM Core models to be adapted from a straight Resource as well?

jckautzmann commented 5 years ago

hi @HitmanInWis , thx for your feedback. The models of the Core Components are meant to be used to render within a page. We assume a page request context. We didn’t design them to be adapted from a Resource. There are a few objects that are only available within a page request context, e.g. currentStyle. Updating the models to be adaptable from a Resource would add some complexity to the model implementations as we would have to retrieve objects that are not available with the resource context like the page, template, policy, etc.

HitmanInWis commented 5 years ago

I understand, but some of this stuff can be handled generically in a base model class if we need to. For example, we could define Page currentPage and in the context of a request grab it from @ScriptVariable but if not then traverse up the tree to grab a page.

1) There's no hit to performance in the use case that involves a request, since it still grabs it from a request. 2) Honestly there's no noticeable hit to performance in the case of a resource either, in my experience, but even if there were it would be limited to that use case.

A component is an entity that exists outside the context of a HTTP request, so limiting the sling models to only working within the context of a HTTP request feels like we've tightly bound two things that shouldnt be. I'm not advocating we take all request-related logic out of sling models, but I feel that the parts of a sling model that do not require a request should be able to work without it.

jckautzmann commented 5 years ago

I agree that this would be feasible but it requires some effort. Do you have a specific use case or a customer/project need that would benefit from it? Could you share more details?

HitmanInWis commented 5 years ago

Here are some use cases where having sling models support Resource allows devs to leverage them outside the context of a request.

HitmanInWis commented 5 years ago

Also note https://issues.apache.org/jira/browse/SLING-8279 - if this were to be addressed, we could actually have models adapt primarily from a Resource and this issue would be a lot simpler to address.

kwin commented 5 years ago

For the child resources injector I think it makes sense to extend that. I started a discussion in https://www.mail-archive.com/dev@sling.apache.org/msg83942.html.

HitmanInWis commented 5 years ago

FWIW @adamcin and I have already coded up a new @ChildRequest (child-requests) annotation that mirrors @ChildResource but acts upon a Sling Request (wrapped to point at the path of the requested children), complete with fallback to Resource adaptation if the adaptable is not a Sling Request. I was planning on potential submission to ACS AEM Commons, but I could see leveraging the code to instead extend Sling directly.

jckautzmann commented 5 years ago

@HitmanInWis Thx for the list of use cases. Once Sling is extended to facilitate adaption from a Resource, you're welcome to submit a pull request to make the Core Components models adapt from a Resource.

HitmanInWis commented 5 years ago

Sling already facilitates adaption from a Resource (i.e. @ChildResource). Or did you mean when it supports adaption from a Request?

jckautzmann commented 5 years ago

@HitmanInWis yes sorry I got confused with the Sling issue posted above. If nothing in Sling stands in the way, you're welcome to submit a pull request to make the Core Components models adapt from a Resource.