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/10/17 00:30:05 UTC
[GitHub] [sling-org-apache-sling-resourceresolver] Gabiesfat opened a new pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Gabiesfat opened a new pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50
This PR has the code changes for issue SLING-10844. Apparently earlier implementation of API getMapping() in ResourceMapperImpl.java was returning null in case the path was empty which was breaking the contract mentioned in corresponding interface so to fix code changes been made to return empty string (but not null).
The test [0] would be fixed in further commit of this PR.
[0]: https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/master/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java#L173
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] sonarcloud[bot] commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-951615049
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
rombert commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-1046762441
@humbledev123 - thanks for your comments. I still think we need a clear test case or a reliable way of reproducing the problem in the Sling Starter. Then we can see where the fix should land.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] Gabiesfat commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
Gabiesfat commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-969837795
> Thanks for the PR @Gabiesfat and apologies for looking so late into this. At this point I am not sure what we are trying to fix. I think it would be best if you could demonstrate it with a unit test that fails without the change and passed with it.
>
> I tried to do so by adding a new test case to `ResourceMapperImplTest.java`:
>
> ```java
> @Test
> public void mapEmptyPath() {
> ExpectedMappings.nonExistingResource("")
> .singleMapping("/")
> .singleMappingWithRequest("/app/")
> .allMappings("/")
> .allMappingsWithRequest("/app/")
> .verify(resolver, req);
> }
> ```
>
> The test passes currently so I am probably misunderstanding what this PR is trying to fix.
I tried to make sure that method _getMapping_ returns empty mapping instead of null incase of _resourcePath_ provided as empty string. I have limited understanding of unit tests here as this is the first time I am seeing this code, if you could help me in writing a test case where empty string returns after providing empty _resourcePath_ then it would be great.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
rombert commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-963006503
Thanks for the PR @Gabiesfat and apologies for looking so late into this. At this point I am not sure what we are trying to fix. I think it would be best if you could demonstrate it with a unit test that fails without the change and passed with it.
I tried to do so by adding a new test case to `ResourceMapperImplTest.java`:
```java
@Test
public void mapEmptyPath() {
ExpectedMappings.nonExistingResource("")
.singleMapping("/")
.singleMappingWithRequest("/app/")
.allMappings("/")
.allMappingsWithRequest("/app/")
.verify(resolver, req);
}
```
The test passes currently so I am probably misunderstanding what this PR is trying to fix.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r811021901
##########
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:
@humbledev123 where is this done? I don't see any new commits added to this PR.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] Gabiesfat commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
Gabiesfat commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-996360417
> @Gabiesfat - that was my point, this test case shows that for an empty path you don't get a null result. It's possible that the unit tests are incorrect somehow. Maybe it's best that you debug your scenario with the empty path, see what code paths are triggered, and reproduce them with a test case.
>
> If you have problems with the test case I can definitely help, but we need to understand the failure scenario first.
Apparently the issue was at [0] the resolver could also resolve to null resource and the same scenario has not been replicated in the test you mentioned. I have added a test which replicates the same scenario and the same fails without my fix. Please have a look at it let me know your thoughts.
[0]: https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/master/src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java#L145
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] sonarcloud[bot] removed a comment on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-1049874632
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r816690749
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
##########
@@ -100,7 +104,10 @@ public void prepare() throws LoginException {
ctx.registerInjectActivateService(new StringInterpolationProviderImpl());
InMemoryResourceProvider resourceProvider = new InMemoryResourceProvider();
- resourceProvider.putResource("/"); // root
+
+ if(!testName.getMethodName().contains("mapEmptyPathWithUnreadableRoot")) {
Review comment:
I am still interested in finding out how the unreadable root can take happen. Please describe the scenario in detail - either here, or in the Jira issue so that we can reproduce it in the Sling Starter. A content package that works with the Sling Starter would be ideal, but I can also work with "textual" descriptions.
If this becomes too complicated for this unit testing framework we can always add an integration test in https://github.com/apache/sling-org-apache-sling-launchpad-integration-tests .
But first, I want to understand the problem.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] mohiaror commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
mohiaror commented on a change in pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r735099168
##########
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:
If we are _always_ adding a path in `mappings`, can there be a case when `mappings` collection is `empty`?
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java
##########
@@ -154,7 +154,7 @@ public String getMapping(String resourcePath, HttpServletRequest request) {
}
// 5. add the requested path itself, if not already populated
- if ( !mappedPath.isEmpty() && !mappings.contains(mappedPath))
+ if (!mappings.contains(mappedPath))
Review comment:
I think the order in which is the entries are added in `mappings` array is important since `getMappings` API simply returns the first entry of the collection. After this change an empty path will be added before the `nonDecoratedResource` path. Can you please check if that is acceptable behavior when compared with how it was before these changes were added?
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [sling-org-apache-sling-resourceresolver] humbledev123 commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
humbledev123 commented on a change in pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r809710289
##########
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:
Done
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] sonarcloud[bot] commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-1049879785
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] humbledev123 commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
humbledev123 commented on a change in pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r809710061
##########
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:
Finding an appropriate mappings relies on having non-empty mappedPath or non-null nonDecoratedResource but in case if mappedPath is empty and nonDecoratedResource is null (it could be possible based on doc [0]) then there could be no possible mapping and the result would be empty. Besides I cold not think of any possible mapping which ought to be returned in such case, any thoughts ?
[0]: https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/deb25ba47cf563e5bc52978eef36e80d735a067c/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java#L718
##########
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:
This is for the case where resource is non-existing with empty path.
##########
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:
Apparently root path is not empty this was done to replicate the scenario which was observed in bug where path is empty and then resolveInternal ought to return null.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] Gabiesfat commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
Gabiesfat commented on a change in pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r813897637
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
##########
@@ -100,7 +104,10 @@ public void prepare() throws LoginException {
ctx.registerInjectActivateService(new StringInterpolationProviderImpl());
InMemoryResourceProvider resourceProvider = new InMemoryResourceProvider();
- resourceProvider.putResource("/"); // root
+
+ if(!testName.getMethodName().contains("mapEmptyPathWithUnreadableRoot")) {
Review comment:
This was done to recreate the scenario observed in the bug where no resource as "/" added.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] Gabiesfat commented on a change in pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
Gabiesfat commented on a change in pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#discussion_r736197884
##########
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:
This method depends on getAllMappings() for the mappings which only states that it returns a collection
of mappings. So theoretically _yes_ but practically _no_. Ideally this method should only be worried about the contract getAllMappings shares (and intelligent enough to handle scenario based on that) rather than depending upon implementation beneath. However having said that after addressing the other comment mappings can be empty even practically too.
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java
##########
@@ -154,7 +154,7 @@ public String getMapping(String resourcePath, HttpServletRequest request) {
}
// 5. add the requested path itself, if not already populated
- if ( !mappedPath.isEmpty() && !mappings.contains(mappedPath))
+ if (!mappings.contains(mappedPath))
Review comment:
Actually it was preventing the mappedPath from getting added to mappings which was leading to making it null however that case has been handled above and mappings can be empty so I believe the condition can be imposed again.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
rombert commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-970151016
@Gabiesfat - that was my point, this test case shows that for an empty path you don't get a null result. It's possible that the unit tests are incorrect somehow. Maybe it's best that you debug your scenario with the empty path, see what code paths are triggered, and reproduce them with a test case.
If you have problems with the test case I can definitely help, but we need to understand the failure scenario first.
--
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
[GitHub] [sling-org-apache-sling-resourceresolver] sonarcloud[bot] commented on pull request #50: SLING-10844: ResourceMapper.getMapping() returns null for empty path
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #50:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/50#issuecomment-1049874632
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=50&metric=new_duplicated_lines_density&view=list)
--
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