ONOS-4129 Fixing issue where two pending configurations with the same config key…

…, but for two different subject classes are encountered.

Change-Id: I4de7f6e22bdf038dff91630f4cf576d9c38e9807

@@ -29,7 +29,6 @@ import com.fasterxml.jackson.databind.node.ShortNode;
 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 @@ public class DistributedNetworkConfigStore
 
     // 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")

@@ -59,6 +59,11 @@ public class DistributedNetworkConfigStoreTest {
     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 @@ public class DistributedNetworkConfigStoreTest {
     }
 
     /**
+     * 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 @@ public class DistributedNetworkConfigStoreTest {
     }
 
     /**
+     * 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 class DistributedNetworkConfigStoreTest {
     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));
+    }
 }