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