-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 <[email protected]> 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) > > >
