[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on a change in pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests

[GitBox <gi...@apache.org>]

cjelger commented on a change in pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#discussion_r507677579



##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -404,11 +430,7 @@ public boolean isModelClass(@NotNull Class<?> type) {
                         ModelType model = (ModelType) Proxy.newProxyInstance(modelClass.getType().getClassLoader(), new Class<?>[] { modelClass.getType() }, handlerResult.getValue());
 
                         if (modelAnnotation.cache()) {
-                            Map<Class<?>, SoftReference<Object>> adaptableCache = adapterCache.get(cacheKey);
-                            if (adaptableCache == null) {
-                                adaptableCache = Collections.synchronizedMap(new WeakHashMap<Class<?>, SoftReference<Object>>());
-                                adapterCache.put(cacheKey, adaptableCache);
-                            }
+                            Map<Class<?>, SoftReference<Object>> adaptableCache = getOrCreateCache(adaptable);

Review comment:
       No there could be a cache with the same `adaptable` but a different "target" model class. For example, one could first `adapt` the request to `MyModel.class` and then to `MyOtherModel.class`. So in the first case, the cache doesn't exist and the model is of course not cached, but in the 2nd case, the cache exists but the model is also not cached.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org