YANGTOOLS-844: Filtering/SimpleSchemaContext violate getModules() contract

getModules() is explicit about how the modules should be ordered,
which is not compatible with ModuleDependencySort. As it turns out
ModuleDependencySort is only needed in MD-SAL codegen, where we
already invoke it explicitly.

Explicitly sort modules according to requirements of getModules(),
fulfilling that contract.

This also affects findModule(String) and related functions, which
are sorted, but in inverse order of what is specified. Fix that
by adjusting AbstractSchemaContext.REVISION_COMPARATOR and
introducing NAME_REVISION_COMPARATOR for use in places where modules
are not guaranteed to have a unique name.

Change-Id: Ie91685b830694764158381e4696e3f7d5bb57fba
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/AbstractSchemaContext.java b/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/AbstractSchemaContext.java
index be3e24d5834523b807b4b4a17f0913043ae161fb..fa6779137325e98eba956161315294316b9a2a59 100644 (file)
--- a/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/AbstractSchemaContext.java
+++ b/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/AbstractSchemaContext.java
@@ -41,9 +41,30 @@ import org.opendaylight.yangtools.yang.model.api.UnknownSchemaNode;
 import org.opendaylight.yangtools.yang.model.api.UsesNode;
 
 public abstract class AbstractSchemaContext implements SchemaContext {
+    /**
+     * A {@link Module} comparator based on {@link Module#getRevision()}, placing latest revision first. Note this
+     * comparator does not take into account module name and so two modules with different names but same revisions
+     * compare as equal.
+     */
     protected static final Comparator<Module> REVISION_COMPARATOR =
-        (first, second) -> Revision.compare(first.getRevision(), second.getRevision());
+        (first, second) -> Revision.compare(second.getRevision(), first.getRevision());
+
+    /**
+     * A {@link Module} comparator based on {@link Module#getName()} and {@link Module#getRevision()}, ordering modules
+     * lexicographically by their name and then in order of descending revision. This comparator assumes that
+     * the combination of these two attributes is sufficient to be consistent with hashCode/equals.
+     */
+    protected static final Comparator<Module> NAME_REVISION_COMPARATOR = (first, second) -> {
+        final int cmp = first.getName().compareTo(second.getName());
+        return cmp != 0 ? cmp : REVISION_COMPARATOR.compare(first, second);
+    };
 
+    /**
+     * Create a TreeSet for containing Modules with the same name, such that the set is ordered
+     * by {@link #REVISION_COMPARATOR}.
+     *
+     * @return A fresh TreeSet instance.
+     */
     protected static final TreeSet<Module> createModuleSet() {
         return new TreeSet<>(REVISION_COMPARATOR);
     }

diff --git a/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/FilteringSchemaContextProxy.java b/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/FilteringSchemaContextProxy.java
index b7a599fa092496deb201143caedefa6aff6ac00d..e804c39f5e51d748c662a498666bc885b1b35492 100644 (file)
--- a/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/FilteringSchemaContextProxy.java
+++ b/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/FilteringSchemaContextProxy.java
@@ -12,6 +12,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSet.Builder;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Lists;
@@ -21,6 +22,7 @@ import com.google.common.collect.Multimaps;
 import com.google.common.collect.SetMultimap;
 import com.google.common.collect.TreeMultimap;
 import java.net.URI;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -63,11 +65,12 @@ public final class FilteringSchemaContextProxy extends AbstractSchemaContext {
         final Builder<Module> filteredModulesBuilder = new Builder<>();
 
         // preparing map to get all modules with one name but difference in revision
-        final TreeMultimap<String, Module> nameToModulesAll = getStringModuleTreeMultimap();
+        final TreeMultimap<String, Module> nameToModulesAll = TreeMultimap.create(String::compareTo,
+            REVISION_COMPARATOR);
 
         nameToModulesAll.putAll(getStringModuleMap(delegate));
 
-        // in case there is a particular dependancy to view filteredModules/yang models dependancy is checked
+        // in case there is a particular dependency to view filteredModules/YANG models dependency is checked
         // for module name and imports
         processForRootModules(delegate, rootModules, filteredModulesBuilder);
 
@@ -79,10 +82,11 @@ public final class FilteringSchemaContextProxy extends AbstractSchemaContext {
                 filteredModulesBuilder.build(), nameToModulesAll));
 
         /**
-         * Instead of doing this on each invocation of getModules(), pre-compute
-         * it once and keep it around -- better than the set we got in.
+         * Instead of doing this on each invocation of getModules(), pre-compute it once and keep it around.
          */
-        this.filteredModules = filteredModulesBuilder.build();
+        final List<Module> sortedModules = new ArrayList<>(filteredModulesBuilder.build());
+        sortedModules.sort(NAME_REVISION_COMPARATOR);
+        this.filteredModules = ImmutableSet.copyOf(sortedModules);
 
         final SetMultimap<URI, Module> nsMap = Multimaps.newSetMultimap(new TreeMap<>(),
             AbstractSchemaContext::createModuleSet);
@@ -100,10 +104,6 @@ public final class FilteringSchemaContextProxy extends AbstractSchemaContext {
         moduleMap = moduleMapBuilder.build();
     }
 
-    private static TreeMultimap<String, Module> getStringModuleTreeMultimap() {
-        return TreeMultimap.create(String::compareTo, REVISION_COMPARATOR);
-    }
-
     private static void processForAdditionalModules(final SchemaContext delegate,
             final Set<ModuleId> additionalModuleIds, final Builder<Module> filteredModulesBuilder) {
         filteredModulesBuilder.addAll(Collections2.filter(delegate.getModules(),
@@ -130,7 +130,7 @@ public final class FilteringSchemaContextProxy extends AbstractSchemaContext {
             for (ModuleImport moduleImport : module.getImports()) {
                 Optional<Revision> revisionDate = moduleImport.getRevision();
                 if (!revisionDate.isPresent()) {
-                    revisionDate = nameToModulesAll.get(moduleImport.getModuleName()).last().getRevision();
+                    revisionDate = nameToModulesAll.get(moduleImport.getModuleName()).first().getRevision();
                 }
 
                 ModuleId key = new ModuleId(moduleImport.getModuleName(), revisionDate);

diff --git a/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/SimpleSchemaContext.java b/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/SimpleSchemaContext.java
index 3e860502ac67e71440d3ebd23306a8c10b6d5f34..dd452ed2202b4ed7a5b788b7f36c16db93a3465d 100644 (file)
--- a/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/SimpleSchemaContext.java
+++ b/yang/yang-model-util/src/main/java/org/opendaylight/yangtools/yang/model/util/SimpleSchemaContext.java
@@ -17,6 +17,8 @@ import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Multimaps;
 import com.google.common.collect.SetMultimap;
 import java.net.URI;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
@@ -37,10 +39,15 @@ public class SimpleSchemaContext extends AbstractSchemaContext {
 
     protected SimpleSchemaContext(final Set<Module> modules) {
         /*
-         * Instead of doing this on each invocation of getModules(), pre-compute
-         * it once and keep it around -- better than the set we got in.
+         * Instead of doing this on each invocation of getModules(), pre-compute it once and keep it around -- better
+         * than the set we got in.
+         *
+         * Note we are performing two sort operations: the dependency sort takes care of detecting multiple imports,
+         * performing sorting as a side-effect, but we really want the modules sorted to comply with getModules().
          */
-        this.modules = ImmutableSet.copyOf(ModuleDependencySort.sort(modules));
+        final List<Module> sortedModules = new ArrayList<>(ModuleDependencySort.sort(modules));
+        sortedModules.sort(NAME_REVISION_COMPARATOR);
+        this.modules = ImmutableSet.copyOf(sortedModules);
 
         /*
          * The most common lookup is from Namespace->Module.

diff --git a/yang/yang-model-util/src/test/java/org/opendaylight/yangtools/yang/model/util/SchemaContextProxyTest.java b/yang/yang-model-util/src/test/java/org/opendaylight/yangtools/yang/model/util/SchemaContextProxyTest.java
index 26ab38275ee549d1477c13beaef13c4c653bbfc3..8b504856104cb5faf2eed3574b65e17f9dc58f6f 100644 (file)
--- a/yang/yang-model-util/src/test/java/org/opendaylight/yangtools/yang/model/util/SchemaContextProxyTest.java
+++ b/yang/yang-model-util/src/test/java/org/opendaylight/yangtools/yang/model/util/SchemaContextProxyTest.java
@@ -53,9 +53,11 @@ public class SchemaContextProxyTest {
     private static final String MODULE41_NAME = "module41";
     private static final String MODULE5_NAME = "module5";
 
-    private static SchemaContext mockSchema(final Module... module) {
+    private static SchemaContext mockSchema(final Module... modules) {
+        final List<Module> sortedModules = Arrays.asList(modules);
+        sortedModules.sort(AbstractSchemaContext.NAME_REVISION_COMPARATOR);
         SchemaContext mock = mock(SchemaContext.class);
-        doReturn(ImmutableSet.copyOf(module)).when(mock).getModules();
+        doReturn(ImmutableSet.copyOf(sortedModules)).when(mock).getModules();
         return mock;
     }
 

diff --git a/yang/yang-model-util/src/test/java/org/opendaylight/yangtools/yang/model/util/SimpleSchemaContextTest.java b/yang/yang-model-util/src/test/java/org/opendaylight/yangtools/yang/model/util/SimpleSchemaContextTest.java
new file mode 100644 (file)
index 0000000..1218471
--- /dev/null
+++ b/yang/yang-model-util/src/test/java/org/opendaylight/yangtools/yang/model/util/SimpleSchemaContextTest.java
@@ -0,0 +1,81 @@
+/*
+ * Copyright (c) 2018 Pantheon Technologies, s.r.o. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.yangtools.yang.model.util;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import java.net.URI;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import org.junit.Test;
+import org.opendaylight.yangtools.yang.common.QNameModule;
+import org.opendaylight.yangtools.yang.common.Revision;
+import org.opendaylight.yangtools.yang.model.api.Module;
+
+public class SimpleSchemaContextTest {
+    @Test
+    public void testGetModulesOrdering() {
+        final Module foo0 = mockModule("foo", Optional.empty());
+        final Module foo1 = mockModule("foo", Revision.ofNullable("2018-01-01"));
+        final Module foo2 = mockModule("foo", Revision.ofNullable("2018-01-16"));
+
+        final List<Module> expected = ImmutableList.of(foo2, foo1, foo0);
+        assertGetModules(expected, foo0, foo1, foo2);
+        assertGetModules(expected, foo0, foo2, foo1);
+        assertGetModules(expected, foo1, foo0, foo2);
+        assertGetModules(expected, foo1, foo2, foo0);
+        assertGetModules(expected, foo2, foo0, foo1);
+        assertGetModules(expected, foo2, foo1, foo0);
+
+        assertFindModules(expected, "foo", foo0, foo1, foo2);
+        assertFindModules(expected, "foo", foo0, foo2, foo1);
+        assertFindModules(expected, "foo", foo1, foo0, foo2);
+        assertFindModules(expected, "foo", foo1, foo2, foo0);
+        assertFindModules(expected, "foo", foo2, foo0, foo1);
+        assertFindModules(expected, "foo", foo2, foo1, foo0);
+
+        final URI uri = URI.create("foo");
+        assertFindModules(expected, uri, foo0, foo1, foo2);
+        assertFindModules(expected, uri, foo0, foo2, foo1);
+        assertFindModules(expected, uri, foo1, foo0, foo2);
+        assertFindModules(expected, uri, foo1, foo2, foo0);
+        assertFindModules(expected, uri, foo2, foo0, foo1);
+        assertFindModules(expected, uri, foo2, foo1, foo0);
+    }
+
+    private static void assertGetModules(final List<Module> expected, final Module... modules) {
+        final Set<Module> actual = SimpleSchemaContext.forModules(ImmutableSet.copyOf(modules)).getModules();
+        assertArrayEquals(expected.toArray(), actual.toArray());
+    }
+
+    private static void assertFindModules(final List<Module> expected, final String name, final Module... modules) {
+        final Set<Module> actual = SimpleSchemaContext.forModules(ImmutableSet.copyOf(modules)).findModules(name);
+        assertArrayEquals(expected.toArray(), actual.toArray());
+    }
+
+    private static void assertFindModules(final List<Module> expected, final URI uri, final Module... modules) {
+        final Set<Module> actual = SimpleSchemaContext.forModules(ImmutableSet.copyOf(modules)).findModules(uri);
+        assertArrayEquals(expected.toArray(), actual.toArray());
+    }
+
+    private static Module mockModule(final String name, final Optional<Revision> revision) {
+        final QNameModule mod = QNameModule.create(URI.create(name), revision);
+        final Module ret = mock(Module.class);
+        doReturn(name).when(ret).getName();
+        doReturn(mod.getNamespace()).when(ret).getNamespace();
+        doReturn(mod.getRevision()).when(ret).getRevision();
+        doReturn(mod).when(ret).getQNameModule();
+        doReturn(mod.toString()).when(ret).toString();
+        return ret;
+    }
+}