You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sling.apache.org by Justin Edelson <ju...@justinedelson.com> on 2016/05/13 10:45:17 UTC

Re: svn commit: r1743648 - in /sling: site/trunk/content/documentation/bundles/ trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ trun...

-1 to this change (would comment in JIRA but I can't for some reason right
now)

This adds unnecessary complexity and is poor seperation of concerns. The
@Via annotation can be used for this use case (and is documented as such).
On Fri, May 13, 2016 at 11:35 AM <kw...@apache.org> wrote:

> Author: kwin
> Date: Fri May 13 10:35:03 2016
> New Revision: 1743648
>
> URL: http://svn.apache.org/viewvc?rev=1743648&view=rev
> Log:
> SLING-5726 allow both ValueMapInjector and ResourcePathInjector to act on
> adaptable SlingHttpServletRequest in all circumstances
>
> Modified:
>     sling/site/trunk/content/documentation/bundles/models.mdtext
>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>
> Modified: sling/site/trunk/content/documentation/bundles/models.mdtext
> URL:
> http://svn.apache.org/viewvc/sling/site/trunk/content/documentation/bundles/models.mdtext?rev=1743648&r1=1743647&r2=1743648&view=diff
>
> ==============================================================================
> --- sling/site/trunk/content/documentation/bundles/models.mdtext (original)
> +++ sling/site/trunk/content/documentation/bundles/models.mdtext Fri May
> 13 10:35:03 2016
> @@ -366,7 +366,7 @@ In addition all [injector-specific annot
>  Title              |  Name (for `@Source`)   | Service Ranking     |
> Available Since (Implementation Version) | Description  | Applicable To
> (including using `@Via`) | Accepts Null Name? | Array Support |
> Parameterized Type Support
>  -----------------  | ----------------------- | ------------------- |
> ---------------------------------------- | ------------ |
> -------------------------------------- | ------------------ | -------------
> | --------------------------
>  Script Bindings    | `script-bindings`       | 1000                |
> 1.0.0                                    | Lookup objects in the script
> bindings object by name. | A ServletRequest object which has the `Sling
> Bindings` attribute defined | no | no conversion is done | If a
> parameterized type is passed, the bindings value must be of a compatible
> type of the parameterized type.
> -Value Map          | `valuemap`              | 2000                |
> 1.0.0                                    | Gets a property from a
> `ValueMap` by name. | Any object which is or can be adapted to a
> `ValueMap`| no | Primitive arrays wrapped/unwrapped as necessary. Wrapper
> object arrays are unwrapped/wrapped as necessary. | Parameterized `List`
> and `Collection` injection points are injected by getting an array of the
> component type and creating an unmodifiable `List` from the array.
> +Value Map          | `valuemap`              | 2000                |
> 1.0.0                                    | Gets a property from a
> `ValueMap` by name. | Any object which is or can be adapted to a
> `ValueMap`. Since version [1.2.10](
> https://issues.apache.org/jira/browse/SLING-5726) also
> `SlingHttpServletRequest`. | no | Primitive arrays wrapped/unwrapped as
> necessary. Wrapper object arrays are unwrapped/wrapped as necessary. |
> Parameterized `List` and `Collection` injection points are injected by
> getting an array of the component type and creating an unmodifiable `List`
> from the array.
>  Child Resources    | `child-resources`       | 3000                |
> 1.0.0                                    | Gets a child resource by name. |
> `Resource` objects | no | none | if a parameterized type is passed, a
> `List<Resource>` is returned (the contents of which may be adapted to the
> target type).
>  Request Attributes | `request-attributes`    | 4000                |
> 1.0.0                                    | Get a request attribute by name.
> | `ServletRequest` objects | no | no conversion is done | If a
> parameterized type is passed, the request attribute must be of a compatible
> type of the parameterized type.
>  OSGi Services      | `osgi-services`         | 5000                |
> 1.0.0                                    | Lookup services based on class
> name. Since Sling Models Impl 1.2.8 ([SLING-5664](
> https://issues.apache.org/jira/browse/SLING-5664)) the service with the
> highest service ranking is returned. In case multiple services are
> returned, they are ordered descending by their service ranking (i.e. the
> one with the highest ranking first). | Any object | yes | yes |
> Parameterized `List` and `Collection` injection points are injected by
> getting an array of the services and creating an unmodifiable `List` from
> the array.
>
> Modified:
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> URL:
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>
> ==============================================================================
> ---
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> (original)
> +++
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> Fri May 13 10:35:03 2016
> @@ -44,12 +44,30 @@ abstract class AbstractInjector {
>          return resolver;
>      }
>
> +    /**
> +     * Retrieve the ValueMap from the given adaptable. This succeeds, if
> the adaptable is either
> +     * <ul>
> +     * <li>a {@link ValueMap},</li>
> +     * <li>a {@link SlingHttpServletRequest}, in which case the returned
> {@link ValueMap} is the one derived from the request's resource or</li>
> +     * <li>adaptable to a {@link ValueMap}.</li>
> +     * </ul>
> +     * Otherwise {@code null} is returned.
> +     * @param adaptable
> +     * @return a ValueMap or {@code null}.
> +     */
>      protected ValueMap getValueMap(Object adaptable) {
>          if (adaptable instanceof ValueMap) {
>              return (ValueMap) adaptable;
> +        } else if (adaptable instanceof SlingHttpServletRequest) {
> +            final Resource resource =
> ((SlingHttpServletRequest)adaptable).getResource();
> +            // resource may be null for mocked adaptables, therefore do a
> check here
> +            if (resource != null) {
> +                return resource.adaptTo(ValueMap.class);
> +            } else {
> +                return null;
> +            }
>          } else if (adaptable instanceof Adaptable) {
> -            ValueMap map = ((Adaptable)
> adaptable).adaptTo(ValueMap.class);
> -            return map;
> +            return ((Adaptable) adaptable).adaptTo(ValueMap.class);
>          } else {
>              return null;
>          }
>
> Modified:
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> URL:
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>
> ==============================================================================
> ---
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> (original)
> +++
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> Fri May 13 10:35:03 2016
> @@ -84,6 +84,7 @@ public class ResourcePathInjector extend
>
>          ResourceResolver resolver = getResourceResolver(adaptable);
>          if (resolver == null) {
> +            LOG.debug("Could not get resolver from adaptable {}",
> adaptable);
>              return null;
>          }
>          List<Resource> resources = getResources(resolver, resourcePaths,
> name);
>
> Modified:
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> URL:
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>
> ==============================================================================
> ---
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> (original)
> +++
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> Fri May 13 10:35:03 2016
> @@ -169,12 +169,7 @@ public class ValueMapInjector extends Ab
>              if (StringUtils.isNotBlank(annotation.via())) {
>                  return annotation.via();
>              }
> -            // automatically go via resource, if this is the httprequest
> -            if (adaptable instanceof SlingHttpServletRequest) {
> -                return "resource";
> -            } else {
> -                return null;
> -            }
> +            return null;
>          }
>
>          @Override
>
> Modified:
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> URL:
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>
> ==============================================================================
> ---
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> (original)
> +++
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> Fri May 13 10:35:03 2016
> @@ -57,10 +57,10 @@ public class ResourcePathInjectionTest {
>      private ModelAdapterFactory factory;
>
>      @Mock
> -    private Resource adaptable;
> +    private Resource adaptableResource;
>
>      @Mock
> -    SlingHttpServletRequest nonResourceAdaptable;
> +    SlingHttpServletRequest adaptableRequest;
>
>      @Mock
>      private Resource byPathResource;
> @@ -98,13 +98,16 @@ public class ResourcePathInjectionTest {
>          when(componentCtx.getBundleContext()).thenReturn(bundleContext);
>          when(componentCtx.getProperties()).thenReturn(new
> Hashtable<String, Object>());
>
> -
> when(adaptable.getResourceResolver()).thenReturn(resourceResolver);
> -        when(adaptable.adaptTo(ValueMap.class)).thenReturn(properties);
> +
> when(adaptableResource.getResourceResolver()).thenReturn(resourceResolver);
> +
> when(adaptableResource.adaptTo(ValueMap.class)).thenReturn(properties);
>
>
>  when(resourceResolver.getResource("/some/path")).thenReturn(byPathResource);
>
>  when(resourceResolver.getResource("/some/path2")).thenReturn(byPathResource2);
>
>  when(resourceResolver.getResource("/some/other/path")).thenReturn(byPropertyValueResource);
>
>  when(resourceResolver.getResource("/some/other/path2")).thenReturn(byPropertyValueResource2);
> +
> +        when(adaptableRequest.getResource()).thenReturn(byPathResource);
> +
> when(adaptableRequest.getResourceResolver()).thenReturn(resourceResolver);
>
>          factory = new ModelAdapterFactory();
>          factory.activate(componentCtx);
> @@ -116,8 +119,8 @@ public class ResourcePathInjectionTest {
>      }
>
>      @Test
> -    public void testPathInjection() {
> -        ResourcePathModel model = factory.getAdapter(adaptable,
> ResourcePathModel.class);
> +    public void testPathInjectionFromResource() {
> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
> ResourcePathModel.class);
>          assertNotNull(model);
>          assertEquals(byPathResource, model.getFromPath());
>          assertEquals(byPropertyValueResource, model.getByDerefProperty());
> @@ -126,15 +129,20 @@ public class ResourcePathInjectionTest {
>      }
>
>      @Test
> -    public void testPathInjectionWithNonResourceAdaptable() {
> -        ResourcePathModel model =
> factory.getAdapter(nonResourceAdaptable, ResourcePathModel.class);
> -        // should be null because mandatory fields could not be injected
> -        assertNull(model);
> +    public void testPathInjectionFromRequest() {
> +        // return the same properties through this request's resource, as
> through adaptableResource
> +
> doReturn(adaptableResource.adaptTo(ValueMap.class)).when(byPathResource).adaptTo(ValueMap.class);
> +        ResourcePathModel model = factory.getAdapter(adaptableRequest,
> ResourcePathModel.class);
> +        assertNotNull(model);
> +        assertEquals(byPathResource, model.getFromPath());
> +        assertEquals(byPropertyValueResource, model.getByDerefProperty());
> +        assertEquals(byPathResource2, model.getFromPath2());
> +        assertEquals(byPropertyValueResource2,
> model.getByDerefProperty2());
>      }
>
>      @Test
>      public void testOptionalPathInjectionWithNonResourceAdaptable() {
> -        ResourcePathAllOptionalModel model =
> factory.getAdapter(nonResourceAdaptable,
> ResourcePathAllOptionalModel.class);
> +        ResourcePathAllOptionalModel model =
> factory.getAdapter(adaptableRequest, ResourcePathAllOptionalModel.class);
>          // should not be null because resource paths fields are optional
>          assertNotNull(model);
>          // but the field itself are null
> @@ -146,7 +154,7 @@ public class ResourcePathInjectionTest {
>
>      @Test
>      public void testMultiplePathInjection() {
> -        ResourcePathModel model = factory.getAdapter(adaptable,
> ResourcePathModel.class);
> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
> ResourcePathModel.class);
>          assertNotNull(model);
>          List<Resource> resources=model.getMultipleResources();
>          assertNotNull(resources);
> @@ -172,16 +180,16 @@ public class ResourcePathInjectionTest {
>      public void testPartialInjectionFailure1() {
>
>  when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>
> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
> ResourcePathPartialModel.class);
> +        ResourcePathPartialModel model =
> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
>          assertNull(model);
>      }
>
>      @Test
> -    public void testPartialInjectionFailure2() {
> +    public void testPartialInjectionFailure2() {
>
>  when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>
>  when(resourceResolver.getResource("/some/other/path2")).thenReturn(null);
>
> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
> ResourcePathPartialModel.class);
> +        ResourcePathPartialModel model =
> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
>          assertNull(model);
>      }
>
>
> Modified:
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> URL:
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>
> ==============================================================================
> ---
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> (original)
> +++
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> Fri May 13 10:35:03 2016
> @@ -32,7 +32,7 @@ import org.apache.sling.models.annotatio
>  public class ResourcePathAllOptionalModel {
>
>      @Inject
> -    @Path("/some/path")
> +    @Path("/some/invalidpath")
>      @Optional
>      private Resource fromPath;
>
> @@ -42,11 +42,11 @@ public class ResourcePathAllOptionalMode
>      private Resource derefProperty;
>
>      @Inject
> -    @Path(paths={"/some/path", "/some/path2"})
> +    @Path(paths={"/some/invalidpath", "/some/invalidpath2"})
>      @Optional
>      private Resource manyFromPathNonList;
>
> -    @ResourcePath(path = "/some/path2", optional=true)
> +    @ResourcePath(path = "/some/invalidpath2", optional=true)
>      private Resource fromPath2;
>
>      @ResourcePath(name = "anotherPropertyContainingAPath", optional=true)
>
>
>

Re: svn commit: r1743648 - in /sling: site/trunk/content/documentation/bundles/ trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ trun...

Posted by Justin Edelson <ju...@justinedelson.com>.

Hi Georg,
I'm entirely supportive of the notion of using models in multiple contexts.
My concern here is one of separation of concerns. AbstractInjector is *not*
API, it is an internal helper. Putting additional expectations on Injectors
just seems wrong to me.

What we could do as an alternative would be to extend the @Via annotation
to express some kind of condition, e.g.

@Inject @Via(value = "resource", forAdaptable =
SlingHttpServletRequest.class)
private String description;

Which says "for this injection point, use the actual adaptable via
getResource() when the adaptable is of type SlingHttpServletRequest"

That would work consistently across all Injectors.

WDYT?

I'll put this comment in JIRA so we can move this conversation there.

Justin

On Sat, May 14, 2016 at 9:19 AM Georg Henzler <sl...@ghenzler.de> wrote:

> It wasn't my intention to trigger this already very polarized discussion
> with SLING-5721, but now that it has started my 50cents:
>
> For me the most important is that I can create models, that behave well
> in request context (for rendering pages) and in async context (jobs,
> workflows etc.), see [1] for an example. Thanks to the now correct order
> of adaptions for request/resource in Sightly (SLING-5031) and the fact
> that @ValueMapValue automatically assumes @Via("resource"), I can
> currently achieve it as long as I don't use @ResourcePath. I believe
> either a) all injectors that somehow operate on a resource should
> automatically assume @Via("resource") if adapted from request
> (@ResourcePath is *not* doing this ATM) or alternatively (and more
> reasonable I believe) AbstractInjector.getValueMap() just does that for
> all injectors (including future ones) automatically.
>
> Using @Via would force me to write two models for [1]. So I would
> suggest reapplying SLING-5726 (and get rid of assuming @Via("resource")
> for @ValueMapValue that is then not needed anymore).
>
> Best Regards
> Georg
>
> [1]
> @Model(adaptables = { SlingHttpServletRequest.class, Resource.class },
> defaultInjectionStrategy = DefaultInjectionStrategy.OPTIONAL)
> public class ProductModel {
>
>      @ValueMapValue
>      @Named(JcrConstants.JCR_TITLE)
>      private String productTitle;
>
>      @ValueMapValue
>      @Named(JcrConstants.JCR_DESCRIPTION)
>      private String productDescription;
>
>      @RequestAttribute
>      private boolean isSale;
>
>      public void setIsSale(boolean isSale) {
>          this.isSale = isSale;
>      }
>
>      // ... code that somehow behaves differently depending on isSale
>
>      // For Sightly isSale may be set using Sightly parameters, for async
> usages the setter setIsSale() can be used
>      // ... but generally the class works fine for both
>      //          request.adaptTo(ProductModel.class) and
> resource.adaptTo(ProductModel.class)
>
> }
>
>
> On 2016-05-13 13:39, Konrad Windszus wrote:
> > I reverted the commit now and reopened
> > https://issues.apache.org/jira/browse/SLING-5726.
> >
> > To clarify my point of view:
> > I think each injector can decide on its own which adaptable to support
> > natively. Whether or not a specific injector also supports accessing
> > the value map from the underlying request's resource or not is IMHO a
> > thing which
> > a) needs to be documented
> > b) is mostly driven by the fact how often consumer will try to use the
> > injector with which adaptable
> >
> > Regarding the point of consistency I think it would be even better if
> > all OOTB injectors support SlingHttpRequestServlet! Some injectors may
> > in addition support other adaptables. That way using those would be
> > much easier.
> > I think not every developer remembers the details of each injector, so
> > it might not be obvious which adaptable is supported. Instead of each
> > time looking at the documentation, just supporting the minimum (i.e.
> > SlingHttpServletRequest) seems to be the better choice here. Some
> > developers do not even care which injector is injecting the value and
> > where it comes from, but in case something does not work, very often
> > the problem is, that just the wrong adaptable has been used (which
> > fails silently and can only be traced down by increasing the log
> > level, or just by knowing what is wrong).
> >
> > Having via in addition does not do any harm IMHO, but to be honest I
> > have not seen the usage of "via" very often in the past and to use
> > "via" properly you have to know some details on the injector that I
> > don't want every developer to be aware of.
> > That was also the reason, why for the annotation "ValueMapValue" the
> > via element was automatically set to "resource" in case you were
> > dealing with a SlingHttpServletRequest
> > (
> https://github.com/apache/sling/blob/c431b4a0504f79a463593b7509cefb5ba15cda2c/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java#L174
> ).
> >
> > I hope my point is more clear now.
> > Konrad
> >
> >
> >
> >> On 13 May 2016, at 13:11, Justin Edelson <ju...@justinedelson.com>
> >> wrote:
> >>
> >> Hi Konrad,
> >> I think you are missing the point. The current recommended practice
> >> (to use
> >> @Via) is consistent across injectors. Once you start adding this
> >> functionality to individual injectors, this level of consistency is
> >> lost.
> >> If @Via isn't well understood, let's fix that problem. But making it
> >> unnecessary in some (but not all) cases just adds confusion.
> >>
> >> Please go ahead and revert while you solicit other feedback. I'm
> >> doubtful
> >> that other feedback will cause me to revoke my veto, but I'm open to
> >> the
> >> idea.
> >>
> >> Regards,
> >> Justin
> >> On Fri, May 13, 2016 at 11:57 AM Konrad Windszus <ko...@gmx.de>
> >> wrote:
> >>
> >>> I know that you can use Via for that purpose, but it is very often
> >>> forgotten. And due to the way how injectors are asked this oversight
> >>> is
> >>> very hard to debug and trace down.
> >>>
> >>> Just allowing to directly adapt from a request for both injectors,
> >>> makes
> >>> using those injectors a lot more comfortable.
> >>>
> >>> The additional complexity I don't really see here, the only real
> >>> change is
> >>> in
> >>>
> https://github.com/apache/sling/commit/c17cccfa17921f3a46003ce3840b1f3d3d59a490#diff-edbcc30dda0513662c6a3227c530ea38L47
> ,
> >>> method getValueMap().
> >>>
> >>> In addition it was never documented in
> >>>
> http://sling.apache.org/documentation/bundles/models.html#available-injectors
> >>> that resource-path injection only worked with paths retrieved from a
> >>> resource property, if the adaptable Resource was used.
> >>> It just states
> >>> Applicable to: Resource or SlingHttpServletRequest objects
> >>>
> >>> So IMHO this commit fixes a bug in ResourcePathInjector and makes
> >>> using
> >>> ValueMapInjector just more comfortable.
> >>> I would be interested to hear other opinions on that change.
> >>> Thanks,
> >>> Konrad
> >>>
> >>>
> >>>> On 13 May 2016, at 12:45, Justin Edelson <ju...@justinedelson.com>
> >>> wrote:
> >>>>
> >>>> -1 to this change (would comment in JIRA but I can't for some reason
> >>> right
> >>>> now)
> >>>>
> >>>> This adds unnecessary complexity and is poor seperation of concerns.
> >>>> The
> >>>> @Via annotation can be used for this use case (and is documented as
> >>> such).
> >>>> On Fri, May 13, 2016 at 11:35 AM <kw...@apache.org> wrote:
> >>>>
> >>>>> Author: kwin
> >>>>> Date: Fri May 13 10:35:03 2016
> >>>>> New Revision: 1743648
> >>>>>
> >>>>> URL: http://svn.apache.org/viewvc?rev=1743648&view=rev
> >>>>> Log:
> >>>>> SLING-5726 allow both ValueMapInjector and ResourcePathInjector to
> >>>>> act
> >>> on
> >>>>> adaptable SlingHttpServletRequest in all circumstances
> >>>>>
> >>>>> Modified:
> >>>>>   sling/site/trunk/content/documentation/bundles/models.mdtext
> >>>>>
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >>>>>
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >>>>>
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >>>>>
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >>>>>
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >>>>>
> >>>>> Modified:
> >>>>> sling/site/trunk/content/documentation/bundles/models.mdtext
> >>>>> URL:
> >>>>>
> >>>
> http://svn.apache.org/viewvc/sling/site/trunk/content/documentation/bundles/models.mdtext?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>>>>
> >>>>>
> >>>
> ==============================================================================
> >>>>> --- sling/site/trunk/content/documentation/bundles/models.mdtext
> >>> (original)
> >>>>> +++ sling/site/trunk/content/documentation/bundles/models.mdtext
> >>>>> Fri May
> >>>>> 13 10:35:03 2016
> >>>>> @@ -366,7 +366,7 @@ In addition all [injector-specific annot
> >>>>> Title              |  Name (for `@Source`)   | Service Ranking
> >>>>> |
> >>>>> Available Since (Implementation Version) | Description  |
> >>>>> Applicable To
> >>>>> (including using `@Via`) | Accepts Null Name? | Array Support |
> >>>>> Parameterized Type Support
> >>>>> -----------------  | ----------------------- | -------------------
> >>>>> |
> >>>>> ---------------------------------------- | ------------ |
> >>>>> -------------------------------------- | ------------------ |
> >>> -------------
> >>>>> | --------------------------
> >>>>> Script Bindings    | `script-bindings`       | 1000
> >>>>> |
> >>>>> 1.0.0                                    | Lookup objects in the
> >>>>> script
> >>>>> bindings object by name. | A ServletRequest object which has the
> >>>>> `Sling
> >>>>> Bindings` attribute defined | no | no conversion is done | If a
> >>>>> parameterized type is passed, the bindings value must be of a
> >>>>> compatible
> >>>>> type of the parameterized type.
> >>>>> -Value Map          | `valuemap`              | 2000
> >>>>> |
> >>>>> 1.0.0                                    | Gets a property from a
> >>>>> `ValueMap` by name. | Any object which is or can be adapted to a
> >>>>> `ValueMap`| no | Primitive arrays wrapped/unwrapped as necessary.
> >>> Wrapper
> >>>>> object arrays are unwrapped/wrapped as necessary. | Parameterized
> >>>>> `List`
> >>>>> and `Collection` injection points are injected by getting an array
> >>>>> of
> >>> the
> >>>>> component type and creating an unmodifiable `List` from the array.
> >>>>> +Value Map          | `valuemap`              | 2000
> >>>>> |
> >>>>> 1.0.0                                    | Gets a property from a
> >>>>> `ValueMap` by name. | Any object which is or can be adapted to a
> >>>>> `ValueMap`. Since version [1.2.10](
> >>>>> https://issues.apache.org/jira/browse/SLING-5726) also
> >>>>> `SlingHttpServletRequest`. | no | Primitive arrays
> >>>>> wrapped/unwrapped as
> >>>>> necessary. Wrapper object arrays are unwrapped/wrapped as
> >>>>> necessary. |
> >>>>> Parameterized `List` and `Collection` injection points are injected
> >>>>> by
> >>>>> getting an array of the component type and creating an unmodifiable
> >>> `List`
> >>>>> from the array.
> >>>>> Child Resources    | `child-resources`       | 3000
> >>>>> |
> >>>>> 1.0.0                                    | Gets a child resource by
> >>> name. |
> >>>>> `Resource` objects | no | none | if a parameterized type is passed,
> >>>>> a
> >>>>> `List<Resource>` is returned (the contents of which may be adapted
> >>>>> to
> >>> the
> >>>>> target type).
> >>>>> Request Attributes | `request-attributes`    | 4000
> >>>>> |
> >>>>> 1.0.0                                    | Get a request attribute
> >>>>> by
> >>> name.
> >>>>> | `ServletRequest` objects | no | no conversion is done | If a
> >>>>> parameterized type is passed, the request attribute must be of a
> >>> compatible
> >>>>> type of the parameterized type.
> >>>>> OSGi Services      | `osgi-services`         | 5000
> >>>>> |
> >>>>> 1.0.0                                    | Lookup services based on
> >>> class
> >>>>> name. Since Sling Models Impl 1.2.8 ([SLING-5664](
> >>>>> https://issues.apache.org/jira/browse/SLING-5664)) the service with
> >>>>> the
> >>>>> highest service ranking is returned. In case multiple services are
> >>>>> returned, they are ordered descending by their service ranking
> >>>>> (i.e. the
> >>>>> one with the highest ranking first). | Any object | yes | yes |
> >>>>> Parameterized `List` and `Collection` injection points are injected
> >>>>> by
> >>>>> getting an array of the services and creating an unmodifiable
> >>>>> `List`
> >>> from
> >>>>> the array.
> >>>>>
> >>>>> Modified:
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >>>>> URL:
> >>>>>
> >>>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>>>>
> >>>>>
> >>>
> ==============================================================================
> >>>>> ---
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >>>>> (original)
> >>>>> +++
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >>>>> Fri May 13 10:35:03 2016
> >>>>> @@ -44,12 +44,30 @@ abstract class AbstractInjector {
> >>>>>        return resolver;
> >>>>>    }
> >>>>>
> >>>>> +    /**
> >>>>> +     * Retrieve the ValueMap from the given adaptable. This
> >>>>> succeeds,
> >>> if
> >>>>> the adaptable is either
> >>>>> +     * <ul>
> >>>>> +     * <li>a {@link ValueMap},</li>
> >>>>> +     * <li>a {@link SlingHttpServletRequest}, in which case the
> >>> returned
> >>>>> {@link ValueMap} is the one derived from the request's resource
> >>>>> or</li>
> >>>>> +     * <li>adaptable to a {@link ValueMap}.</li>
> >>>>> +     * </ul>
> >>>>> +     * Otherwise {@code null} is returned.
> >>>>> +     * @param adaptable
> >>>>> +     * @return a ValueMap or {@code null}.
> >>>>> +     */
> >>>>>    protected ValueMap getValueMap(Object adaptable) {
> >>>>>        if (adaptable instanceof ValueMap) {
> >>>>>            return (ValueMap) adaptable;
> >>>>> +        } else if (adaptable instanceof SlingHttpServletRequest) {
> >>>>> +            final Resource resource =
> >>>>> ((SlingHttpServletRequest)adaptable).getResource();
> >>>>> +            // resource may be null for mocked adaptables,
> >>>>> therefore
> >>> do a
> >>>>> check here
> >>>>> +            if (resource != null) {
> >>>>> +                return resource.adaptTo(ValueMap.class);
> >>>>> +            } else {
> >>>>> +                return null;
> >>>>> +            }
> >>>>>        } else if (adaptable instanceof Adaptable) {
> >>>>> -            ValueMap map = ((Adaptable)
> >>>>> adaptable).adaptTo(ValueMap.class);
> >>>>> -            return map;
> >>>>> +            return ((Adaptable)
> >>>>> adaptable).adaptTo(ValueMap.class);
> >>>>>        } else {
> >>>>>            return null;
> >>>>>        }
> >>>>>
> >>>>> Modified:
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >>>>> URL:
> >>>>>
> >>>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>>>>
> >>>>>
> >>>
> ==============================================================================
> >>>>> ---
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >>>>> (original)
> >>>>> +++
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >>>>> Fri May 13 10:35:03 2016
> >>>>> @@ -84,6 +84,7 @@ public class ResourcePathInjector extend
> >>>>>
> >>>>>        ResourceResolver resolver = getResourceResolver(adaptable);
> >>>>>        if (resolver == null) {
> >>>>> +            LOG.debug("Could not get resolver from adaptable {}",
> >>>>> adaptable);
> >>>>>            return null;
> >>>>>        }
> >>>>>        List<Resource> resources = getResources(resolver,
> >>>>> resourcePaths,
> >>>>> name);
> >>>>>
> >>>>> Modified:
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >>>>> URL:
> >>>>>
> >>>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>>>>
> >>>>>
> >>>
> ==============================================================================
> >>>>> ---
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >>>>> (original)
> >>>>> +++
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >>>>> Fri May 13 10:35:03 2016
> >>>>> @@ -169,12 +169,7 @@ public class ValueMapInjector extends Ab
> >>>>>            if (StringUtils.isNotBlank(annotation.via())) {
> >>>>>                return annotation.via();
> >>>>>            }
> >>>>> -            // automatically go via resource, if this is the
> >>> httprequest
> >>>>> -            if (adaptable instanceof SlingHttpServletRequest) {
> >>>>> -                return "resource";
> >>>>> -            } else {
> >>>>> -                return null;
> >>>>> -            }
> >>>>> +            return null;
> >>>>>        }
> >>>>>
> >>>>>        @Override
> >>>>>
> >>>>> Modified:
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >>>>> URL:
> >>>>>
> >>>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>>>>
> >>>>>
> >>>
> ==============================================================================
> >>>>> ---
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >>>>> (original)
> >>>>> +++
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >>>>> Fri May 13 10:35:03 2016
> >>>>> @@ -57,10 +57,10 @@ public class ResourcePathInjectionTest {
> >>>>>    private ModelAdapterFactory factory;
> >>>>>
> >>>>>    @Mock
> >>>>> -    private Resource adaptable;
> >>>>> +    private Resource adaptableResource;
> >>>>>
> >>>>>    @Mock
> >>>>> -    SlingHttpServletRequest nonResourceAdaptable;
> >>>>> +    SlingHttpServletRequest adaptableRequest;
> >>>>>
> >>>>>    @Mock
> >>>>>    private Resource byPathResource;
> >>>>> @@ -98,13 +98,16 @@ public class ResourcePathInjectionTest {
> >>>>>
> >>>>> when(componentCtx.getBundleContext()).thenReturn(bundleContext);
> >>>>>        when(componentCtx.getProperties()).thenReturn(new
> >>>>> Hashtable<String, Object>());
> >>>>>
> >>>>> -
> >>>>> when(adaptable.getResourceResolver()).thenReturn(resourceResolver);
> >>>>> -
> >>>>> when(adaptable.adaptTo(ValueMap.class)).thenReturn(properties);
> >>>>> +
> >>>>>
> >>>
> when(adaptableResource.getResourceResolver()).thenReturn(resourceResolver);
> >>>>> +
> >>>>>
> when(adaptableResource.adaptTo(ValueMap.class)).thenReturn(properties);
> >>>>>
> >>>>>
> >>>>>
> >>>
> when(resourceResolver.getResource("/some/path")).thenReturn(byPathResource);
> >>>>>
> >>>>>
> >>>
> when(resourceResolver.getResource("/some/path2")).thenReturn(byPathResource2);
> >>>>>
> >>>>>
> >>>
> when(resourceResolver.getResource("/some/other/path")).thenReturn(byPropertyValueResource);
> >>>>>
> >>>>>
> >>>
> when(resourceResolver.getResource("/some/other/path2")).thenReturn(byPropertyValueResource2);
> >>>>> +
> >>>>> +
> >>> when(adaptableRequest.getResource()).thenReturn(byPathResource);
> >>>>> +
> >>>>>
> >>>
> when(adaptableRequest.getResourceResolver()).thenReturn(resourceResolver);
> >>>>>
> >>>>>        factory = new ModelAdapterFactory();
> >>>>>        factory.activate(componentCtx);
> >>>>> @@ -116,8 +119,8 @@ public class ResourcePathInjectionTest {
> >>>>>    }
> >>>>>
> >>>>>    @Test
> >>>>> -    public void testPathInjection() {
> >>>>> -        ResourcePathModel model = factory.getAdapter(adaptable,
> >>>>> ResourcePathModel.class);
> >>>>> +    public void testPathInjectionFromResource() {
> >>>>> +        ResourcePathModel model =
> >>>>> factory.getAdapter(adaptableResource,
> >>>>> ResourcePathModel.class);
> >>>>>        assertNotNull(model);
> >>>>>        assertEquals(byPathResource, model.getFromPath());
> >>>>>        assertEquals(byPropertyValueResource,
> >>> model.getByDerefProperty());
> >>>>> @@ -126,15 +129,20 @@ public class ResourcePathInjectionTest {
> >>>>>    }
> >>>>>
> >>>>>    @Test
> >>>>> -    public void testPathInjectionWithNonResourceAdaptable() {
> >>>>> -        ResourcePathModel model =
> >>>>> factory.getAdapter(nonResourceAdaptable, ResourcePathModel.class);
> >>>>> -        // should be null because mandatory fields could not be
> >>> injected
> >>>>> -        assertNull(model);
> >>>>> +    public void testPathInjectionFromRequest() {
> >>>>> +        // return the same properties through this request's
> >>>>> resource,
> >>> as
> >>>>> through adaptableResource
> >>>>> +
> >>>>>
> >>>
> doReturn(adaptableResource.adaptTo(ValueMap.class)).when(byPathResource).adaptTo(ValueMap.class);
> >>>>> +        ResourcePathModel model =
> >>>>> factory.getAdapter(adaptableRequest,
> >>>>> ResourcePathModel.class);
> >>>>> +        assertNotNull(model);
> >>>>> +        assertEquals(byPathResource, model.getFromPath());
> >>>>> +        assertEquals(byPropertyValueResource,
> >>> model.getByDerefProperty());
> >>>>> +        assertEquals(byPathResource2, model.getFromPath2());
> >>>>> +        assertEquals(byPropertyValueResource2,
> >>>>> model.getByDerefProperty2());
> >>>>>    }
> >>>>>
> >>>>>    @Test
> >>>>>    public void testOptionalPathInjectionWithNonResourceAdaptable()
> >>>>> {
> >>>>> -        ResourcePathAllOptionalModel model =
> >>>>> factory.getAdapter(nonResourceAdaptable,
> >>>>> ResourcePathAllOptionalModel.class);
> >>>>> +        ResourcePathAllOptionalModel model =
> >>>>> factory.getAdapter(adaptableRequest,
> >>> ResourcePathAllOptionalModel.class);
> >>>>>        // should not be null because resource paths fields are
> >>>>> optional
> >>>>>        assertNotNull(model);
> >>>>>        // but the field itself are null
> >>>>> @@ -146,7 +154,7 @@ public class ResourcePathInjectionTest {
> >>>>>
> >>>>>    @Test
> >>>>>    public void testMultiplePathInjection() {
> >>>>> -        ResourcePathModel model = factory.getAdapter(adaptable,
> >>>>> ResourcePathModel.class);
> >>>>> +        ResourcePathModel model =
> >>>>> factory.getAdapter(adaptableResource,
> >>>>> ResourcePathModel.class);
> >>>>>        assertNotNull(model);
> >>>>>        List<Resource> resources=model.getMultipleResources();
> >>>>>        assertNotNull(resources);
> >>>>> @@ -172,16 +180,16 @@ public class ResourcePathInjectionTest {
> >>>>>    public void testPartialInjectionFailure1() {
> >>>>>
> >>>>>
> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
> >>>>>
> >>>>> -        ResourcePathPartialModel model =
> >>>>> factory.getAdapter(adaptable,
> >>>>> ResourcePathPartialModel.class);
> >>>>> +        ResourcePathPartialModel model =
> >>>>> factory.getAdapter(adaptableResource,
> >>>>> ResourcePathPartialModel.class);
> >>>>>        assertNull(model);
> >>>>>    }
> >>>>>
> >>>>>    @Test
> >>>>> -    public void testPartialInjectionFailure2() {
> >>>>> +    public void testPartialInjectionFailure2() {
> >>>>>
> >>>>>
> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
> >>>>>
> >>>>>
> >>>
> when(resourceResolver.getResource("/some/other/path2")).thenReturn(null);
> >>>>>
> >>>>> -        ResourcePathPartialModel model =
> >>>>> factory.getAdapter(adaptable,
> >>>>> ResourcePathPartialModel.class);
> >>>>> +        ResourcePathPartialModel model =
> >>>>> factory.getAdapter(adaptableResource,
> >>>>> ResourcePathPartialModel.class);
> >>>>>        assertNull(model);
> >>>>>    }
> >>>>>
> >>>>>
> >>>>> Modified:
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >>>>> URL:
> >>>>>
> >>>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>>>>
> >>>>>
> >>>
> ==============================================================================
> >>>>> ---
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >>>>> (original)
> >>>>> +++
> >>>>>
> >>>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >>>>> Fri May 13 10:35:03 2016
> >>>>> @@ -32,7 +32,7 @@ import org.apache.sling.models.annotatio
> >>>>> public class ResourcePathAllOptionalModel {
> >>>>>
> >>>>>    @Inject
> >>>>> -    @Path("/some/path")
> >>>>> +    @Path("/some/invalidpath")
> >>>>>    @Optional
> >>>>>    private Resource fromPath;
> >>>>>
> >>>>> @@ -42,11 +42,11 @@ public class ResourcePathAllOptionalMode
> >>>>>    private Resource derefProperty;
> >>>>>
> >>>>>    @Inject
> >>>>> -    @Path(paths={"/some/path", "/some/path2"})
> >>>>> +    @Path(paths={"/some/invalidpath", "/some/invalidpath2"})
> >>>>>    @Optional
> >>>>>    private Resource manyFromPathNonList;
> >>>>>
> >>>>> -    @ResourcePath(path = "/some/path2", optional=true)
> >>>>> +    @ResourcePath(path = "/some/invalidpath2", optional=true)
> >>>>>    private Resource fromPath2;
> >>>>>
> >>>>>    @ResourcePath(name = "anotherPropertyContainingAPath",
> >>> optional=true)
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
>
>

Re: svn commit: r1743648 - in /sling: site/trunk/content/documentation/bundles/ trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ trun...

Posted by Georg Henzler <sl...@ghenzler.de>.

It wasn't my intention to trigger this already very polarized discussion 
with SLING-5721, but now that it has started my 50cents:

For me the most important is that I can create models, that behave well 
in request context (for rendering pages) and in async context (jobs, 
workflows etc.), see [1] for an example. Thanks to the now correct order 
of adaptions for request/resource in Sightly (SLING-5031) and the fact 
that @ValueMapValue automatically assumes @Via("resource"), I can 
currently achieve it as long as I don't use @ResourcePath. I believe 
either a) all injectors that somehow operate on a resource should 
automatically assume @Via("resource") if adapted from request 
(@ResourcePath is *not* doing this ATM) or alternatively (and more 
reasonable I believe) AbstractInjector.getValueMap() just does that for 
all injectors (including future ones) automatically.

Using @Via would force me to write two models for [1]. So I would 
suggest reapplying SLING-5726 (and get rid of assuming @Via("resource") 
for @ValueMapValue that is then not needed anymore).

Best Regards
Georg

[1]
@Model(adaptables = { SlingHttpServletRequest.class, Resource.class }, 
defaultInjectionStrategy = DefaultInjectionStrategy.OPTIONAL)
public class ProductModel {

     @ValueMapValue
     @Named(JcrConstants.JCR_TITLE)
     private String productTitle;

     @ValueMapValue
     @Named(JcrConstants.JCR_DESCRIPTION)
     private String productDescription;

     @RequestAttribute
     private boolean isSale;

     public void setIsSale(boolean isSale) {
         this.isSale = isSale;
     }

     // ... code that somehow behaves differently depending on isSale

     // For Sightly isSale may be set using Sightly parameters, for async 
usages the setter setIsSale() can be used
     // ... but generally the class works fine for both
     //          request.adaptTo(ProductModel.class) and 
resource.adaptTo(ProductModel.class)

}


On 2016-05-13 13:39, Konrad Windszus wrote:
> I reverted the commit now and reopened
> https://issues.apache.org/jira/browse/SLING-5726.
> 
> To clarify my point of view:
> I think each injector can decide on its own which adaptable to support
> natively. Whether or not a specific injector also supports accessing
> the value map from the underlying request's resource or not is IMHO a
> thing which
> a) needs to be documented
> b) is mostly driven by the fact how often consumer will try to use the
> injector with which adaptable
> 
> Regarding the point of consistency I think it would be even better if
> all OOTB injectors support SlingHttpRequestServlet! Some injectors may
> in addition support other adaptables. That way using those would be
> much easier.
> I think not every developer remembers the details of each injector, so
> it might not be obvious which adaptable is supported. Instead of each
> time looking at the documentation, just supporting the minimum (i.e.
> SlingHttpServletRequest) seems to be the better choice here. Some
> developers do not even care which injector is injecting the value and
> where it comes from, but in case something does not work, very often
> the problem is, that just the wrong adaptable has been used (which
> fails silently and can only be traced down by increasing the log
> level, or just by knowing what is wrong).
> 
> Having via in addition does not do any harm IMHO, but to be honest I
> have not seen the usage of "via" very often in the past and to use
> "via" properly you have to know some details on the injector that I
> don't want every developer to be aware of.
> That was also the reason, why for the annotation "ValueMapValue" the
> via element was automatically set to "resource" in case you were
> dealing with a SlingHttpServletRequest
> (https://github.com/apache/sling/blob/c431b4a0504f79a463593b7509cefb5ba15cda2c/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java#L174).
> 
> I hope my point is more clear now.
> Konrad
> 
> 
> 
>> On 13 May 2016, at 13:11, Justin Edelson <ju...@justinedelson.com> 
>> wrote:
>> 
>> Hi Konrad,
>> I think you are missing the point. The current recommended practice 
>> (to use
>> @Via) is consistent across injectors. Once you start adding this
>> functionality to individual injectors, this level of consistency is 
>> lost.
>> If @Via isn't well understood, let's fix that problem. But making it
>> unnecessary in some (but not all) cases just adds confusion.
>> 
>> Please go ahead and revert while you solicit other feedback. I'm 
>> doubtful
>> that other feedback will cause me to revoke my veto, but I'm open to 
>> the
>> idea.
>> 
>> Regards,
>> Justin
>> On Fri, May 13, 2016 at 11:57 AM Konrad Windszus <ko...@gmx.de> 
>> wrote:
>> 
>>> I know that you can use Via for that purpose, but it is very often
>>> forgotten. And due to the way how injectors are asked this oversight 
>>> is
>>> very hard to debug and trace down.
>>> 
>>> Just allowing to directly adapt from a request for both injectors, 
>>> makes
>>> using those injectors a lot more comfortable.
>>> 
>>> The additional complexity I don't really see here, the only real 
>>> change is
>>> in
>>> https://github.com/apache/sling/commit/c17cccfa17921f3a46003ce3840b1f3d3d59a490#diff-edbcc30dda0513662c6a3227c530ea38L47,
>>> method getValueMap().
>>> 
>>> In addition it was never documented in
>>> http://sling.apache.org/documentation/bundles/models.html#available-injectors
>>> that resource-path injection only worked with paths retrieved from a
>>> resource property, if the adaptable Resource was used.
>>> It just states
>>> Applicable to: Resource or SlingHttpServletRequest objects
>>> 
>>> So IMHO this commit fixes a bug in ResourcePathInjector and makes 
>>> using
>>> ValueMapInjector just more comfortable.
>>> I would be interested to hear other opinions on that change.
>>> Thanks,
>>> Konrad
>>> 
>>> 
>>>> On 13 May 2016, at 12:45, Justin Edelson <ju...@justinedelson.com>
>>> wrote:
>>>> 
>>>> -1 to this change (would comment in JIRA but I can't for some reason
>>> right
>>>> now)
>>>> 
>>>> This adds unnecessary complexity and is poor seperation of concerns. 
>>>> The
>>>> @Via annotation can be used for this use case (and is documented as
>>> such).
>>>> On Fri, May 13, 2016 at 11:35 AM <kw...@apache.org> wrote:
>>>> 
>>>>> Author: kwin
>>>>> Date: Fri May 13 10:35:03 2016
>>>>> New Revision: 1743648
>>>>> 
>>>>> URL: http://svn.apache.org/viewvc?rev=1743648&view=rev
>>>>> Log:
>>>>> SLING-5726 allow both ValueMapInjector and ResourcePathInjector to 
>>>>> act
>>> on
>>>>> adaptable SlingHttpServletRequest in all circumstances
>>>>> 
>>>>> Modified:
>>>>>   sling/site/trunk/content/documentation/bundles/models.mdtext
>>>>> 
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>>> 
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>>> 
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>>> 
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>>> 
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>>> 
>>>>> Modified: 
>>>>> sling/site/trunk/content/documentation/bundles/models.mdtext
>>>>> URL:
>>>>> 
>>> http://svn.apache.org/viewvc/sling/site/trunk/content/documentation/bundles/models.mdtext?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>>> 
>>>>> 
>>> ==============================================================================
>>>>> --- sling/site/trunk/content/documentation/bundles/models.mdtext
>>> (original)
>>>>> +++ sling/site/trunk/content/documentation/bundles/models.mdtext 
>>>>> Fri May
>>>>> 13 10:35:03 2016
>>>>> @@ -366,7 +366,7 @@ In addition all [injector-specific annot
>>>>> Title              |  Name (for `@Source`)   | Service Ranking     
>>>>> |
>>>>> Available Since (Implementation Version) | Description  | 
>>>>> Applicable To
>>>>> (including using `@Via`) | Accepts Null Name? | Array Support |
>>>>> Parameterized Type Support
>>>>> -----------------  | ----------------------- | ------------------- 
>>>>> |
>>>>> ---------------------------------------- | ------------ |
>>>>> -------------------------------------- | ------------------ |
>>> -------------
>>>>> | --------------------------
>>>>> Script Bindings    | `script-bindings`       | 1000                
>>>>> |
>>>>> 1.0.0                                    | Lookup objects in the 
>>>>> script
>>>>> bindings object by name. | A ServletRequest object which has the 
>>>>> `Sling
>>>>> Bindings` attribute defined | no | no conversion is done | If a
>>>>> parameterized type is passed, the bindings value must be of a 
>>>>> compatible
>>>>> type of the parameterized type.
>>>>> -Value Map          | `valuemap`              | 2000                
>>>>> |
>>>>> 1.0.0                                    | Gets a property from a
>>>>> `ValueMap` by name. | Any object which is or can be adapted to a
>>>>> `ValueMap`| no | Primitive arrays wrapped/unwrapped as necessary.
>>> Wrapper
>>>>> object arrays are unwrapped/wrapped as necessary. | Parameterized 
>>>>> `List`
>>>>> and `Collection` injection points are injected by getting an array 
>>>>> of
>>> the
>>>>> component type and creating an unmodifiable `List` from the array.
>>>>> +Value Map          | `valuemap`              | 2000                
>>>>> |
>>>>> 1.0.0                                    | Gets a property from a
>>>>> `ValueMap` by name. | Any object which is or can be adapted to a
>>>>> `ValueMap`. Since version [1.2.10](
>>>>> https://issues.apache.org/jira/browse/SLING-5726) also
>>>>> `SlingHttpServletRequest`. | no | Primitive arrays 
>>>>> wrapped/unwrapped as
>>>>> necessary. Wrapper object arrays are unwrapped/wrapped as 
>>>>> necessary. |
>>>>> Parameterized `List` and `Collection` injection points are injected 
>>>>> by
>>>>> getting an array of the component type and creating an unmodifiable
>>> `List`
>>>>> from the array.
>>>>> Child Resources    | `child-resources`       | 3000                
>>>>> |
>>>>> 1.0.0                                    | Gets a child resource by
>>> name. |
>>>>> `Resource` objects | no | none | if a parameterized type is passed, 
>>>>> a
>>>>> `List<Resource>` is returned (the contents of which may be adapted 
>>>>> to
>>> the
>>>>> target type).
>>>>> Request Attributes | `request-attributes`    | 4000                
>>>>> |
>>>>> 1.0.0                                    | Get a request attribute 
>>>>> by
>>> name.
>>>>> | `ServletRequest` objects | no | no conversion is done | If a
>>>>> parameterized type is passed, the request attribute must be of a
>>> compatible
>>>>> type of the parameterized type.
>>>>> OSGi Services      | `osgi-services`         | 5000                
>>>>> |
>>>>> 1.0.0                                    | Lookup services based on
>>> class
>>>>> name. Since Sling Models Impl 1.2.8 ([SLING-5664](
>>>>> https://issues.apache.org/jira/browse/SLING-5664)) the service with 
>>>>> the
>>>>> highest service ranking is returned. In case multiple services are
>>>>> returned, they are ordered descending by their service ranking 
>>>>> (i.e. the
>>>>> one with the highest ranking first). | Any object | yes | yes |
>>>>> Parameterized `List` and `Collection` injection points are injected 
>>>>> by
>>>>> getting an array of the services and creating an unmodifiable 
>>>>> `List`
>>> from
>>>>> the array.
>>>>> 
>>>>> Modified:
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>>> URL:
>>>>> 
>>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>>> 
>>>>> 
>>> ==============================================================================
>>>>> ---
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>>> (original)
>>>>> +++
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>>> Fri May 13 10:35:03 2016
>>>>> @@ -44,12 +44,30 @@ abstract class AbstractInjector {
>>>>>        return resolver;
>>>>>    }
>>>>> 
>>>>> +    /**
>>>>> +     * Retrieve the ValueMap from the given adaptable. This 
>>>>> succeeds,
>>> if
>>>>> the adaptable is either
>>>>> +     * <ul>
>>>>> +     * <li>a {@link ValueMap},</li>
>>>>> +     * <li>a {@link SlingHttpServletRequest}, in which case the
>>> returned
>>>>> {@link ValueMap} is the one derived from the request's resource 
>>>>> or</li>
>>>>> +     * <li>adaptable to a {@link ValueMap}.</li>
>>>>> +     * </ul>
>>>>> +     * Otherwise {@code null} is returned.
>>>>> +     * @param adaptable
>>>>> +     * @return a ValueMap or {@code null}.
>>>>> +     */
>>>>>    protected ValueMap getValueMap(Object adaptable) {
>>>>>        if (adaptable instanceof ValueMap) {
>>>>>            return (ValueMap) adaptable;
>>>>> +        } else if (adaptable instanceof SlingHttpServletRequest) {
>>>>> +            final Resource resource =
>>>>> ((SlingHttpServletRequest)adaptable).getResource();
>>>>> +            // resource may be null for mocked adaptables, 
>>>>> therefore
>>> do a
>>>>> check here
>>>>> +            if (resource != null) {
>>>>> +                return resource.adaptTo(ValueMap.class);
>>>>> +            } else {
>>>>> +                return null;
>>>>> +            }
>>>>>        } else if (adaptable instanceof Adaptable) {
>>>>> -            ValueMap map = ((Adaptable)
>>>>> adaptable).adaptTo(ValueMap.class);
>>>>> -            return map;
>>>>> +            return ((Adaptable) 
>>>>> adaptable).adaptTo(ValueMap.class);
>>>>>        } else {
>>>>>            return null;
>>>>>        }
>>>>> 
>>>>> Modified:
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>>> URL:
>>>>> 
>>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>>> 
>>>>> 
>>> ==============================================================================
>>>>> ---
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>>> (original)
>>>>> +++
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>>> Fri May 13 10:35:03 2016
>>>>> @@ -84,6 +84,7 @@ public class ResourcePathInjector extend
>>>>> 
>>>>>        ResourceResolver resolver = getResourceResolver(adaptable);
>>>>>        if (resolver == null) {
>>>>> +            LOG.debug("Could not get resolver from adaptable {}",
>>>>> adaptable);
>>>>>            return null;
>>>>>        }
>>>>>        List<Resource> resources = getResources(resolver, 
>>>>> resourcePaths,
>>>>> name);
>>>>> 
>>>>> Modified:
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>>> URL:
>>>>> 
>>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>>> 
>>>>> 
>>> ==============================================================================
>>>>> ---
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>>> (original)
>>>>> +++
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>>> Fri May 13 10:35:03 2016
>>>>> @@ -169,12 +169,7 @@ public class ValueMapInjector extends Ab
>>>>>            if (StringUtils.isNotBlank(annotation.via())) {
>>>>>                return annotation.via();
>>>>>            }
>>>>> -            // automatically go via resource, if this is the
>>> httprequest
>>>>> -            if (adaptable instanceof SlingHttpServletRequest) {
>>>>> -                return "resource";
>>>>> -            } else {
>>>>> -                return null;
>>>>> -            }
>>>>> +            return null;
>>>>>        }
>>>>> 
>>>>>        @Override
>>>>> 
>>>>> Modified:
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>>> URL:
>>>>> 
>>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>>> 
>>>>> 
>>> ==============================================================================
>>>>> ---
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>>> (original)
>>>>> +++
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>>> Fri May 13 10:35:03 2016
>>>>> @@ -57,10 +57,10 @@ public class ResourcePathInjectionTest {
>>>>>    private ModelAdapterFactory factory;
>>>>> 
>>>>>    @Mock
>>>>> -    private Resource adaptable;
>>>>> +    private Resource adaptableResource;
>>>>> 
>>>>>    @Mock
>>>>> -    SlingHttpServletRequest nonResourceAdaptable;
>>>>> +    SlingHttpServletRequest adaptableRequest;
>>>>> 
>>>>>    @Mock
>>>>>    private Resource byPathResource;
>>>>> @@ -98,13 +98,16 @@ public class ResourcePathInjectionTest {
>>>>>        
>>>>> when(componentCtx.getBundleContext()).thenReturn(bundleContext);
>>>>>        when(componentCtx.getProperties()).thenReturn(new
>>>>> Hashtable<String, Object>());
>>>>> 
>>>>> -
>>>>> when(adaptable.getResourceResolver()).thenReturn(resourceResolver);
>>>>> -        
>>>>> when(adaptable.adaptTo(ValueMap.class)).thenReturn(properties);
>>>>> +
>>>>> 
>>> when(adaptableResource.getResourceResolver()).thenReturn(resourceResolver);
>>>>> +
>>>>> when(adaptableResource.adaptTo(ValueMap.class)).thenReturn(properties);
>>>>> 
>>>>> 
>>>>> 
>>> when(resourceResolver.getResource("/some/path")).thenReturn(byPathResource);
>>>>> 
>>>>> 
>>> when(resourceResolver.getResource("/some/path2")).thenReturn(byPathResource2);
>>>>> 
>>>>> 
>>> when(resourceResolver.getResource("/some/other/path")).thenReturn(byPropertyValueResource);
>>>>> 
>>>>> 
>>> when(resourceResolver.getResource("/some/other/path2")).thenReturn(byPropertyValueResource2);
>>>>> +
>>>>> +
>>> when(adaptableRequest.getResource()).thenReturn(byPathResource);
>>>>> +
>>>>> 
>>> when(adaptableRequest.getResourceResolver()).thenReturn(resourceResolver);
>>>>> 
>>>>>        factory = new ModelAdapterFactory();
>>>>>        factory.activate(componentCtx);
>>>>> @@ -116,8 +119,8 @@ public class ResourcePathInjectionTest {
>>>>>    }
>>>>> 
>>>>>    @Test
>>>>> -    public void testPathInjection() {
>>>>> -        ResourcePathModel model = factory.getAdapter(adaptable,
>>>>> ResourcePathModel.class);
>>>>> +    public void testPathInjectionFromResource() {
>>>>> +        ResourcePathModel model = 
>>>>> factory.getAdapter(adaptableResource,
>>>>> ResourcePathModel.class);
>>>>>        assertNotNull(model);
>>>>>        assertEquals(byPathResource, model.getFromPath());
>>>>>        assertEquals(byPropertyValueResource,
>>> model.getByDerefProperty());
>>>>> @@ -126,15 +129,20 @@ public class ResourcePathInjectionTest {
>>>>>    }
>>>>> 
>>>>>    @Test
>>>>> -    public void testPathInjectionWithNonResourceAdaptable() {
>>>>> -        ResourcePathModel model =
>>>>> factory.getAdapter(nonResourceAdaptable, ResourcePathModel.class);
>>>>> -        // should be null because mandatory fields could not be
>>> injected
>>>>> -        assertNull(model);
>>>>> +    public void testPathInjectionFromRequest() {
>>>>> +        // return the same properties through this request's 
>>>>> resource,
>>> as
>>>>> through adaptableResource
>>>>> +
>>>>> 
>>> doReturn(adaptableResource.adaptTo(ValueMap.class)).when(byPathResource).adaptTo(ValueMap.class);
>>>>> +        ResourcePathModel model = 
>>>>> factory.getAdapter(adaptableRequest,
>>>>> ResourcePathModel.class);
>>>>> +        assertNotNull(model);
>>>>> +        assertEquals(byPathResource, model.getFromPath());
>>>>> +        assertEquals(byPropertyValueResource,
>>> model.getByDerefProperty());
>>>>> +        assertEquals(byPathResource2, model.getFromPath2());
>>>>> +        assertEquals(byPropertyValueResource2,
>>>>> model.getByDerefProperty2());
>>>>>    }
>>>>> 
>>>>>    @Test
>>>>>    public void testOptionalPathInjectionWithNonResourceAdaptable() 
>>>>> {
>>>>> -        ResourcePathAllOptionalModel model =
>>>>> factory.getAdapter(nonResourceAdaptable,
>>>>> ResourcePathAllOptionalModel.class);
>>>>> +        ResourcePathAllOptionalModel model =
>>>>> factory.getAdapter(adaptableRequest,
>>> ResourcePathAllOptionalModel.class);
>>>>>        // should not be null because resource paths fields are 
>>>>> optional
>>>>>        assertNotNull(model);
>>>>>        // but the field itself are null
>>>>> @@ -146,7 +154,7 @@ public class ResourcePathInjectionTest {
>>>>> 
>>>>>    @Test
>>>>>    public void testMultiplePathInjection() {
>>>>> -        ResourcePathModel model = factory.getAdapter(adaptable,
>>>>> ResourcePathModel.class);
>>>>> +        ResourcePathModel model = 
>>>>> factory.getAdapter(adaptableResource,
>>>>> ResourcePathModel.class);
>>>>>        assertNotNull(model);
>>>>>        List<Resource> resources=model.getMultipleResources();
>>>>>        assertNotNull(resources);
>>>>> @@ -172,16 +180,16 @@ public class ResourcePathInjectionTest {
>>>>>    public void testPartialInjectionFailure1() {
>>>>> 
>>>>> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>>>>> 
>>>>> -        ResourcePathPartialModel model = 
>>>>> factory.getAdapter(adaptable,
>>>>> ResourcePathPartialModel.class);
>>>>> +        ResourcePathPartialModel model =
>>>>> factory.getAdapter(adaptableResource, 
>>>>> ResourcePathPartialModel.class);
>>>>>        assertNull(model);
>>>>>    }
>>>>> 
>>>>>    @Test
>>>>> -    public void testPartialInjectionFailure2() {
>>>>> +    public void testPartialInjectionFailure2() {
>>>>> 
>>>>> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>>>>> 
>>>>> 
>>> when(resourceResolver.getResource("/some/other/path2")).thenReturn(null);
>>>>> 
>>>>> -        ResourcePathPartialModel model = 
>>>>> factory.getAdapter(adaptable,
>>>>> ResourcePathPartialModel.class);
>>>>> +        ResourcePathPartialModel model =
>>>>> factory.getAdapter(adaptableResource, 
>>>>> ResourcePathPartialModel.class);
>>>>>        assertNull(model);
>>>>>    }
>>>>> 
>>>>> 
>>>>> Modified:
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>>> URL:
>>>>> 
>>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>>> 
>>>>> 
>>> ==============================================================================
>>>>> ---
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>>> (original)
>>>>> +++
>>>>> 
>>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>>> Fri May 13 10:35:03 2016
>>>>> @@ -32,7 +32,7 @@ import org.apache.sling.models.annotatio
>>>>> public class ResourcePathAllOptionalModel {
>>>>> 
>>>>>    @Inject
>>>>> -    @Path("/some/path")
>>>>> +    @Path("/some/invalidpath")
>>>>>    @Optional
>>>>>    private Resource fromPath;
>>>>> 
>>>>> @@ -42,11 +42,11 @@ public class ResourcePathAllOptionalMode
>>>>>    private Resource derefProperty;
>>>>> 
>>>>>    @Inject
>>>>> -    @Path(paths={"/some/path", "/some/path2"})
>>>>> +    @Path(paths={"/some/invalidpath", "/some/invalidpath2"})
>>>>>    @Optional
>>>>>    private Resource manyFromPathNonList;
>>>>> 
>>>>> -    @ResourcePath(path = "/some/path2", optional=true)
>>>>> +    @ResourcePath(path = "/some/invalidpath2", optional=true)
>>>>>    private Resource fromPath2;
>>>>> 
>>>>>    @ResourcePath(name = "anotherPropertyContainingAPath",
>>> optional=true)
>>>>> 
>>>>> 
>>>>> 
>>> 
>>>

Re: svn commit: r1743648 - in /sling: site/trunk/content/documentation/bundles/ trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ trun...

Posted by Konrad Windszus <ko...@gmx.de>.

I reverted the commit now and reopened https://issues.apache.org/jira/browse/SLING-5726.

To clarify my point of view:
I think each injector can decide on its own which adaptable to support natively. Whether or not a specific injector also supports accessing the value map from the underlying request's resource or not is IMHO a thing which
a) needs to be documented
b) is mostly driven by the fact how often consumer will try to use the injector with which adaptable

Regarding the point of consistency I think it would be even better if all OOTB injectors support SlingHttpRequestServlet! Some injectors may in addition support other adaptables. That way using those would be much easier.
I think not every developer remembers the details of each injector, so it might not be obvious which adaptable is supported. Instead of each time looking at the documentation, just supporting the minimum (i.e. SlingHttpServletRequest) seems to be the better choice here. Some developers do not even care which injector is injecting the value and where it comes from, but in case something does not work, very often the problem is, that just the wrong adaptable has been used (which fails silently and can only be traced down by increasing the log level, or just by knowing what is wrong).

Having via in addition does not do any harm IMHO, but to be honest I have not seen the usage of "via" very often in the past and to use "via" properly you have to know some details on the injector that I don't want every developer to be aware of.
That was also the reason, why for the annotation "ValueMapValue" the via element was automatically set to "resource" in case you were dealing with a SlingHttpServletRequest (https://github.com/apache/sling/blob/c431b4a0504f79a463593b7509cefb5ba15cda2c/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java#L174).

I hope my point is more clear now.
Konrad



> On 13 May 2016, at 13:11, Justin Edelson <ju...@justinedelson.com> wrote:
> 
> Hi Konrad,
> I think you are missing the point. The current recommended practice (to use
> @Via) is consistent across injectors. Once you start adding this
> functionality to individual injectors, this level of consistency is lost.
> If @Via isn't well understood, let's fix that problem. But making it
> unnecessary in some (but not all) cases just adds confusion.
> 
> Please go ahead and revert while you solicit other feedback. I'm doubtful
> that other feedback will cause me to revoke my veto, but I'm open to the
> idea.
> 
> Regards,
> Justin
> On Fri, May 13, 2016 at 11:57 AM Konrad Windszus <ko...@gmx.de> wrote:
> 
>> I know that you can use Via for that purpose, but it is very often
>> forgotten. And due to the way how injectors are asked this oversight is
>> very hard to debug and trace down.
>> 
>> Just allowing to directly adapt from a request for both injectors, makes
>> using those injectors a lot more comfortable.
>> 
>> The additional complexity I don't really see here, the only real change is
>> in
>> https://github.com/apache/sling/commit/c17cccfa17921f3a46003ce3840b1f3d3d59a490#diff-edbcc30dda0513662c6a3227c530ea38L47,
>> method getValueMap().
>> 
>> In addition it was never documented in
>> http://sling.apache.org/documentation/bundles/models.html#available-injectors
>> that resource-path injection only worked with paths retrieved from a
>> resource property, if the adaptable Resource was used.
>> It just states
>> Applicable to: Resource or SlingHttpServletRequest objects
>> 
>> So IMHO this commit fixes a bug in ResourcePathInjector and makes using
>> ValueMapInjector just more comfortable.
>> I would be interested to hear other opinions on that change.
>> Thanks,
>> Konrad
>> 
>> 
>>> On 13 May 2016, at 12:45, Justin Edelson <ju...@justinedelson.com>
>> wrote:
>>> 
>>> -1 to this change (would comment in JIRA but I can't for some reason
>> right
>>> now)
>>> 
>>> This adds unnecessary complexity and is poor seperation of concerns. The
>>> @Via annotation can be used for this use case (and is documented as
>> such).
>>> On Fri, May 13, 2016 at 11:35 AM <kw...@apache.org> wrote:
>>> 
>>>> Author: kwin
>>>> Date: Fri May 13 10:35:03 2016
>>>> New Revision: 1743648
>>>> 
>>>> URL: http://svn.apache.org/viewvc?rev=1743648&view=rev
>>>> Log:
>>>> SLING-5726 allow both ValueMapInjector and ResourcePathInjector to act
>> on
>>>> adaptable SlingHttpServletRequest in all circumstances
>>>> 
>>>> Modified:
>>>>   sling/site/trunk/content/documentation/bundles/models.mdtext
>>>> 
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>> 
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>> 
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>> 
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>> 
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>> 
>>>> Modified: sling/site/trunk/content/documentation/bundles/models.mdtext
>>>> URL:
>>>> 
>> http://svn.apache.org/viewvc/sling/site/trunk/content/documentation/bundles/models.mdtext?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>> 
>>>> 
>> ==============================================================================
>>>> --- sling/site/trunk/content/documentation/bundles/models.mdtext
>> (original)
>>>> +++ sling/site/trunk/content/documentation/bundles/models.mdtext Fri May
>>>> 13 10:35:03 2016
>>>> @@ -366,7 +366,7 @@ In addition all [injector-specific annot
>>>> Title              |  Name (for `@Source`)   | Service Ranking     |
>>>> Available Since (Implementation Version) | Description  | Applicable To
>>>> (including using `@Via`) | Accepts Null Name? | Array Support |
>>>> Parameterized Type Support
>>>> -----------------  | ----------------------- | ------------------- |
>>>> ---------------------------------------- | ------------ |
>>>> -------------------------------------- | ------------------ |
>> -------------
>>>> | --------------------------
>>>> Script Bindings    | `script-bindings`       | 1000                |
>>>> 1.0.0                                    | Lookup objects in the script
>>>> bindings object by name. | A ServletRequest object which has the `Sling
>>>> Bindings` attribute defined | no | no conversion is done | If a
>>>> parameterized type is passed, the bindings value must be of a compatible
>>>> type of the parameterized type.
>>>> -Value Map          | `valuemap`              | 2000                |
>>>> 1.0.0                                    | Gets a property from a
>>>> `ValueMap` by name. | Any object which is or can be adapted to a
>>>> `ValueMap`| no | Primitive arrays wrapped/unwrapped as necessary.
>> Wrapper
>>>> object arrays are unwrapped/wrapped as necessary. | Parameterized `List`
>>>> and `Collection` injection points are injected by getting an array of
>> the
>>>> component type and creating an unmodifiable `List` from the array.
>>>> +Value Map          | `valuemap`              | 2000                |
>>>> 1.0.0                                    | Gets a property from a
>>>> `ValueMap` by name. | Any object which is or can be adapted to a
>>>> `ValueMap`. Since version [1.2.10](
>>>> https://issues.apache.org/jira/browse/SLING-5726) also
>>>> `SlingHttpServletRequest`. | no | Primitive arrays wrapped/unwrapped as
>>>> necessary. Wrapper object arrays are unwrapped/wrapped as necessary. |
>>>> Parameterized `List` and `Collection` injection points are injected by
>>>> getting an array of the component type and creating an unmodifiable
>> `List`
>>>> from the array.
>>>> Child Resources    | `child-resources`       | 3000                |
>>>> 1.0.0                                    | Gets a child resource by
>> name. |
>>>> `Resource` objects | no | none | if a parameterized type is passed, a
>>>> `List<Resource>` is returned (the contents of which may be adapted to
>> the
>>>> target type).
>>>> Request Attributes | `request-attributes`    | 4000                |
>>>> 1.0.0                                    | Get a request attribute by
>> name.
>>>> | `ServletRequest` objects | no | no conversion is done | If a
>>>> parameterized type is passed, the request attribute must be of a
>> compatible
>>>> type of the parameterized type.
>>>> OSGi Services      | `osgi-services`         | 5000                |
>>>> 1.0.0                                    | Lookup services based on
>> class
>>>> name. Since Sling Models Impl 1.2.8 ([SLING-5664](
>>>> https://issues.apache.org/jira/browse/SLING-5664)) the service with the
>>>> highest service ranking is returned. In case multiple services are
>>>> returned, they are ordered descending by their service ranking (i.e. the
>>>> one with the highest ranking first). | Any object | yes | yes |
>>>> Parameterized `List` and `Collection` injection points are injected by
>>>> getting an array of the services and creating an unmodifiable `List`
>> from
>>>> the array.
>>>> 
>>>> Modified:
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>> URL:
>>>> 
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>> 
>>>> 
>> ==============================================================================
>>>> ---
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>> (original)
>>>> +++
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>>>> Fri May 13 10:35:03 2016
>>>> @@ -44,12 +44,30 @@ abstract class AbstractInjector {
>>>>        return resolver;
>>>>    }
>>>> 
>>>> +    /**
>>>> +     * Retrieve the ValueMap from the given adaptable. This succeeds,
>> if
>>>> the adaptable is either
>>>> +     * <ul>
>>>> +     * <li>a {@link ValueMap},</li>
>>>> +     * <li>a {@link SlingHttpServletRequest}, in which case the
>> returned
>>>> {@link ValueMap} is the one derived from the request's resource or</li>
>>>> +     * <li>adaptable to a {@link ValueMap}.</li>
>>>> +     * </ul>
>>>> +     * Otherwise {@code null} is returned.
>>>> +     * @param adaptable
>>>> +     * @return a ValueMap or {@code null}.
>>>> +     */
>>>>    protected ValueMap getValueMap(Object adaptable) {
>>>>        if (adaptable instanceof ValueMap) {
>>>>            return (ValueMap) adaptable;
>>>> +        } else if (adaptable instanceof SlingHttpServletRequest) {
>>>> +            final Resource resource =
>>>> ((SlingHttpServletRequest)adaptable).getResource();
>>>> +            // resource may be null for mocked adaptables, therefore
>> do a
>>>> check here
>>>> +            if (resource != null) {
>>>> +                return resource.adaptTo(ValueMap.class);
>>>> +            } else {
>>>> +                return null;
>>>> +            }
>>>>        } else if (adaptable instanceof Adaptable) {
>>>> -            ValueMap map = ((Adaptable)
>>>> adaptable).adaptTo(ValueMap.class);
>>>> -            return map;
>>>> +            return ((Adaptable) adaptable).adaptTo(ValueMap.class);
>>>>        } else {
>>>>            return null;
>>>>        }
>>>> 
>>>> Modified:
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>> URL:
>>>> 
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>> 
>>>> 
>> ==============================================================================
>>>> ---
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>> (original)
>>>> +++
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>>>> Fri May 13 10:35:03 2016
>>>> @@ -84,6 +84,7 @@ public class ResourcePathInjector extend
>>>> 
>>>>        ResourceResolver resolver = getResourceResolver(adaptable);
>>>>        if (resolver == null) {
>>>> +            LOG.debug("Could not get resolver from adaptable {}",
>>>> adaptable);
>>>>            return null;
>>>>        }
>>>>        List<Resource> resources = getResources(resolver, resourcePaths,
>>>> name);
>>>> 
>>>> Modified:
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>> URL:
>>>> 
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>> 
>>>> 
>> ==============================================================================
>>>> ---
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>> (original)
>>>> +++
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>>>> Fri May 13 10:35:03 2016
>>>> @@ -169,12 +169,7 @@ public class ValueMapInjector extends Ab
>>>>            if (StringUtils.isNotBlank(annotation.via())) {
>>>>                return annotation.via();
>>>>            }
>>>> -            // automatically go via resource, if this is the
>> httprequest
>>>> -            if (adaptable instanceof SlingHttpServletRequest) {
>>>> -                return "resource";
>>>> -            } else {
>>>> -                return null;
>>>> -            }
>>>> +            return null;
>>>>        }
>>>> 
>>>>        @Override
>>>> 
>>>> Modified:
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>> URL:
>>>> 
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>> 
>>>> 
>> ==============================================================================
>>>> ---
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>> (original)
>>>> +++
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>>>> Fri May 13 10:35:03 2016
>>>> @@ -57,10 +57,10 @@ public class ResourcePathInjectionTest {
>>>>    private ModelAdapterFactory factory;
>>>> 
>>>>    @Mock
>>>> -    private Resource adaptable;
>>>> +    private Resource adaptableResource;
>>>> 
>>>>    @Mock
>>>> -    SlingHttpServletRequest nonResourceAdaptable;
>>>> +    SlingHttpServletRequest adaptableRequest;
>>>> 
>>>>    @Mock
>>>>    private Resource byPathResource;
>>>> @@ -98,13 +98,16 @@ public class ResourcePathInjectionTest {
>>>>        when(componentCtx.getBundleContext()).thenReturn(bundleContext);
>>>>        when(componentCtx.getProperties()).thenReturn(new
>>>> Hashtable<String, Object>());
>>>> 
>>>> -
>>>> when(adaptable.getResourceResolver()).thenReturn(resourceResolver);
>>>> -        when(adaptable.adaptTo(ValueMap.class)).thenReturn(properties);
>>>> +
>>>> 
>> when(adaptableResource.getResourceResolver()).thenReturn(resourceResolver);
>>>> +
>>>> when(adaptableResource.adaptTo(ValueMap.class)).thenReturn(properties);
>>>> 
>>>> 
>>>> 
>> when(resourceResolver.getResource("/some/path")).thenReturn(byPathResource);
>>>> 
>>>> 
>> when(resourceResolver.getResource("/some/path2")).thenReturn(byPathResource2);
>>>> 
>>>> 
>> when(resourceResolver.getResource("/some/other/path")).thenReturn(byPropertyValueResource);
>>>> 
>>>> 
>> when(resourceResolver.getResource("/some/other/path2")).thenReturn(byPropertyValueResource2);
>>>> +
>>>> +
>> when(adaptableRequest.getResource()).thenReturn(byPathResource);
>>>> +
>>>> 
>> when(adaptableRequest.getResourceResolver()).thenReturn(resourceResolver);
>>>> 
>>>>        factory = new ModelAdapterFactory();
>>>>        factory.activate(componentCtx);
>>>> @@ -116,8 +119,8 @@ public class ResourcePathInjectionTest {
>>>>    }
>>>> 
>>>>    @Test
>>>> -    public void testPathInjection() {
>>>> -        ResourcePathModel model = factory.getAdapter(adaptable,
>>>> ResourcePathModel.class);
>>>> +    public void testPathInjectionFromResource() {
>>>> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
>>>> ResourcePathModel.class);
>>>>        assertNotNull(model);
>>>>        assertEquals(byPathResource, model.getFromPath());
>>>>        assertEquals(byPropertyValueResource,
>> model.getByDerefProperty());
>>>> @@ -126,15 +129,20 @@ public class ResourcePathInjectionTest {
>>>>    }
>>>> 
>>>>    @Test
>>>> -    public void testPathInjectionWithNonResourceAdaptable() {
>>>> -        ResourcePathModel model =
>>>> factory.getAdapter(nonResourceAdaptable, ResourcePathModel.class);
>>>> -        // should be null because mandatory fields could not be
>> injected
>>>> -        assertNull(model);
>>>> +    public void testPathInjectionFromRequest() {
>>>> +        // return the same properties through this request's resource,
>> as
>>>> through adaptableResource
>>>> +
>>>> 
>> doReturn(adaptableResource.adaptTo(ValueMap.class)).when(byPathResource).adaptTo(ValueMap.class);
>>>> +        ResourcePathModel model = factory.getAdapter(adaptableRequest,
>>>> ResourcePathModel.class);
>>>> +        assertNotNull(model);
>>>> +        assertEquals(byPathResource, model.getFromPath());
>>>> +        assertEquals(byPropertyValueResource,
>> model.getByDerefProperty());
>>>> +        assertEquals(byPathResource2, model.getFromPath2());
>>>> +        assertEquals(byPropertyValueResource2,
>>>> model.getByDerefProperty2());
>>>>    }
>>>> 
>>>>    @Test
>>>>    public void testOptionalPathInjectionWithNonResourceAdaptable() {
>>>> -        ResourcePathAllOptionalModel model =
>>>> factory.getAdapter(nonResourceAdaptable,
>>>> ResourcePathAllOptionalModel.class);
>>>> +        ResourcePathAllOptionalModel model =
>>>> factory.getAdapter(adaptableRequest,
>> ResourcePathAllOptionalModel.class);
>>>>        // should not be null because resource paths fields are optional
>>>>        assertNotNull(model);
>>>>        // but the field itself are null
>>>> @@ -146,7 +154,7 @@ public class ResourcePathInjectionTest {
>>>> 
>>>>    @Test
>>>>    public void testMultiplePathInjection() {
>>>> -        ResourcePathModel model = factory.getAdapter(adaptable,
>>>> ResourcePathModel.class);
>>>> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
>>>> ResourcePathModel.class);
>>>>        assertNotNull(model);
>>>>        List<Resource> resources=model.getMultipleResources();
>>>>        assertNotNull(resources);
>>>> @@ -172,16 +180,16 @@ public class ResourcePathInjectionTest {
>>>>    public void testPartialInjectionFailure1() {
>>>> 
>>>> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>>>> 
>>>> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
>>>> ResourcePathPartialModel.class);
>>>> +        ResourcePathPartialModel model =
>>>> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
>>>>        assertNull(model);
>>>>    }
>>>> 
>>>>    @Test
>>>> -    public void testPartialInjectionFailure2() {
>>>> +    public void testPartialInjectionFailure2() {
>>>> 
>>>> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>>>> 
>>>> 
>> when(resourceResolver.getResource("/some/other/path2")).thenReturn(null);
>>>> 
>>>> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
>>>> ResourcePathPartialModel.class);
>>>> +        ResourcePathPartialModel model =
>>>> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
>>>>        assertNull(model);
>>>>    }
>>>> 
>>>> 
>>>> Modified:
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>> URL:
>>>> 
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>>>> 
>>>> 
>> ==============================================================================
>>>> ---
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>> (original)
>>>> +++
>>>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>>>> Fri May 13 10:35:03 2016
>>>> @@ -32,7 +32,7 @@ import org.apache.sling.models.annotatio
>>>> public class ResourcePathAllOptionalModel {
>>>> 
>>>>    @Inject
>>>> -    @Path("/some/path")
>>>> +    @Path("/some/invalidpath")
>>>>    @Optional
>>>>    private Resource fromPath;
>>>> 
>>>> @@ -42,11 +42,11 @@ public class ResourcePathAllOptionalMode
>>>>    private Resource derefProperty;
>>>> 
>>>>    @Inject
>>>> -    @Path(paths={"/some/path", "/some/path2"})
>>>> +    @Path(paths={"/some/invalidpath", "/some/invalidpath2"})
>>>>    @Optional
>>>>    private Resource manyFromPathNonList;
>>>> 
>>>> -    @ResourcePath(path = "/some/path2", optional=true)
>>>> +    @ResourcePath(path = "/some/invalidpath2", optional=true)
>>>>    private Resource fromPath2;
>>>> 
>>>>    @ResourcePath(name = "anotherPropertyContainingAPath",
>> optional=true)
>>>> 
>>>> 
>>>> 
>> 
>>

Re: svn commit: r1743648 - in /sling: site/trunk/content/documentation/bundles/ trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ trun...

Posted by Justin Edelson <ju...@justinedelson.com>.

Hi Konrad,
I think you are missing the point. The current recommended practice (to use
@Via) is consistent across injectors. Once you start adding this
functionality to individual injectors, this level of consistency is lost.
If @Via isn't well understood, let's fix that problem. But making it
unnecessary in some (but not all) cases just adds confusion.

Please go ahead and revert while you solicit other feedback. I'm doubtful
that other feedback will cause me to revoke my veto, but I'm open to the
idea.

Regards,
Justin
On Fri, May 13, 2016 at 11:57 AM Konrad Windszus <ko...@gmx.de> wrote:

> I know that you can use Via for that purpose, but it is very often
> forgotten. And due to the way how injectors are asked this oversight is
> very hard to debug and trace down.
>
> Just allowing to directly adapt from a request for both injectors, makes
> using those injectors a lot more comfortable.
>
> The additional complexity I don't really see here, the only real change is
> in
> https://github.com/apache/sling/commit/c17cccfa17921f3a46003ce3840b1f3d3d59a490#diff-edbcc30dda0513662c6a3227c530ea38L47,
> method getValueMap().
>
> In addition it was never documented in
> http://sling.apache.org/documentation/bundles/models.html#available-injectors
> that resource-path injection only worked with paths retrieved from a
> resource property, if the adaptable Resource was used.
> It just states
> Applicable to: Resource or SlingHttpServletRequest objects
>
> So IMHO this commit fixes a bug in ResourcePathInjector and makes using
> ValueMapInjector just more comfortable.
> I would be interested to hear other opinions on that change.
> Thanks,
> Konrad
>
>
> > On 13 May 2016, at 12:45, Justin Edelson <ju...@justinedelson.com>
> wrote:
> >
> > -1 to this change (would comment in JIRA but I can't for some reason
> right
> > now)
> >
> > This adds unnecessary complexity and is poor seperation of concerns. The
> > @Via annotation can be used for this use case (and is documented as
> such).
> > On Fri, May 13, 2016 at 11:35 AM <kw...@apache.org> wrote:
> >
> >> Author: kwin
> >> Date: Fri May 13 10:35:03 2016
> >> New Revision: 1743648
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1743648&view=rev
> >> Log:
> >> SLING-5726 allow both ValueMapInjector and ResourcePathInjector to act
> on
> >> adaptable SlingHttpServletRequest in all circumstances
> >>
> >> Modified:
> >>    sling/site/trunk/content/documentation/bundles/models.mdtext
> >>
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >>
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >>
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >>
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >>
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >>
> >> Modified: sling/site/trunk/content/documentation/bundles/models.mdtext
> >> URL:
> >>
> http://svn.apache.org/viewvc/sling/site/trunk/content/documentation/bundles/models.mdtext?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>
> >>
> ==============================================================================
> >> --- sling/site/trunk/content/documentation/bundles/models.mdtext
> (original)
> >> +++ sling/site/trunk/content/documentation/bundles/models.mdtext Fri May
> >> 13 10:35:03 2016
> >> @@ -366,7 +366,7 @@ In addition all [injector-specific annot
> >> Title              |  Name (for `@Source`)   | Service Ranking     |
> >> Available Since (Implementation Version) | Description  | Applicable To
> >> (including using `@Via`) | Accepts Null Name? | Array Support |
> >> Parameterized Type Support
> >> -----------------  | ----------------------- | ------------------- |
> >> ---------------------------------------- | ------------ |
> >> -------------------------------------- | ------------------ |
> -------------
> >> | --------------------------
> >> Script Bindings    | `script-bindings`       | 1000                |
> >> 1.0.0                                    | Lookup objects in the script
> >> bindings object by name. | A ServletRequest object which has the `Sling
> >> Bindings` attribute defined | no | no conversion is done | If a
> >> parameterized type is passed, the bindings value must be of a compatible
> >> type of the parameterized type.
> >> -Value Map          | `valuemap`              | 2000                |
> >> 1.0.0                                    | Gets a property from a
> >> `ValueMap` by name. | Any object which is or can be adapted to a
> >> `ValueMap`| no | Primitive arrays wrapped/unwrapped as necessary.
> Wrapper
> >> object arrays are unwrapped/wrapped as necessary. | Parameterized `List`
> >> and `Collection` injection points are injected by getting an array of
> the
> >> component type and creating an unmodifiable `List` from the array.
> >> +Value Map          | `valuemap`              | 2000                |
> >> 1.0.0                                    | Gets a property from a
> >> `ValueMap` by name. | Any object which is or can be adapted to a
> >> `ValueMap`. Since version [1.2.10](
> >> https://issues.apache.org/jira/browse/SLING-5726) also
> >> `SlingHttpServletRequest`. | no | Primitive arrays wrapped/unwrapped as
> >> necessary. Wrapper object arrays are unwrapped/wrapped as necessary. |
> >> Parameterized `List` and `Collection` injection points are injected by
> >> getting an array of the component type and creating an unmodifiable
> `List`
> >> from the array.
> >> Child Resources    | `child-resources`       | 3000                |
> >> 1.0.0                                    | Gets a child resource by
> name. |
> >> `Resource` objects | no | none | if a parameterized type is passed, a
> >> `List<Resource>` is returned (the contents of which may be adapted to
> the
> >> target type).
> >> Request Attributes | `request-attributes`    | 4000                |
> >> 1.0.0                                    | Get a request attribute by
> name.
> >> | `ServletRequest` objects | no | no conversion is done | If a
> >> parameterized type is passed, the request attribute must be of a
> compatible
> >> type of the parameterized type.
> >> OSGi Services      | `osgi-services`         | 5000                |
> >> 1.0.0                                    | Lookup services based on
> class
> >> name. Since Sling Models Impl 1.2.8 ([SLING-5664](
> >> https://issues.apache.org/jira/browse/SLING-5664)) the service with the
> >> highest service ranking is returned. In case multiple services are
> >> returned, they are ordered descending by their service ranking (i.e. the
> >> one with the highest ranking first). | Any object | yes | yes |
> >> Parameterized `List` and `Collection` injection points are injected by
> >> getting an array of the services and creating an unmodifiable `List`
> from
> >> the array.
> >>
> >> Modified:
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >> (original)
> >> +++
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
> >> Fri May 13 10:35:03 2016
> >> @@ -44,12 +44,30 @@ abstract class AbstractInjector {
> >>         return resolver;
> >>     }
> >>
> >> +    /**
> >> +     * Retrieve the ValueMap from the given adaptable. This succeeds,
> if
> >> the adaptable is either
> >> +     * <ul>
> >> +     * <li>a {@link ValueMap},</li>
> >> +     * <li>a {@link SlingHttpServletRequest}, in which case the
> returned
> >> {@link ValueMap} is the one derived from the request's resource or</li>
> >> +     * <li>adaptable to a {@link ValueMap}.</li>
> >> +     * </ul>
> >> +     * Otherwise {@code null} is returned.
> >> +     * @param adaptable
> >> +     * @return a ValueMap or {@code null}.
> >> +     */
> >>     protected ValueMap getValueMap(Object adaptable) {
> >>         if (adaptable instanceof ValueMap) {
> >>             return (ValueMap) adaptable;
> >> +        } else if (adaptable instanceof SlingHttpServletRequest) {
> >> +            final Resource resource =
> >> ((SlingHttpServletRequest)adaptable).getResource();
> >> +            // resource may be null for mocked adaptables, therefore
> do a
> >> check here
> >> +            if (resource != null) {
> >> +                return resource.adaptTo(ValueMap.class);
> >> +            } else {
> >> +                return null;
> >> +            }
> >>         } else if (adaptable instanceof Adaptable) {
> >> -            ValueMap map = ((Adaptable)
> >> adaptable).adaptTo(ValueMap.class);
> >> -            return map;
> >> +            return ((Adaptable) adaptable).adaptTo(ValueMap.class);
> >>         } else {
> >>             return null;
> >>         }
> >>
> >> Modified:
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >> (original)
> >> +++
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
> >> Fri May 13 10:35:03 2016
> >> @@ -84,6 +84,7 @@ public class ResourcePathInjector extend
> >>
> >>         ResourceResolver resolver = getResourceResolver(adaptable);
> >>         if (resolver == null) {
> >> +            LOG.debug("Could not get resolver from adaptable {}",
> >> adaptable);
> >>             return null;
> >>         }
> >>         List<Resource> resources = getResources(resolver, resourcePaths,
> >> name);
> >>
> >> Modified:
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >> (original)
> >> +++
> >>
> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
> >> Fri May 13 10:35:03 2016
> >> @@ -169,12 +169,7 @@ public class ValueMapInjector extends Ab
> >>             if (StringUtils.isNotBlank(annotation.via())) {
> >>                 return annotation.via();
> >>             }
> >> -            // automatically go via resource, if this is the
> httprequest
> >> -            if (adaptable instanceof SlingHttpServletRequest) {
> >> -                return "resource";
> >> -            } else {
> >> -                return null;
> >> -            }
> >> +            return null;
> >>         }
> >>
> >>         @Override
> >>
> >> Modified:
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >> (original)
> >> +++
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
> >> Fri May 13 10:35:03 2016
> >> @@ -57,10 +57,10 @@ public class ResourcePathInjectionTest {
> >>     private ModelAdapterFactory factory;
> >>
> >>     @Mock
> >> -    private Resource adaptable;
> >> +    private Resource adaptableResource;
> >>
> >>     @Mock
> >> -    SlingHttpServletRequest nonResourceAdaptable;
> >> +    SlingHttpServletRequest adaptableRequest;
> >>
> >>     @Mock
> >>     private Resource byPathResource;
> >> @@ -98,13 +98,16 @@ public class ResourcePathInjectionTest {
> >>         when(componentCtx.getBundleContext()).thenReturn(bundleContext);
> >>         when(componentCtx.getProperties()).thenReturn(new
> >> Hashtable<String, Object>());
> >>
> >> -
> >> when(adaptable.getResourceResolver()).thenReturn(resourceResolver);
> >> -        when(adaptable.adaptTo(ValueMap.class)).thenReturn(properties);
> >> +
> >>
> when(adaptableResource.getResourceResolver()).thenReturn(resourceResolver);
> >> +
> >> when(adaptableResource.adaptTo(ValueMap.class)).thenReturn(properties);
> >>
> >>
> >>
> when(resourceResolver.getResource("/some/path")).thenReturn(byPathResource);
> >>
> >>
> when(resourceResolver.getResource("/some/path2")).thenReturn(byPathResource2);
> >>
> >>
> when(resourceResolver.getResource("/some/other/path")).thenReturn(byPropertyValueResource);
> >>
> >>
> when(resourceResolver.getResource("/some/other/path2")).thenReturn(byPropertyValueResource2);
> >> +
> >> +
> when(adaptableRequest.getResource()).thenReturn(byPathResource);
> >> +
> >>
> when(adaptableRequest.getResourceResolver()).thenReturn(resourceResolver);
> >>
> >>         factory = new ModelAdapterFactory();
> >>         factory.activate(componentCtx);
> >> @@ -116,8 +119,8 @@ public class ResourcePathInjectionTest {
> >>     }
> >>
> >>     @Test
> >> -    public void testPathInjection() {
> >> -        ResourcePathModel model = factory.getAdapter(adaptable,
> >> ResourcePathModel.class);
> >> +    public void testPathInjectionFromResource() {
> >> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
> >> ResourcePathModel.class);
> >>         assertNotNull(model);
> >>         assertEquals(byPathResource, model.getFromPath());
> >>         assertEquals(byPropertyValueResource,
> model.getByDerefProperty());
> >> @@ -126,15 +129,20 @@ public class ResourcePathInjectionTest {
> >>     }
> >>
> >>     @Test
> >> -    public void testPathInjectionWithNonResourceAdaptable() {
> >> -        ResourcePathModel model =
> >> factory.getAdapter(nonResourceAdaptable, ResourcePathModel.class);
> >> -        // should be null because mandatory fields could not be
> injected
> >> -        assertNull(model);
> >> +    public void testPathInjectionFromRequest() {
> >> +        // return the same properties through this request's resource,
> as
> >> through adaptableResource
> >> +
> >>
> doReturn(adaptableResource.adaptTo(ValueMap.class)).when(byPathResource).adaptTo(ValueMap.class);
> >> +        ResourcePathModel model = factory.getAdapter(adaptableRequest,
> >> ResourcePathModel.class);
> >> +        assertNotNull(model);
> >> +        assertEquals(byPathResource, model.getFromPath());
> >> +        assertEquals(byPropertyValueResource,
> model.getByDerefProperty());
> >> +        assertEquals(byPathResource2, model.getFromPath2());
> >> +        assertEquals(byPropertyValueResource2,
> >> model.getByDerefProperty2());
> >>     }
> >>
> >>     @Test
> >>     public void testOptionalPathInjectionWithNonResourceAdaptable() {
> >> -        ResourcePathAllOptionalModel model =
> >> factory.getAdapter(nonResourceAdaptable,
> >> ResourcePathAllOptionalModel.class);
> >> +        ResourcePathAllOptionalModel model =
> >> factory.getAdapter(adaptableRequest,
> ResourcePathAllOptionalModel.class);
> >>         // should not be null because resource paths fields are optional
> >>         assertNotNull(model);
> >>         // but the field itself are null
> >> @@ -146,7 +154,7 @@ public class ResourcePathInjectionTest {
> >>
> >>     @Test
> >>     public void testMultiplePathInjection() {
> >> -        ResourcePathModel model = factory.getAdapter(adaptable,
> >> ResourcePathModel.class);
> >> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
> >> ResourcePathModel.class);
> >>         assertNotNull(model);
> >>         List<Resource> resources=model.getMultipleResources();
> >>         assertNotNull(resources);
> >> @@ -172,16 +180,16 @@ public class ResourcePathInjectionTest {
> >>     public void testPartialInjectionFailure1() {
> >>
> >> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
> >>
> >> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
> >> ResourcePathPartialModel.class);
> >> +        ResourcePathPartialModel model =
> >> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
> >>         assertNull(model);
> >>     }
> >>
> >>     @Test
> >> -    public void testPartialInjectionFailure2() {
> >> +    public void testPartialInjectionFailure2() {
> >>
> >> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
> >>
> >>
> when(resourceResolver.getResource("/some/other/path2")).thenReturn(null);
> >>
> >> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
> >> ResourcePathPartialModel.class);
> >> +        ResourcePathPartialModel model =
> >> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
> >>         assertNull(model);
> >>     }
> >>
> >>
> >> Modified:
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java?rev=1743648&r1=1743647&r2=1743648&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >> (original)
> >> +++
> >>
> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
> >> Fri May 13 10:35:03 2016
> >> @@ -32,7 +32,7 @@ import org.apache.sling.models.annotatio
> >> public class ResourcePathAllOptionalModel {
> >>
> >>     @Inject
> >> -    @Path("/some/path")
> >> +    @Path("/some/invalidpath")
> >>     @Optional
> >>     private Resource fromPath;
> >>
> >> @@ -42,11 +42,11 @@ public class ResourcePathAllOptionalMode
> >>     private Resource derefProperty;
> >>
> >>     @Inject
> >> -    @Path(paths={"/some/path", "/some/path2"})
> >> +    @Path(paths={"/some/invalidpath", "/some/invalidpath2"})
> >>     @Optional
> >>     private Resource manyFromPathNonList;
> >>
> >> -    @ResourcePath(path = "/some/path2", optional=true)
> >> +    @ResourcePath(path = "/some/invalidpath2", optional=true)
> >>     private Resource fromPath2;
> >>
> >>     @ResourcePath(name = "anotherPropertyContainingAPath",
> optional=true)
> >>
> >>
> >>
>
>

Re: svn commit: r1743648 - in /sling: site/trunk/content/documentation/bundles/ trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ trun...

Posted by Konrad Windszus <ko...@gmx.de>.

I know that you can use Via for that purpose, but it is very often forgotten. And due to the way how injectors are asked this oversight is very hard to debug and trace down.

Just allowing to directly adapt from a request for both injectors, makes using those injectors a lot more comfortable.

The additional complexity I don't really see here, the only real change is in https://github.com/apache/sling/commit/c17cccfa17921f3a46003ce3840b1f3d3d59a490#diff-edbcc30dda0513662c6a3227c530ea38L47, method getValueMap().

In addition it was never documented in http://sling.apache.org/documentation/bundles/models.html#available-injectors that resource-path injection only worked with paths retrieved from a resource property, if the adaptable Resource was used.
It just states 
Applicable to: Resource or SlingHttpServletRequest objects

So IMHO this commit fixes a bug in ResourcePathInjector and makes using ValueMapInjector just more comfortable.
I would be interested to hear other opinions on that change.
Thanks,
Konrad


> On 13 May 2016, at 12:45, Justin Edelson <ju...@justinedelson.com> wrote:
> 
> -1 to this change (would comment in JIRA but I can't for some reason right
> now)
> 
> This adds unnecessary complexity and is poor seperation of concerns. The
> @Via annotation can be used for this use case (and is documented as such).
> On Fri, May 13, 2016 at 11:35 AM <kw...@apache.org> wrote:
> 
>> Author: kwin
>> Date: Fri May 13 10:35:03 2016
>> New Revision: 1743648
>> 
>> URL: http://svn.apache.org/viewvc?rev=1743648&view=rev
>> Log:
>> SLING-5726 allow both ValueMapInjector and ResourcePathInjector to act on
>> adaptable SlingHttpServletRequest in all circumstances
>> 
>> Modified:
>>    sling/site/trunk/content/documentation/bundles/models.mdtext
>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>> 
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>> 
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>> 
>> Modified: sling/site/trunk/content/documentation/bundles/models.mdtext
>> URL:
>> http://svn.apache.org/viewvc/sling/site/trunk/content/documentation/bundles/models.mdtext?rev=1743648&r1=1743647&r2=1743648&view=diff
>> 
>> ==============================================================================
>> --- sling/site/trunk/content/documentation/bundles/models.mdtext (original)
>> +++ sling/site/trunk/content/documentation/bundles/models.mdtext Fri May
>> 13 10:35:03 2016
>> @@ -366,7 +366,7 @@ In addition all [injector-specific annot
>> Title              |  Name (for `@Source`)   | Service Ranking     |
>> Available Since (Implementation Version) | Description  | Applicable To
>> (including using `@Via`) | Accepts Null Name? | Array Support |
>> Parameterized Type Support
>> -----------------  | ----------------------- | ------------------- |
>> ---------------------------------------- | ------------ |
>> -------------------------------------- | ------------------ | -------------
>> | --------------------------
>> Script Bindings    | `script-bindings`       | 1000                |
>> 1.0.0                                    | Lookup objects in the script
>> bindings object by name. | A ServletRequest object which has the `Sling
>> Bindings` attribute defined | no | no conversion is done | If a
>> parameterized type is passed, the bindings value must be of a compatible
>> type of the parameterized type.
>> -Value Map          | `valuemap`              | 2000                |
>> 1.0.0                                    | Gets a property from a
>> `ValueMap` by name. | Any object which is or can be adapted to a
>> `ValueMap`| no | Primitive arrays wrapped/unwrapped as necessary. Wrapper
>> object arrays are unwrapped/wrapped as necessary. | Parameterized `List`
>> and `Collection` injection points are injected by getting an array of the
>> component type and creating an unmodifiable `List` from the array.
>> +Value Map          | `valuemap`              | 2000                |
>> 1.0.0                                    | Gets a property from a
>> `ValueMap` by name. | Any object which is or can be adapted to a
>> `ValueMap`. Since version [1.2.10](
>> https://issues.apache.org/jira/browse/SLING-5726) also
>> `SlingHttpServletRequest`. | no | Primitive arrays wrapped/unwrapped as
>> necessary. Wrapper object arrays are unwrapped/wrapped as necessary. |
>> Parameterized `List` and `Collection` injection points are injected by
>> getting an array of the component type and creating an unmodifiable `List`
>> from the array.
>> Child Resources    | `child-resources`       | 3000                |
>> 1.0.0                                    | Gets a child resource by name. |
>> `Resource` objects | no | none | if a parameterized type is passed, a
>> `List<Resource>` is returned (the contents of which may be adapted to the
>> target type).
>> Request Attributes | `request-attributes`    | 4000                |
>> 1.0.0                                    | Get a request attribute by name.
>> | `ServletRequest` objects | no | no conversion is done | If a
>> parameterized type is passed, the request attribute must be of a compatible
>> type of the parameterized type.
>> OSGi Services      | `osgi-services`         | 5000                |
>> 1.0.0                                    | Lookup services based on class
>> name. Since Sling Models Impl 1.2.8 ([SLING-5664](
>> https://issues.apache.org/jira/browse/SLING-5664)) the service with the
>> highest service ranking is returned. In case multiple services are
>> returned, they are ordered descending by their service ranking (i.e. the
>> one with the highest ranking first). | Any object | yes | yes |
>> Parameterized `List` and `Collection` injection points are injected by
>> getting an array of the services and creating an unmodifiable `List` from
>> the array.
>> 
>> Modified:
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>> URL:
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>> 
>> ==============================================================================
>> ---
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>> (original)
>> +++
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/AbstractInjector.java
>> Fri May 13 10:35:03 2016
>> @@ -44,12 +44,30 @@ abstract class AbstractInjector {
>>         return resolver;
>>     }
>> 
>> +    /**
>> +     * Retrieve the ValueMap from the given adaptable. This succeeds, if
>> the adaptable is either
>> +     * <ul>
>> +     * <li>a {@link ValueMap},</li>
>> +     * <li>a {@link SlingHttpServletRequest}, in which case the returned
>> {@link ValueMap} is the one derived from the request's resource or</li>
>> +     * <li>adaptable to a {@link ValueMap}.</li>
>> +     * </ul>
>> +     * Otherwise {@code null} is returned.
>> +     * @param adaptable
>> +     * @return a ValueMap or {@code null}.
>> +     */
>>     protected ValueMap getValueMap(Object adaptable) {
>>         if (adaptable instanceof ValueMap) {
>>             return (ValueMap) adaptable;
>> +        } else if (adaptable instanceof SlingHttpServletRequest) {
>> +            final Resource resource =
>> ((SlingHttpServletRequest)adaptable).getResource();
>> +            // resource may be null for mocked adaptables, therefore do a
>> check here
>> +            if (resource != null) {
>> +                return resource.adaptTo(ValueMap.class);
>> +            } else {
>> +                return null;
>> +            }
>>         } else if (adaptable instanceof Adaptable) {
>> -            ValueMap map = ((Adaptable)
>> adaptable).adaptTo(ValueMap.class);
>> -            return map;
>> +            return ((Adaptable) adaptable).adaptTo(ValueMap.class);
>>         } else {
>>             return null;
>>         }
>> 
>> Modified:
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>> URL:
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>> 
>> ==============================================================================
>> ---
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>> (original)
>> +++
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ResourcePathInjector.java
>> Fri May 13 10:35:03 2016
>> @@ -84,6 +84,7 @@ public class ResourcePathInjector extend
>> 
>>         ResourceResolver resolver = getResourceResolver(adaptable);
>>         if (resolver == null) {
>> +            LOG.debug("Could not get resolver from adaptable {}",
>> adaptable);
>>             return null;
>>         }
>>         List<Resource> resources = getResources(resolver, resourcePaths,
>> name);
>> 
>> Modified:
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>> URL:
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>> 
>> ==============================================================================
>> ---
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>> (original)
>> +++
>> sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
>> Fri May 13 10:35:03 2016
>> @@ -169,12 +169,7 @@ public class ValueMapInjector extends Ab
>>             if (StringUtils.isNotBlank(annotation.via())) {
>>                 return annotation.via();
>>             }
>> -            // automatically go via resource, if this is the httprequest
>> -            if (adaptable instanceof SlingHttpServletRequest) {
>> -                return "resource";
>> -            } else {
>> -                return null;
>> -            }
>> +            return null;
>>         }
>> 
>>         @Override
>> 
>> Modified:
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>> URL:
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>> 
>> ==============================================================================
>> ---
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>> (original)
>> +++
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/impl/ResourcePathInjectionTest.java
>> Fri May 13 10:35:03 2016
>> @@ -57,10 +57,10 @@ public class ResourcePathInjectionTest {
>>     private ModelAdapterFactory factory;
>> 
>>     @Mock
>> -    private Resource adaptable;
>> +    private Resource adaptableResource;
>> 
>>     @Mock
>> -    SlingHttpServletRequest nonResourceAdaptable;
>> +    SlingHttpServletRequest adaptableRequest;
>> 
>>     @Mock
>>     private Resource byPathResource;
>> @@ -98,13 +98,16 @@ public class ResourcePathInjectionTest {
>>         when(componentCtx.getBundleContext()).thenReturn(bundleContext);
>>         when(componentCtx.getProperties()).thenReturn(new
>> Hashtable<String, Object>());
>> 
>> -
>> when(adaptable.getResourceResolver()).thenReturn(resourceResolver);
>> -        when(adaptable.adaptTo(ValueMap.class)).thenReturn(properties);
>> +
>> when(adaptableResource.getResourceResolver()).thenReturn(resourceResolver);
>> +
>> when(adaptableResource.adaptTo(ValueMap.class)).thenReturn(properties);
>> 
>> 
>> when(resourceResolver.getResource("/some/path")).thenReturn(byPathResource);
>> 
>> when(resourceResolver.getResource("/some/path2")).thenReturn(byPathResource2);
>> 
>> when(resourceResolver.getResource("/some/other/path")).thenReturn(byPropertyValueResource);
>> 
>> when(resourceResolver.getResource("/some/other/path2")).thenReturn(byPropertyValueResource2);
>> +
>> +        when(adaptableRequest.getResource()).thenReturn(byPathResource);
>> +
>> when(adaptableRequest.getResourceResolver()).thenReturn(resourceResolver);
>> 
>>         factory = new ModelAdapterFactory();
>>         factory.activate(componentCtx);
>> @@ -116,8 +119,8 @@ public class ResourcePathInjectionTest {
>>     }
>> 
>>     @Test
>> -    public void testPathInjection() {
>> -        ResourcePathModel model = factory.getAdapter(adaptable,
>> ResourcePathModel.class);
>> +    public void testPathInjectionFromResource() {
>> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
>> ResourcePathModel.class);
>>         assertNotNull(model);
>>         assertEquals(byPathResource, model.getFromPath());
>>         assertEquals(byPropertyValueResource, model.getByDerefProperty());
>> @@ -126,15 +129,20 @@ public class ResourcePathInjectionTest {
>>     }
>> 
>>     @Test
>> -    public void testPathInjectionWithNonResourceAdaptable() {
>> -        ResourcePathModel model =
>> factory.getAdapter(nonResourceAdaptable, ResourcePathModel.class);
>> -        // should be null because mandatory fields could not be injected
>> -        assertNull(model);
>> +    public void testPathInjectionFromRequest() {
>> +        // return the same properties through this request's resource, as
>> through adaptableResource
>> +
>> doReturn(adaptableResource.adaptTo(ValueMap.class)).when(byPathResource).adaptTo(ValueMap.class);
>> +        ResourcePathModel model = factory.getAdapter(adaptableRequest,
>> ResourcePathModel.class);
>> +        assertNotNull(model);
>> +        assertEquals(byPathResource, model.getFromPath());
>> +        assertEquals(byPropertyValueResource, model.getByDerefProperty());
>> +        assertEquals(byPathResource2, model.getFromPath2());
>> +        assertEquals(byPropertyValueResource2,
>> model.getByDerefProperty2());
>>     }
>> 
>>     @Test
>>     public void testOptionalPathInjectionWithNonResourceAdaptable() {
>> -        ResourcePathAllOptionalModel model =
>> factory.getAdapter(nonResourceAdaptable,
>> ResourcePathAllOptionalModel.class);
>> +        ResourcePathAllOptionalModel model =
>> factory.getAdapter(adaptableRequest, ResourcePathAllOptionalModel.class);
>>         // should not be null because resource paths fields are optional
>>         assertNotNull(model);
>>         // but the field itself are null
>> @@ -146,7 +154,7 @@ public class ResourcePathInjectionTest {
>> 
>>     @Test
>>     public void testMultiplePathInjection() {
>> -        ResourcePathModel model = factory.getAdapter(adaptable,
>> ResourcePathModel.class);
>> +        ResourcePathModel model = factory.getAdapter(adaptableResource,
>> ResourcePathModel.class);
>>         assertNotNull(model);
>>         List<Resource> resources=model.getMultipleResources();
>>         assertNotNull(resources);
>> @@ -172,16 +180,16 @@ public class ResourcePathInjectionTest {
>>     public void testPartialInjectionFailure1() {
>> 
>> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>> 
>> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
>> ResourcePathPartialModel.class);
>> +        ResourcePathPartialModel model =
>> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
>>         assertNull(model);
>>     }
>> 
>>     @Test
>> -    public void testPartialInjectionFailure2() {
>> +    public void testPartialInjectionFailure2() {
>> 
>> when(resourceResolver.getResource("/some/other/path")).thenReturn(null);
>> 
>> when(resourceResolver.getResource("/some/other/path2")).thenReturn(null);
>> 
>> -        ResourcePathPartialModel model = factory.getAdapter(adaptable,
>> ResourcePathPartialModel.class);
>> +        ResourcePathPartialModel model =
>> factory.getAdapter(adaptableResource, ResourcePathPartialModel.class);
>>         assertNull(model);
>>     }
>> 
>> 
>> Modified:
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>> URL:
>> http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java?rev=1743648&r1=1743647&r2=1743648&view=diff
>> 
>> ==============================================================================
>> ---
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>> (original)
>> +++
>> sling/trunk/bundles/extensions/models/impl/src/test/java/org/apache/sling/models/testmodels/classes/ResourcePathAllOptionalModel.java
>> Fri May 13 10:35:03 2016
>> @@ -32,7 +32,7 @@ import org.apache.sling.models.annotatio
>> public class ResourcePathAllOptionalModel {
>> 
>>     @Inject
>> -    @Path("/some/path")
>> +    @Path("/some/invalidpath")
>>     @Optional
>>     private Resource fromPath;
>> 
>> @@ -42,11 +42,11 @@ public class ResourcePathAllOptionalMode
>>     private Resource derefProperty;
>> 
>>     @Inject
>> -    @Path(paths={"/some/path", "/some/path2"})
>> +    @Path(paths={"/some/invalidpath", "/some/invalidpath2"})
>>     @Optional
>>     private Resource manyFromPathNonList;
>> 
>> -    @ResourcePath(path = "/some/path2", optional=true)
>> +    @ResourcePath(path = "/some/invalidpath2", optional=true)
>>     private Resource fromPath2;
>> 
>>     @ResourcePath(name = "anotherPropertyContainingAPath", optional=true)
>> 
>> 
>>