Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
HitmanInWis opened this issue Feb 18, 2019 · 10 comments
Open
Labels
enhancement New feature, or improvement to an existing feature.
Milestone

Comments

@HitmanInWis
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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?

@richardhand richardhand added the enhancement New feature, or improvement to an existing feature. label Feb 20, 2019
@HitmanInWis
Copy link
Author

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

  • Any use of the @ChildResource annotation to directly inject sling models for a single child or list of children of a parent (e.g. injecting a SocialLink model inside a SocialLinks component)
  • Page listing component that wants to render a custom "Author" field for page, but the sling model that determines the author includes logic to traverse the page tree upward to a section landing page that includes the referenced author.
  • Locations listing page that is grabbing a "Location Info" component from all location details pages (yes I know this can be solved by abstracting Location Info to a structured content fragment, but not all companies want the extra abstraction)
  • Teaser component that wants to access a custom component on the referenced page to augment the teaser (especially if the sling model contains logic for the data included in that component)

@HitmanInWis
Copy link
Author

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
Copy link
Contributor

kwin commented Feb 25, 2019

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

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

@jckautzmann
Copy link
Contributor

@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.

@gabrielwalt gabrielwalt changed the title Model classes should support adaptation from Resource [Architecture] Model classes should support adaptation from Resource Jul 2, 2019
@gabrielwalt gabrielwalt added this to the Future milestone Jul 2, 2019
@gabrielwalt gabrielwalt modified the milestones: Future, 3.0.0 Aug 31, 2020
@vladbailescu vladbailescu modified the milestones: 3.0.0, Major Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, or improvement to an existing feature.
Projects
None yet
Development

No branches or pull requests

6 participants