rombert commented on a change in pull request #50:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r771388759
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
##########
@@ -355,6 +358,31 @@ public void mapNestedAliasTarget() {
.allMappingsWithRequest("/app/alias-parent/alias-child",
"/app/alias-parent/child", "/app/parent/alias-child")
.verify(resolver, req);
}
+
+ /**
+ * Validates that mappings for an empty path
+ *
+ * @throws LoginException
+ */
+ @Test
+ public void mapEmptyPathWithNonExistingResource() {
Review comment:
```suggestion
public void mapEmptyPathWithUnreadableRoot() {
````
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java
##########
@@ -72,7 +72,7 @@ public String getMapping(String resourcePath,
HttpServletRequest request) {
Collection<String> mappings = getAllMappings(resourcePath, request);
if ( mappings.isEmpty() )
- return null;
+ return "";
Review comment:
I would prefer that we handle this differently, in the `getAllMappings`
method. The javadoc of that mapping promises that no empty results will be
returned.
https://github.com/apache/sling-org-apache-sling-api/blob/org.apache.sling.api-2.24.0/src/main/java/org/apache/sling/api/resource/mapping/ResourceMapper.java#L109
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
##########
@@ -355,6 +358,31 @@ public void mapNestedAliasTarget() {
.allMappingsWithRequest("/app/alias-parent/alias-child",
"/app/alias-parent/child", "/app/parent/alias-child")
.verify(resolver, req);
}
+
+ /**
+ * Validates that mappings for an empty path
+ *
+ * @throws LoginException
+ */
+ @Test
+ public void mapEmptyPathWithNonExistingResource() {
+ MapEntriesHandler mapEntriesHandler = mock(MapEntriesHandler.class);
+
when(mapEntriesHandler.getVanityPathMappings()).thenReturn(Collections.emptyMap());
+
+ ResourceResolverImpl resolver = mock(ResourceResolverImpl.class);
+ when(resolver.resolveInternal(any(), any())).thenReturn(null);
+
when(resolver.getResource("")).thenReturn(this.resolver.getResource(""));
+ when(resolver.adaptTo(ResourceMapper.class)).thenReturn(new
ResourceMapperImpl(resolver, mock(ResourceDecoratorTracker.class),
+ mapEntriesHandler, mock(Object.class)));
+
+ ExpectedMappings.nonExistingResource("")
+ .singleMapping("")
+ .singleMappingWithRequest("")
+ .allMappings("")
+ .allMappingsWithRequest("")
+ .testingEmptyPathWithNonExistingResource(true)
Review comment:
Why is this needed?
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
##########
@@ -355,6 +358,31 @@ public void mapNestedAliasTarget() {
.allMappingsWithRequest("/app/alias-parent/alias-child",
"/app/alias-parent/child", "/app/parent/alias-child")
.verify(resolver, req);
}
+
+ /**
+ * Validates that mappings for an empty path
+ *
+ * @throws LoginException
+ */
+ @Test
+ public void mapEmptyPathWithNonExistingResource() {
+ MapEntriesHandler mapEntriesHandler = mock(MapEntriesHandler.class);
+
when(mapEntriesHandler.getVanityPathMappings()).thenReturn(Collections.emptyMap());
+
+ ResourceResolverImpl resolver = mock(ResourceResolverImpl.class);
+ when(resolver.resolveInternal(any(), any())).thenReturn(null);
Review comment:
Can we make this something return `null` only for the root path and
delegate to the real resolver? I think that's a more likely scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
