SLING-9745 Sling Uri Mapping SPI #25
Conversation
| * @return the intermediate mappings | ||
| */ | ||
| @NotNull | ||
| Map<String, SlingUri> getIntermediateMappings(); |
There was a problem hiding this comment.
Why is this a map and not a list? What is the key for the values?
I assume this map is immutable?
There was a problem hiding this comment.
So currently the key is the classname of the SlingUriMapper that produced the result, it would probably be better to use type Class<? extends SlingUriMapper> as key, that would make it automatically clearer. Most clients will only need the values (and they can get it with values(), but sometimes it might be useful to check if a certain mapper produced a mapping result, and that can be done by getIntermediateMappings().containsKey() with the current API.
The map is immutable as the constructor call
https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/4ab43acf49dfd9cd25119b74054980b3c4144918/src/main/java/org/apache/sling/resourceresolver/impl/urimapping/PathToUriMappingServiceImpl.java#L108
uses Collections.unmodifiableMap(intermediateMappings)
https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/4ab43acf49dfd9cd25119b74054980b3c4144918/src/main/java/org/apache/sling/resourceresolver/impl/urimapping/MappingChainContextInternal.java#L54
There was a problem hiding this comment.
I don't think that a class name is a good key - in theory there could be a factory component for SlingUriMapper with different configurations - sharing the same class name.
As this is ordered, it probably should be a list. So how about a list of some type of object, which has two getter methods, one to get the SlingUri, the other to get some info about the SlingUriMapper. We could add a default method to SlingUriMapper like getName() which by default returns the class name. But this way, the contract is at least more clear.
There was a problem hiding this comment.
Updated the getIntermediateMappings() method in 43f5804 (for now left out SlingUriMapper.getName(), I think this can be autogenerated and if needed, be overwritten by a OSGI service property like we do it for HCs, that's maybe cleaner than forcing everybody to implement getName())
| * @return the URI mappings | ||
| */ | ||
| @NotNull | ||
| Map<String, SlingUri> getIntermediateMappings(); |
There was a problem hiding this comment.
same as with Result? Why a map, immutable?
There was a problem hiding this comment.
Shouldn't this be changed to List ?
ghenzler
force-pushed
the
feature/SLING-9662-Introduce-SlingUri-Mapping-SPI-v3
branch
from
f149adc to
855191c
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
An aspect that was missing is that sometimes, independent of if you have a request at hand or not, you need to force an absolute URI to be generated. E.g. while producing an html rendering with HTL, most links may remain relative to host (starting with |
API PR (can be merged separately):
#25
Impl PRs that belong together:
apache/sling-org-apache-sling-resourceresolver#22
apache/sling-org-apache-sling-engine#10
apache/sling-org-apache-sling-auth-core#6