Re: [PR] SLING-12279 - Use ResourceResolver#getPropertyMap() for resource/resolver adaptables [sling-org-apache-sling-models-impl]
joerghoh commented on code in PR #50:
URL:
https://github.com/apache/sling-org-apache-sling-models-impl/pull/50#discussion_r1581591547
##
src/test/java/org/apache/sling/models/impl/AdapterCacheHolderTest.java:
##
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sling.models.impl;
+
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import org.apache.sling.api.SlingHttpServletRequest;
+import org.apache.sling.api.wrappers.SlingHttpServletRequestWrapper;
+import org.apache.sling.servlethelpers.MockSlingHttpServletRequest;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.lang.ref.SoftReference;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.WeakHashMap;
+
+@RunWith(MockitoJUnitRunner.class)
+public class AdapterCacheHolderTest {
+
+@Test
+public void testGetMapNonServletRequestAdaptable() {
+Map, SoftReference>> outerMap = spy(new
HashMap<>());
+Object adaptable1 = new Object();
+Object adaptable2 = new Object();
+
+try (AdapterCacheHolder adapterCacheHolder = new
AdapterCacheHolder(false, outerMap)) {
+Map, SoftReference> map1 =
adapterCacheHolder.getCacheMapForAdaptable(adaptable1);
+Map, SoftReference> map2 =
adapterCacheHolder.getCacheMapForAdaptable(adaptable1);
+Map, SoftReference> map3 =
adapterCacheHolder.getCacheMapForAdaptable(adaptable2);
+
+assertSame(map1, map2);
+assertNotSame(map1, map3);
+}
+}
+
+@Test
+public void testGetMapServletRequestAdaptable() {
+Map, SoftReference>> outerMap = spy(new
HashMap<>());
+SlingHttpServletRequest adaptable1 = new
MockSlingHttpServletRequest(null);
+SlingHttpServletRequest adaptable2 = new
SlingHttpServletRequestWrapper(adaptable1);
+SlingHttpServletRequest adaptable3 = new
MockSlingHttpServletRequest(null);
+
+try (AdapterCacheHolder adapterCacheHolder = new
AdapterCacheHolder(false, outerMap)) {
+Map, SoftReference> map1 =
adapterCacheHolder.getCacheMapForAdaptable(adaptable1);
+Map, SoftReference> map2 =
adapterCacheHolder.getCacheMapForAdaptable(adaptable2);
+Map, SoftReference> map3 =
adapterCacheHolder.getCacheMapForAdaptable(adaptable3);
+
+assertSame(map1, map2);
+assertSame(map1, map3);
+}
+}
+
+@Test
+public void testClose() {
+Object adaptable = new Object();
+Object result = new Object();
+SoftReference ref = new SoftReference<>(result);
+
+Map, SoftReference> innerMap = spy(new HashMap<>());
+innerMap.put(Object.class, ref);
+
+Map, SoftReference>> outerMap = spy(new
HashMap<>());
+outerMap.put(adaptable, innerMap);
+
+try (AdapterCacheHolder adapterCacheHolder = new
AdapterCacheHolder(false, outerMap)) {
+// intentionally empty
+}
+
+verify(outerMap).values();
+verify(outerMap).clear();
+verify(innerMap).clear();
+}
+
+@Test
+public void testNewMapSyncMap() {
+// This is a very weak test, but it should be sufficient to at least
confirm that we are not just using a
+// WeakHashMap, in case someone changes it without knowing why.
+Map nonSyncMap= AdapterCacheHolder.newMap(false);
+Map syncMap= AdapterCacheHolder.newMap(true);
+
+System.out.println(nonSyncMap.getClass());
Review Comment:
is this required here?
--
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
Re: [VOTE] Release Apache Sling Commons JSON 2.0.28
+1 Am Do., 25. Apr. 2024 um 04:21 Uhr schrieb Robert Munteanu < romb...@apache.org>: > Hi, > > We solved 1 issue in this release: > > https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12310710=12354549=Text > > Staging repository: > https://repository.apache.org/content/repositories/orgapachesling-2856/ > > You can use this UNIX script to download the release and verify the > signatures: > > https://raw.githubusercontent.com/apache/sling-tooling-release/master/check_staged_release.sh > > Usage: > sh check_staged_release.sh 2856 /tmp/sling-staging > > Please vote to approve this release: > > [ ] +1 Approve the release > [ ] 0 Don't care > [ ] -1 Don't release, because ... > > This majority vote is open for at least 72 hours. > > Regards, > Robert Munteanu > -- https://cqdump.joerghoh.de
[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map
[
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841341#comment-17841341
]
Paul Bjorkstrand commented on SLING-12279:
--
[~joerghoh], can you take a look at this and see if it solves your problem? I
believe it will, but I don't have a way to reliably test. I could try to
contrive some kind of unit test that does some verification, though it may
prove difficult to make. Since the GC is largely non-deterministic (from an
outsider's perspective), getting this into a field test would likely be better.
> Move Sling Model cache holder for Resource and ResourceResolver adaptables
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>Reporter: Paul Bjorkstrand
>Priority: Major
>
> There have been several instances of issues with model caching over the
> years, the most recent being SLING-12259 and [this
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from
> Jörg Hoh. These recent issues have been around cached items sticking
> around for too long. In that thread, it was discussed using
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
> whenever possible. This binds the cache to the lifecycle of the
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
> there is already a divergence that binds the cache for request-based models
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and
> use the resolver's property map to bind the cache for those adaptables to the
> lifecycle of the ResourceResolver. Resources are already (largely) bound to
> the lifecycle of the ResourceResolver that supplied them, and binding the
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the
> likelihood of the JVM's GC being put under pressure in cases when cached
> models [reference the original adaptable using
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
> As a bonus, if the cache object implements {{Closeable}}, the Sling
> Implementation will call {{close()}} on the cache when the resolver is
> closed,, giving us further control over the lifetime of objects in the cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12197) cpconverter: Sling-Initial-Content directories created as nt:folder instead of sling:Folder
[
https://issues.apache.org/jira/browse/SLING-12197?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841292#comment-17841292
]
Niek Raaijmakers commented on SLING-12197:
--
In particular: the invocation resides here:
[https://github.com/adobe/aemanalyser-maven-plugin/blob/main/aemanalyser-core/src/main/java/com/adobe/aem/analyser/AemPackageConverter.java#L135]
> cpconverter: Sling-Initial-Content directories created as nt:folder instead
> of sling:Folder
> ---
>
> Key: SLING-12197
> URL: https://issues.apache.org/jira/browse/SLING-12197
> Project: Sling
> Issue Type: Bug
> Components: Content-Package to Feature Model Converter
>Affects Versions: Content-Package to Feature Model Converter 1.3.4
>Reporter: Stefan Seifert
>Priority: Major
> Fix For: Content-Package to Feature Model Converter 1.3.8
>
> Attachments: io.wcm.handler.link-apps-1.10.2-cp2fm-converted.zip
>
>
> the cpconverter extracts Sling-Initial-Content from OSGi bundles and creates
> FileVault packages with the transformed content.
> this works well, but there is one difference when the resulting content
> package is installed compared when uploading the OSGi bundle with the
> Sling-Initial-Content directly:
> * the JCR Content Loader by defaults creates a {{sling:Folder}} node type for
> each directory found in the Sling-Initial-Content (see also
> [docs|https://sling.apache.org/documentation/bundles/content-loading-jcr-contentloader.html#initial-content-loading-1])
> * the cpconverter process creates no {{.content.xml}} file for the folders,
> but only for the actual JSON files found in the process. as a result, the
> folders are created as {{nt:folder}} when uploading the transformed package
> * this difference becomes relevant, when a JSON file in Sling-Initial-Content
> defines a primary type of {{nt:unstructured}} - it is not allowed to created
> such a node directly below a {{nt:folder}} node - but it is allowed to do so
> below a {{sling:Folder}} node
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Comment Edited] (SLING-12197) cpconverter: Sling-Initial-Content directories created as nt:folder instead of sling:Folder
[
https://issues.apache.org/jira/browse/SLING-12197?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841291#comment-17841291
]
Niek Raaijmakers edited comment on SLING-12197 at 4/26/24 4:19 PM:
---
[~sseifert] we identified the issue. the create path repoinit statements were
not created due to a different invocation of the CPConverter then before.
Strangely enough, the packages were extracted from the bundle. In any case it's
upstream.
was (Author: sc0rpic0m):
[~sseifert] we identified the issue. the create path repoinit statements were
not created due to a different invocation of the CPConverter then before.
Strangly enough, the packages were extracted from the bundle. In any case it's
upstream.
> cpconverter: Sling-Initial-Content directories created as nt:folder instead
> of sling:Folder
> ---
>
> Key: SLING-12197
> URL: https://issues.apache.org/jira/browse/SLING-12197
> Project: Sling
> Issue Type: Bug
> Components: Content-Package to Feature Model Converter
>Affects Versions: Content-Package to Feature Model Converter 1.3.4
>Reporter: Stefan Seifert
>Priority: Major
> Fix For: Content-Package to Feature Model Converter 1.3.8
>
> Attachments: io.wcm.handler.link-apps-1.10.2-cp2fm-converted.zip
>
>
> the cpconverter extracts Sling-Initial-Content from OSGi bundles and creates
> FileVault packages with the transformed content.
> this works well, but there is one difference when the resulting content
> package is installed compared when uploading the OSGi bundle with the
> Sling-Initial-Content directly:
> * the JCR Content Loader by defaults creates a {{sling:Folder}} node type for
> each directory found in the Sling-Initial-Content (see also
> [docs|https://sling.apache.org/documentation/bundles/content-loading-jcr-contentloader.html#initial-content-loading-1])
> * the cpconverter process creates no {{.content.xml}} file for the folders,
> but only for the actual JSON files found in the process. as a result, the
> folders are created as {{nt:folder}} when uploading the transformed package
> * this difference becomes relevant, when a JSON file in Sling-Initial-Content
> defines a primary type of {{nt:unstructured}} - it is not allowed to created
> such a node directly below a {{nt:folder}} node - but it is allowed to do so
> below a {{sling:Folder}} node
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12197) cpconverter: Sling-Initial-Content directories created as nt:folder instead of sling:Folder
[
https://issues.apache.org/jira/browse/SLING-12197?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841291#comment-17841291
]
Niek Raaijmakers commented on SLING-12197:
--
[~sseifert] we identified the issue. the create path repoinit statements were
not created due to a different invocation of the CPConverter then before.
Strangly enough, the packages were extracted from the bundle. In any case it's
upstream.
> cpconverter: Sling-Initial-Content directories created as nt:folder instead
> of sling:Folder
> ---
>
> Key: SLING-12197
> URL: https://issues.apache.org/jira/browse/SLING-12197
> Project: Sling
> Issue Type: Bug
> Components: Content-Package to Feature Model Converter
>Affects Versions: Content-Package to Feature Model Converter 1.3.4
>Reporter: Stefan Seifert
>Priority: Major
> Fix For: Content-Package to Feature Model Converter 1.3.8
>
> Attachments: io.wcm.handler.link-apps-1.10.2-cp2fm-converted.zip
>
>
> the cpconverter extracts Sling-Initial-Content from OSGi bundles and creates
> FileVault packages with the transformed content.
> this works well, but there is one difference when the resulting content
> package is installed compared when uploading the OSGi bundle with the
> Sling-Initial-Content directly:
> * the JCR Content Loader by defaults creates a {{sling:Folder}} node type for
> each directory found in the Sling-Initial-Content (see also
> [docs|https://sling.apache.org/documentation/bundles/content-loading-jcr-contentloader.html#initial-content-loading-1])
> * the cpconverter process creates no {{.content.xml}} file for the folders,
> but only for the actual JSON files found in the process. as a result, the
> folders are created as {{nt:folder}} when uploading the transformed package
> * this difference becomes relevant, when a JSON file in Sling-Initial-Content
> defines a primary type of {{nt:unstructured}} - it is not allowed to created
> such a node directly below a {{nt:folder}} node - but it is allowed to do so
> below a {{sling:Folder}} node
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12197) cpconverter: Sling-Initial-Content directories created as nt:folder instead of sling:Folder
[
https://issues.apache.org/jira/browse/SLING-12197?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841068#comment-17841068
]
Stefan Seifert commented on SLING-12197:
bq. At least for mutable content, if i'm not wrong we use the order of the
generated packages.csv from the cpconverter to deploy later.
yes, but in this case its about the ordering of (sticking to the unit test
example):
1. repoinit statements generated from {{io.wcm.handler.media-1.11.6.jar}}
2. content package {{io.wcm.handler.media-apps-1.11.6-cp2fm-converted.zip}}
with *immutable* content generated from {{io.wcm.handler.media-1.11.6.jar}},
containing nodes and scripts below {{/apps}}
they have to be executed in exactly this order - if the folders already exists
after installing the package the repoinit statements have no effect.
> cpconverter: Sling-Initial-Content directories created as nt:folder instead
> of sling:Folder
> ---
>
> Key: SLING-12197
> URL: https://issues.apache.org/jira/browse/SLING-12197
> Project: Sling
> Issue Type: Bug
> Components: Content-Package to Feature Model Converter
>Affects Versions: Content-Package to Feature Model Converter 1.3.4
>Reporter: Stefan Seifert
>Priority: Major
> Fix For: Content-Package to Feature Model Converter 1.3.8
>
> Attachments: io.wcm.handler.link-apps-1.10.2-cp2fm-converted.zip
>
>
> the cpconverter extracts Sling-Initial-Content from OSGi bundles and creates
> FileVault packages with the transformed content.
> this works well, but there is one difference when the resulting content
> package is installed compared when uploading the OSGi bundle with the
> Sling-Initial-Content directly:
> * the JCR Content Loader by defaults creates a {{sling:Folder}} node type for
> each directory found in the Sling-Initial-Content (see also
> [docs|https://sling.apache.org/documentation/bundles/content-loading-jcr-contentloader.html#initial-content-loading-1])
> * the cpconverter process creates no {{.content.xml}} file for the folders,
> but only for the actual JSON files found in the process. as a result, the
> folders are created as {{nt:folder}} when uploading the transformed package
> * this difference becomes relevant, when a JSON file in Sling-Initial-Content
> defines a primary type of {{nt:unstructured}} - it is not allowed to created
> such a node directly below a {{nt:folder}} node - but it is allowed to do so
> below a {{sling:Folder}} node
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Comment Edited] (SLING-12197) cpconverter: Sling-Initial-Content directories created as nt:folder instead of sling:Folder
[
https://issues.apache.org/jira/browse/SLING-12197?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841068#comment-17841068
]
Stefan Seifert edited comment on SLING-12197 at 4/26/24 7:02 AM:
-
bq. At least for mutable content, if i'm not wrong we use the order of the
generated packages.csv from the cpconverter to deploy later.
yes, but in this case its about the ordering of (sticking to the unit test
example):
# repoinit statements generated from {{io.wcm.handler.media-1.11.6.jar}}
# content package {{io.wcm.handler.media-apps-1.11.6-cp2fm-converted.zip}} with
*immutable* content generated from {{io.wcm.handler.media-1.11.6.jar}},
containing nodes and scripts below {{/apps}}
they have to be executed in exactly this order - if the folders already exists
after installing the package the repoinit statements have no effect.
was (Author: sseif...@pro-vision.de):
bq. At least for mutable content, if i'm not wrong we use the order of the
generated packages.csv from the cpconverter to deploy later.
yes, but in this case its about the ordering of (sticking to the unit test
example):
1. repoinit statements generated from {{io.wcm.handler.media-1.11.6.jar}}
2. content package {{io.wcm.handler.media-apps-1.11.6-cp2fm-converted.zip}}
with *immutable* content generated from {{io.wcm.handler.media-1.11.6.jar}},
containing nodes and scripts below {{/apps}}
they have to be executed in exactly this order - if the folders already exists
after installing the package the repoinit statements have no effect.
> cpconverter: Sling-Initial-Content directories created as nt:folder instead
> of sling:Folder
> ---
>
> Key: SLING-12197
> URL: https://issues.apache.org/jira/browse/SLING-12197
> Project: Sling
> Issue Type: Bug
> Components: Content-Package to Feature Model Converter
>Affects Versions: Content-Package to Feature Model Converter 1.3.4
>Reporter: Stefan Seifert
>Priority: Major
> Fix For: Content-Package to Feature Model Converter 1.3.8
>
> Attachments: io.wcm.handler.link-apps-1.10.2-cp2fm-converted.zip
>
>
> the cpconverter extracts Sling-Initial-Content from OSGi bundles and creates
> FileVault packages with the transformed content.
> this works well, but there is one difference when the resulting content
> package is installed compared when uploading the OSGi bundle with the
> Sling-Initial-Content directly:
> * the JCR Content Loader by defaults creates a {{sling:Folder}} node type for
> each directory found in the Sling-Initial-Content (see also
> [docs|https://sling.apache.org/documentation/bundles/content-loading-jcr-contentloader.html#initial-content-loading-1])
> * the cpconverter process creates no {{.content.xml}} file for the folders,
> but only for the actual JSON files found in the process. as a result, the
> folders are created as {{nt:folder}} when uploading the transformed package
> * this difference becomes relevant, when a JSON file in Sling-Initial-Content
> defines a primary type of {{nt:unstructured}} - it is not allowed to created
> such a node directly below a {{nt:folder}} node - but it is allowed to do so
> below a {{sling:Folder}} node
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
