You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/12/17 13:25:18 UTC
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org