ONOS-4129 Fixing issue where two pending configurations with the same config key, but for two different subject classes are encountered.
Change-Id: I4de7f6e22bdf038dff91630f4cf576d9c38e9807
diff --git a/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java b/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
index f2bee40..5397f54 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
@@ -29,7 +29,6 @@
import com.fasterxml.jackson.databind.node.TextNode;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
@@ -124,14 +123,18 @@
// Sweep through any pending configurations, validate them and then prune them.
private void processPendingConfigs(ConfigFactory configFactory) {
- Set<ConfigKey> toBePruned = Sets.newHashSet();
- configs.keySet().forEach(k -> {
- if (Objects.equals(k.configKey, configFactory.configKey())) {
+ ImmutableSet.copyOf(configs.keySet()).forEach(k -> {
+ if (Objects.equals(k.configKey, configFactory.configKey()) &&
+ isAssignableFrom(configFactory, k)) {
validateConfig(k, configFactory, configs.get(k).value());
- toBePruned.add(k); // Prune whether valid or not
+ configs.remove(k); // Prune whether valid or not
}
});
- toBePruned.forEach(configs::remove);
+ }
+
+ @SuppressWarnings("unchecked")
+ private boolean isAssignableFrom(ConfigFactory configFactory, ConfigKey k) {
+ return configFactory.subjectFactory().subjectClass().isAssignableFrom(k.subject.getClass());
}
@SuppressWarnings("unchecked")
diff --git a/core/store/dist/src/test/java/org/onosproject/store/config/impl/DistributedNetworkConfigStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/config/impl/DistributedNetworkConfigStoreTest.java
index 4a399b5..3ae4dc3 100644
--- a/core/store/dist/src/test/java/org/onosproject/store/config/impl/DistributedNetworkConfigStoreTest.java
+++ b/core/store/dist/src/test/java/org/onosproject/store/config/impl/DistributedNetworkConfigStoreTest.java
@@ -59,6 +59,11 @@
public class BasicConfig extends Config<String> { }
/**
+ * Config class for testing.
+ */
+ public class BasicIntConfig extends Config<Integer> { }
+
+ /**
* Config factory class for testing.
*/
public class MockConfigFactory extends ConfigFactory<String, BasicConfig> {
@@ -72,6 +77,19 @@
}
/**
+ * Config factory class for testing.
+ */
+ public class MockIntConfigFactory extends ConfigFactory<Integer, BasicIntConfig> {
+ protected MockIntConfigFactory(Class<BasicIntConfig> configClass, String configKey) {
+ super(new MockIntSubjectFactory("strings"), configClass, configKey);
+ }
+ @Override
+ public BasicIntConfig createConfig() {
+ return new BasicIntConfig();
+ }
+ }
+
+ /**
* Subject factory class for testing.
*/
public class MockSubjectFactory extends SubjectFactory<String> {
@@ -86,6 +104,19 @@
}
/**
+ * Subject factory class for testing.
+ */
+ public class MockIntSubjectFactory extends SubjectFactory<Integer> {
+ protected MockIntSubjectFactory(String subjectClassKey) {
+ super(Integer.class, subjectClassKey);
+ }
+
+ @Override
+ public Integer createSubject(String subjectKey) {
+ return Integer.parseInt(subjectKey);
+ }
+ }
+ /**
* Tests creation, query and removal of a config.
*/
@Test
@@ -132,9 +163,44 @@
public void testApplyConfig() {
configStore.addConfigFactory(new MockConfigFactory(BasicConfig.class, "config1"));
- configStore.applyConfig("config1", BasicConfig.class, new ObjectMapper().createObjectNode());
- assertThat(configStore.getConfigClasses("config1"), hasSize(1));
+ configStore.applyConfig("subject", BasicConfig.class, new ObjectMapper().createObjectNode());
+ assertThat(configStore.getConfigClasses("subject"), hasSize(1));
assertThat(configStore.getSubjects(String.class, BasicConfig.class), hasSize(1));
assertThat(configStore.getSubjects(String.class), hasSize(1));
}
+
+ /**
+ * Tests inserting a pending configuration.
+ */
+ @Test
+ public void testPendingConfig() {
+ configStore.queueConfig("subject", "config1", new ObjectMapper().createObjectNode());
+ configStore.addConfigFactory(new MockConfigFactory(BasicConfig.class, "config1"));
+
+ assertThat(configStore.getConfigClasses("subject"), hasSize(1));
+ assertThat(configStore.getSubjects(String.class, BasicConfig.class), hasSize(1));
+ assertThat(configStore.getSubjects(String.class), hasSize(1));
+ }
+
+ /**
+ * Tests inserting a pending configuration for the same key, different subject.
+ */
+ @Test
+ public void testPendingConfigSameKey() {
+ configStore.queueConfig("subject", "config1", new ObjectMapper().createObjectNode());
+ configStore.queueConfig(123, "config1", new ObjectMapper().createObjectNode());
+ configStore.addConfigFactory(new MockConfigFactory(BasicConfig.class, "config1"));
+
+ assertThat(configStore.getConfigClasses("subject"), hasSize(1));
+ assertThat(configStore.getConfigClasses(123), hasSize(0));
+ assertThat(configStore.getSubjects(String.class, BasicConfig.class), hasSize(1));
+ assertThat(configStore.getSubjects(String.class), hasSize(1));
+
+ configStore.addConfigFactory(new MockIntConfigFactory(BasicIntConfig.class, "config1"));
+
+ assertThat(configStore.getConfigClasses("subject"), hasSize(1));
+ assertThat(configStore.getConfigClasses(123), hasSize(1));
+ assertThat(configStore.getSubjects(Integer.class, BasicIntConfig.class), hasSize(1));
+ assertThat(configStore.getSubjects(Integer.class), hasSize(1));
+ }
}