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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
