anchela commented on code in PR #37:
URL:
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/37#discussion_r1236998781
##########
src/main/java/org/apache/sling/servlets/resolver/internal/helper/LocationCollector.java:
##########
@@ -73,12 +74,13 @@ class LocationCollector {
private final List<String> result = new ArrayList<>();
private LocationCollector(String resourceType, String resourceSuperType,
String baseResourceType,
- ResourceResolver resolver) {
+ ResourceResolver resolver, Map<String,Resource>
cacheMap) {
Review Comment:
i would suggest to consistently add @NotNull/@Nullable annotations to the
parameters. IMHO it improves maintainability of the code... when looking at the
patch it seems clear that 'cacheMap' is never null. but i wasn't so sure about
the other params.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/helper/LocationCollector.java:
##########
@@ -208,4 +209,75 @@ private void collectPaths() {
}
return rst;
}
+
+ /**
+ * Resolve a path to a resource; the cacheMap is used for it.
+ * @param path the path
+ * @return the resource for it or null
+ */
+ private @Nullable Resource resolveResource(@NotNull String path) {
+ if (cacheMap.containsKey(path)) {
+ return cacheMap.get(path);
+ } else {
+ Resource r = resolver.getResource(path);
+ cacheMap.put(path, r);
+ return r;
+ }
+ }
+
+
+
+ // ---- static helpers ---
+
+ static @NotNull List<Resource> getLocations(String resourceType, String
resourceSuperType, String baseResourceType,
+ ResourceResolver resolver) {
+
+ @SuppressWarnings("unchecked")
+ Map<String,Resource> m = (Map<String,Resource>)
resolver.getPropertyMap().get(CACHE_KEY);
+ if (m == null) {
+ m = new HashMap<>();
+ resolver.getPropertyMap().put(CACHE_KEY, m);
Review Comment:
i have a few concerns regarding adding the cache to the property map of the
resource-resolver:
- the property map is public right? so this cache map might leak out, no?
- is there any chance that someone relies on the current values of the
property map? in other words: could adding a value that is purely for internal
processing cause regressions?
- i am not too familiar with the servlet-resolution bundle: but if the
resource-resolver instance is shared, this cache might be updated
concurrently.... so using a regular hashmap is likely causing issues with
concurrent updates as it is being populated upon the 'resolveResource' call.
my gut feeling: i would feel more comfortable if the resolver would
optionally allow for a separate internal caching mechanism instead of adding it
to the regular property map.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/helper/LocationCollector.java:
##########
@@ -208,4 +209,75 @@ private void collectPaths() {
}
return rst;
}
+
+ /**
+ * Resolve a path to a resource; the cacheMap is used for it.
+ * @param path the path
+ * @return the resource for it or null
+ */
+ private @Nullable Resource resolveResource(@NotNull String path) {
+ if (cacheMap.containsKey(path)) {
+ return cacheMap.get(path);
+ } else {
+ Resource r = resolver.getResource(path);
+ cacheMap.put(path, r);
+ return r;
+ }
+ }
+
+
+
+ // ---- static helpers ---
+
+ static @NotNull List<Resource> getLocations(String resourceType, String
resourceSuperType, String baseResourceType,
+ ResourceResolver resolver) {
+
+ @SuppressWarnings("unchecked")
+ Map<String,Resource> m = (Map<String,Resource>)
resolver.getPropertyMap().get(CACHE_KEY);
+ if (m == null) {
+ m = new HashMap<>();
+ resolver.getPropertyMap().put(CACHE_KEY, m);
+ }
+ final Map<String,Resource> cacheMap = m;
Review Comment:
that extra assignment is not really needed, is it?
##########
src/main/java/org/apache/sling/servlets/resolver/internal/helper/LocationCollector.java:
##########
@@ -208,4 +209,75 @@ private void collectPaths() {
}
return rst;
}
+
+ /**
+ * Resolve a path to a resource; the cacheMap is used for it.
+ * @param path the path
+ * @return the resource for it or null
+ */
+ private @Nullable Resource resolveResource(@NotNull String path) {
+ if (cacheMap.containsKey(path)) {
+ return cacheMap.get(path);
+ } else {
+ Resource r = resolver.getResource(path);
+ cacheMap.put(path, r);
+ return r;
+ }
+ }
+
+
+
+ // ---- static helpers ---
+
+ static @NotNull List<Resource> getLocations(String resourceType, String
resourceSuperType, String baseResourceType,
+ ResourceResolver resolver) {
+
+ @SuppressWarnings("unchecked")
+ Map<String,Resource> m = (Map<String,Resource>)
resolver.getPropertyMap().get(CACHE_KEY);
Review Comment:
for the sake of defensive programming you may want to check if the map entry
is really of the expected type before casting it. if it isn't someone might
have manipulated it (e.g. adding a property with that key.....)
##########
src/main/java/org/apache/sling/servlets/resolver/internal/helper/LocationCollector.java:
##########
@@ -208,4 +209,75 @@ private void collectPaths() {
}
return rst;
}
+
+ /**
+ * Resolve a path to a resource; the cacheMap is used for it.
+ * @param path the path
+ * @return the resource for it or null
+ */
+ private @Nullable Resource resolveResource(@NotNull String path) {
+ if (cacheMap.containsKey(path)) {
+ return cacheMap.get(path);
+ } else {
+ Resource r = resolver.getResource(path);
+ cacheMap.put(path, r);
+ return r;
+ }
+ }
+
+
+
+ // ---- static helpers ---
+
+ static @NotNull List<Resource> getLocations(String resourceType, String
resourceSuperType, String baseResourceType,
+ ResourceResolver resolver) {
+
+ @SuppressWarnings("unchecked")
+ Map<String,Resource> m = (Map<String,Resource>)
resolver.getPropertyMap().get(CACHE_KEY);
+ if (m == null) {
+ m = new HashMap<>();
+ resolver.getPropertyMap().put(CACHE_KEY, m);
+ }
+ final Map<String,Resource> cacheMap = m;
+
+ LocationCollector collector = new
LocationCollector(resourceType, resourceSuperType, baseResourceType,
+ resolver, cacheMap);
+ List<Resource> result = new ArrayList<>();
+ collector.getResolvedLocations().forEach(location -> {
+ // get the location resource, use a synthetic resource
if there
+ // is no real location. There may still be children at
this
+ // location
+ final String path;
+ if (location.endsWith("/")) {
+ path = location.substring(0, location.length()
- 1);
+ } else {
+ path = location;
+ }
+ final Resource locationRes = getResource(resolver,
path, cacheMap);
+ result.add(locationRes);
+ });
+ return result;
+ }
+
+ /**
+ * Resolve a path to a resource
+ * @param resolver
+ * @param path
+ * @return a synthetic or "real" resource
+ */
+ protected static @NotNull Resource getResource(final @NotNull
ResourceResolver resolver,
+ @NotNull String path, @NotNull Map<String,Resource>
cacheMap) {
+
+ if (cacheMap.containsKey(path) && cacheMap.get(path) != null) {
+ return cacheMap.get(path);
+ } else {
+ Resource res = resolver.getResource(path);
+ if (res == null) {
+ res = new SyntheticResource(resolver, path,
"$synthetic$");
+ }
+ cacheMap.put(path, res);
Review Comment:
see above regarding my concern about concurrent update.
--
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-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
