You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by ss...@apache.org on 2017/09/29 17:01:44 UTC
svn commit: r1810138 - in /sling/trunk/bundles/extensions/caconfig/impl/src:
main/java/org/apache/sling/caconfig/impl/def/
test/java/org/apache/sling/caconfig/impl/def/
Author: sseifert
Date: Fri Sep 29 17:01:44 2017
New Revision: 1810138
URL: http://svn.apache.org/viewvc?rev=1810138&view=rev
Log:
SLING-7165 CAConfig Impl: Potential data loss when saving multiple nested configuration lists
Modified:
sling/trunk/bundles/extensions/caconfig/impl/src/main/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategy.java
sling/trunk/bundles/extensions/caconfig/impl/src/test/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategyTest.java
Modified: sling/trunk/bundles/extensions/caconfig/impl/src/main/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategy.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/caconfig/impl/src/main/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategy.java?rev=1810138&r1=1810137&r2=1810138&view=diff
==============================================================================
--- sling/trunk/bundles/extensions/caconfig/impl/src/main/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategy.java (original)
+++ sling/trunk/bundles/extensions/caconfig/impl/src/main/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategy.java Fri Sep 29 17:01:44 2017
@@ -169,7 +169,7 @@ public class DefaultConfigurationPersist
Resource configResourceParent = getOrCreateResource(resourceResolver, configResourceCollectionParentPath, data.getProperties());
// delete existing children and create new ones
- deleteChildren(configResourceParent);
+ deleteChildrenNotInCollection(configResourceParent, data);
for (ConfigurationPersistData item : data.getItems()) {
String path = configResourceParent.getPath() + "/" + item.getCollectionItemName();
getOrCreateResource(resourceResolver, path, item.getProperties());
@@ -211,12 +211,25 @@ public class DefaultConfigurationPersist
}
}
- private void deleteChildren(Resource resource) {
+ /**
+ * Delete children that are no longer contained in list of collection items.
+ * @param resource Parent resource
+ * @param data List of collection items
+ */
+ private void deleteChildrenNotInCollection(Resource resource, ConfigurationCollectionPersistData data) {
ResourceResolver resourceResolver = resource.getResourceResolver();
+
+ Set<String> collectionItemNames = new HashSet<>();
+ for (ConfigurationPersistData item : data.getItems()) {
+ collectionItemNames.add(item.getCollectionItemName());
+ }
+
try {
for (Resource child : resource.getChildren()) {
- log.trace("! Delete resource {}", child.getPath());
- resourceResolver.delete(child);
+ if (!collectionItemNames.contains(child.getName())) {
+ log.trace("! Delete resource {}", child.getPath());
+ resourceResolver.delete(child);
+ }
}
}
catch (PersistenceException ex) {
Modified: sling/trunk/bundles/extensions/caconfig/impl/src/test/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategyTest.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/caconfig/impl/src/test/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategyTest.java?rev=1810138&r1=1810137&r2=1810138&view=diff
==============================================================================
--- sling/trunk/bundles/extensions/caconfig/impl/src/test/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategyTest.java (original)
+++ sling/trunk/bundles/extensions/caconfig/impl/src/test/java/org/apache/sling/caconfig/impl/def/DefaultConfigurationPersistenceStrategyTest.java Fri Sep 29 17:01:44 2017
@@ -32,6 +32,7 @@ import org.apache.sling.caconfig.spi.Con
import org.apache.sling.caconfig.spi.ConfigurationPersistData;
import org.apache.sling.caconfig.spi.ConfigurationPersistenceStrategy2;
import org.apache.sling.hamcrest.ResourceMatchers;
+import org.apache.sling.testing.mock.sling.ResourceResolverType;
import org.apache.sling.testing.mock.sling.junit.SlingContext;
import org.junit.Before;
import org.junit.Rule;
@@ -43,7 +44,7 @@ import com.google.common.collect.Immutab
public class DefaultConfigurationPersistenceStrategyTest {
@Rule
- public SlingContext context = new SlingContext();
+ public SlingContext context = new SlingContext(ResourceResolverType.JCR_MOCK);
@Before
public void setUp() {
@@ -113,22 +114,97 @@ public class DefaultConfigurationPersist
// store new config collection items
assertTrue(underTest.persistConfigurationCollection(context.resourceResolver(), "/conf/test",
new ConfigurationCollectionPersistData(ImmutableList.of(
- new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value1")).collectionItemName("0"),
- new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop2", 5)).collectionItemName("1"))
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value1")).collectionItemName("item1"),
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop2", 5)).collectionItemName("item2"))
).properties(ImmutableMap.<String, Object>of("prop1", "abc", "sling:resourceType", "/a/b/c"))
));
context.resourceResolver().commit();
Resource resource = context.resourceResolver().getResource("/conf/test");
- assertEquals(2, ImmutableList.copyOf(resource.getChildren()).size());
- ValueMap props0 = context.resourceResolver().getResource("/conf/test/0").getValueMap();
- assertEquals("value1", props0.get("prop1", String.class));
- ValueMap props1 = context.resourceResolver().getResource("/conf/test/1").getValueMap();
- assertEquals((Integer)5, props1.get("prop2", Integer.class));
-
- assertThat(resource, ResourceMatchers.props(
- "prop1", "abc",
- "sling:resourceType", "/a/b/c"));
+ assertThat(resource, ResourceMatchers.props("prop1", "abc", "sling:resourceType", "/a/b/c"));
+ assertThat(resource, ResourceMatchers.containsChildren("item1", "item2"));
+
+ assertThat(resource.getChild("item1"), ResourceMatchers.props("prop1", "value1"));
+ assertThat(resource.getChild("item2"), ResourceMatchers.props("prop2", 5L));
+
+ // remove config collection items
+ assertTrue(underTest.persistConfigurationCollection(context.resourceResolver(), "/conf/test",
+ new ConfigurationCollectionPersistData(ImmutableList.<ConfigurationPersistData>of())));
+ context.resourceResolver().commit();
+
+ resource = context.resourceResolver().getResource("/conf/test");
+ assertEquals(0, ImmutableList.copyOf(resource.getChildren()).size());
+
+ underTest.deleteConfiguration(context.resourceResolver(), "/conf/test");
+ assertNull(context.resourceResolver().getResource("/conf/test"));
+ }
+
+ @Test
+ public void testPersistConfigurationCollection_Nested() throws Exception {
+ ConfigurationPersistenceStrategy2 underTest = context.registerInjectActivateService(new DefaultConfigurationPersistenceStrategy());
+
+ // store new config collection items
+ assertTrue(underTest.persistConfigurationCollection(context.resourceResolver(), "/conf/test",
+ new ConfigurationCollectionPersistData(ImmutableList.of(
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value1")).collectionItemName("item1"),
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value2")).collectionItemName("item2")
+ ))
+ ));
+
+ // store nested items
+ assertTrue(underTest.persistConfigurationCollection(context.resourceResolver(), "/conf/test/item1",
+ new ConfigurationCollectionPersistData(ImmutableList.of(
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value11")).collectionItemName("sub1"),
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value12")).collectionItemName("sub2")
+ ))
+ ));
+ assertTrue(underTest.persistConfigurationCollection(context.resourceResolver(), "/conf/test/item2",
+ new ConfigurationCollectionPersistData(ImmutableList.of(
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value21")).collectionItemName("sub1")
+ ))
+ ));
+
+ context.resourceResolver().commit();
+
+
+ Resource resource = context.resourceResolver().getResource("/conf/test");
+ assertThat(resource, ResourceMatchers.containsChildren("item1", "item2"));
+
+ assertThat(resource.getChild("item1"), ResourceMatchers.props("prop1", "value1"));
+ assertThat(resource.getChild("item1"), ResourceMatchers.containsChildren("sub1", "sub2"));
+ assertThat(resource.getChild("item1/sub1"), ResourceMatchers.props("prop1", "value11"));
+ assertThat(resource.getChild("item1/sub2"), ResourceMatchers.props("prop1", "value12"));
+
+ assertThat(resource.getChild("item2"), ResourceMatchers.props("prop1", "value2"));
+ assertThat(resource.getChild("item2"), ResourceMatchers.containsChildren("sub1"));
+ assertThat(resource.getChild("item2/sub1"), ResourceMatchers.props("prop1", "value21"));
+
+
+ // update config collection items
+ assertTrue(underTest.persistConfigurationCollection(context.resourceResolver(), "/conf/test",
+ new ConfigurationCollectionPersistData(ImmutableList.of(
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value2-new")).collectionItemName("item2"),
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value1-new")).collectionItemName("item1"),
+ new ConfigurationPersistData(ImmutableMap.<String,Object>of("prop1", "value3-new")).collectionItemName("item3")
+ ))
+ ));
+ context.resourceResolver().commit();
+
+ resource = context.resourceResolver().getResource("/conf/test");
+ assertThat(resource, ResourceMatchers.containsChildren("item1", "item2", "item3"));
+
+ assertThat(resource.getChild("item1"), ResourceMatchers.props("prop1", "value1-new"));
+ assertThat(resource.getChild("item1"), ResourceMatchers.containsChildren("sub1", "sub2"));
+ assertThat(resource.getChild("item1/sub1"), ResourceMatchers.props("prop1", "value11"));
+ assertThat(resource.getChild("item1/sub2"), ResourceMatchers.props("prop1", "value12"));
+
+ assertThat(resource.getChild("item2"), ResourceMatchers.props("prop1", "value2-new"));
+ assertThat(resource.getChild("item2"), ResourceMatchers.containsChildren("sub1"));
+ assertThat(resource.getChild("item2/sub1"), ResourceMatchers.props("prop1", "value21"));
+
+ assertThat(resource.getChild("item3"), ResourceMatchers.props("prop1", "value3-new"));
+ assertFalse(resource.getChild("item3").listChildren().hasNext());
+
// remove config collection items
assertTrue(underTest.persistConfigurationCollection(context.resourceResolver(), "/conf/test",