You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "anchela (via GitHub)" <gi...@apache.org> on 2023/06/21 13:42:43 UTC
[GitHub] [sling-org-apache-sling-servlets-resolver] anchela commented on a diff in pull request #37: SLING-11558 cache already resolved resources
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org