@ChildResourceFromRequest uses incomplete request wrapper

[HitmanInWis]

@ChildResourceFromRequestInjector leverages a request wrapper class com.adobe.acs.commons.util.OverridePathSlingRequestWrapper in order to inject child resources as sling models, leveraging a Request object rather than only a Resource (since many sling models require a request). However, the sling bindings provided by OverridePathSlingRequestWrapper are incomplete. As an example, the currentStyle binding will be that of the parent resource (the original resource of the original request) rather than the child resource being adapted, which breaks functionality that depends on design (editable template) configs.

Looking at the sling version included with AEM 6.4+, a very similar class to OverridePathSlingRequestWrapper has been implemented at org.apache.sling.models.impl.ResourceOverridingRequestWrapper which has more complete sling bindings support. This class is internal, and cannot be directly referenced, but it can be leveraged via org.apache.sling.models.impl.ModelAdapterFactory#getModelFromWrappedRequest which we could then use from @ChildResourceFromRequest.

However, I'm struggling on best path forward on this, due to the following factors:

1. The improved direct support in sling via ModelAdapterFactory#getModelFromWrappedRequest is not available to AEM 6.3, which ACS Commons still supports.
2. The change alters the functionality of @ChildResourceFromRequest. In some ways this can be considered a bug fix, but there might be existing code that depends on that bug.
3. It's possible there's code out there in the wild that specifically uses OverridePathSlingRequestWrapper

We can solve issue 1 by either copying the code in the new sling class to OverridePathSlingRequestWrapper or by waiting until ACS Commons drops support of AEM 6.3. However, that still leaves issues 2 and 3 above.

Any thoughts on the appropriate solve?

[davidjgonzalez]

2. I read this as a bug in ChildResourceFromRequestInjector, unless there was some communicated intent somewhere. That said in the context of OverridePathSlingRequestWrapper it might not be so clear.
3. To combat the ambiguity in 2, and possible uses of 3, could we introduce a new "correct" class OverridePathSlingRequestWrapperTheSequel(better name of course ;)) and fix it there, and leave the current OverridePathSlingRequestWrapper alone?

[HitmanInWis]

For 3 we certainly could, and deprecate the old one. Feels a bit messy given we'll have 3 versions (2 in ACS Commons, and 1 in Sling). However, so be it :)

I guess my question would be whether we think its more appropriate to fix both the same way, or if we think there's good argument to fix 2 directly and 3 via a new class.

[davidjgonzalez]

@HitmanInWis I run into problems wrapping or creating synthetic sling requests/responses more often than you'd imagine, and its a real PITA when you have to. I'm not sure if Sling's servlet helpers[1] could help here [1] .. though we'd have a dependency on a "third party" library, which is ~against ACS Commons tenants.. which is problematic.

Agree it's not the most elegant solution, but our hands feel a bit tied with that specific class being a Sling impl, and ACS Commons not shipping w/ 3rd party libs (sling servlet helpers, which may not even do the trick).

We can see what others think tho, for sure

[1] https://sling.apache.org/documentation/bundles/servlet-helpers.html

[joerghoh]

2163 could also benefit from a better SyntheticRequest/Response implementation.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[HitmanInWis]

This is still on my list...just gotta get to it...

[HitmanInWis]

Turns out I dont need to deprecate the old OverridePathSlingRequestWrapper necessarily, since it may still be used in cases where you're not adapting to a sling model. I've added a new ctor to OverridePathSlingRequestWrapper to allow for the additional sling model bindings, and updated @ChildResourceFromRequest to use the new ctor.