Fix failure to generate runtime types with duplicate schema

The equality contract for TypeDefinition has changed in yangtools-6,
so that TypeDefinitions compare as equal if they represent the same
*semantic* type.

In BindingRuntimeTimes we made the assumption that there is at most
a 1:1 mapping of schema-to-type and used a BiMap to track that
relationship.

Fix that assumption by keeping the maps separate, and teach
RuntimeTypeGenerator to be careful and use an IdentityHashMap to
build that for us.

JIRA: MDSAL-600
Change-Id: Iff5dc7116bf1e5e7c78050d4161282097a3ff033
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/RuntimeTypeGenerator.java b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/RuntimeTypeGenerator.java
index b49457ad5d242404cb9b48a05c2d32756ceed1c9..ee661724368dccd11fe732f90483e8119c784678 100644 (file)
--- a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/RuntimeTypeGenerator.java
+++ b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/RuntimeTypeGenerator.java
@@ -7,8 +7,6 @@
  */
 package org.opendaylight.mdsal.binding.generator.impl;
 
-import com.google.common.collect.BiMap;
-import com.google.common.collect.HashBiMap;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.Multimap;
 import java.util.Collection;
@@ -41,21 +39,28 @@ final class RuntimeTypeGenerator extends AbstractTypeGenerator {
 
     @NonNull BindingRuntimeTypes toTypeMapping() {
         final Map<Type, AugmentationSchemaNode> augmentationToSchema = new HashMap<>();
-        final BiMap<Type, WithStatus> typeToDefiningSchema = HashBiMap.create();
+        final Map<Type, WithStatus> typeToSchema = new HashMap<>();
         final Multimap<Type, Type> choiceToCases = HashMultimap.create();
         final Map<QName, Type> identities = new HashMap<>();
 
+        // Note: we are keying through WithStatus, but these nodes compare on semantics, so equivalent schema nodes
+        //       can result in two distinct types. We certainly need to keep them separate.
+        final Map<WithStatus, Type> schemaToType = new IdentityHashMap<>();
+
         /*
-         * Fun parts are here. ModuleContext maps have Builders in them, we want plan types. We may encounter each
+         * Fun parts are here. ModuleContext maps have Builders in them, we want plain types. We may encounter each
          * builder multiple times, hence we keep a builder->instance cache.
          */
         final Map<Type, Type> builderToType = new IdentityHashMap<>();
+
         for (final ModuleContext ctx : moduleContexts()) {
             for (Entry<Type, AugmentationSchemaNode> e : ctx.getTypeToAugmentation().entrySet()) {
                 augmentationToSchema.put(builtType(builderToType, e.getKey()), e.getValue());
             }
             for (Entry<Type, WithStatus> e : ctx.getTypeToSchema().entrySet()) {
-                typeToDefiningSchema.put(builtType(builderToType, e.getKey()), e.getValue());
+                final Type type = builtType(builderToType, e.getKey());
+                typeToSchema.put(type, e.getValue());
+                schemaToType.put(e.getValue(), type);
             }
             for (Entry<Type, Type> e : ctx.getChoiceToCases().entries()) {
                 choiceToCases.put(builtType(builderToType, e.getKey()), builtType(builderToType, e.getValue()));
@@ -65,7 +70,7 @@ final class RuntimeTypeGenerator extends AbstractTypeGenerator {
             }
         }
 
-        return new BindingRuntimeTypes(schemaContext(), augmentationToSchema, typeToDefiningSchema, choiceToCases,
+        return new BindingRuntimeTypes(schemaContext(), augmentationToSchema, typeToSchema, schemaToType, choiceToCases,
             identities);
     }
 

diff --git a/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal600Test.java b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal600Test.java
new file mode 100644 (file)
index 0000000..3a529c6
--- /dev/null
+++ b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal600Test.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2020 PANTHEON.tech, 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.mdsal.binding.generator.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertSame;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.junit.Test;
+import org.opendaylight.mdsal.binding.model.api.JavaTypeName;
+import org.opendaylight.yangtools.yang.common.QName;
+import org.opendaylight.yangtools.yang.model.api.ContainerSchemaNode;
+import org.opendaylight.yangtools.yang.model.api.LeafSchemaNode;
+import org.opendaylight.yangtools.yang.test.util.YangParserTestUtils;
+
+@NonNullByDefault
+public class Mdsal600Test {
+    private static final JavaTypeName FOO = JavaTypeName.create("org.opendaylight.yang.gen.v1.mdsal600.norev", "Foo");
+
+    @Test
+    public void mdsal600Test() {
+        final var models = YangParserTestUtils.parseYangResource("/mdsal600.yang");
+        final var module = models.getModules().iterator().next();
+        final var fooSchema = (ContainerSchemaNode) module.findDataTreeChild(QName.create("mdsal600", "foo"))
+            .orElseThrow();
+
+        // Verify leaves
+        final var barSchema = fooSchema.findDataTreeChild(QName.create("mdsal600", "bar")).orElseThrow();
+        final var bazSchema = fooSchema.findDataTreeChild(QName.create("mdsal600", "baz")).orElseThrow();
+        final var barTypeSchema = ((LeafSchemaNode) barSchema).getType();
+        final var bazTypeSchema = ((LeafSchemaNode) bazSchema).getType();
+
+        // Precondition to our bug: the two types compare as equal, but are not the same
+        assertEquals(barTypeSchema, bazTypeSchema);
+        assertNotSame(barTypeSchema, bazTypeSchema);
+
+        // Quick check for codegen, not really interesting
+        final var generatedTypes = DefaultBindingGenerator.generateFor(models);
+        assertEquals(2, generatedTypes.size());
+
+        // The real thing, used to kaboom
+        final var runtimeTypes = new DefaultBindingRuntimeGenerator().generateTypeMapping(models);
+
+        // Verify schema-to-type lookup
+        final var barType = runtimeTypes.findType(barTypeSchema).orElseThrow();
+        final var bazType = runtimeTypes.findType(bazTypeSchema).orElseThrow();
+        assertEquals(FOO.createEnclosed("Bar"), barType.getIdentifier());
+        assertEquals(FOO.createEnclosed("Baz"), bazType.getIdentifier());
+
+        // Verify type-to-schema lookup
+        assertSame(barTypeSchema, runtimeTypes.findSchema(barType).orElseThrow());
+        assertSame(bazTypeSchema, runtimeTypes.findSchema(bazType).orElseThrow());
+    }
+}

diff --git a/binding/mdsal-binding-generator-impl/src/test/resources/mdsal600.yang b/binding/mdsal-binding-generator-impl/src/test/resources/mdsal600.yang
new file mode 100644 (file)
index 0000000..3d008e1
--- /dev/null
+++ b/binding/mdsal-binding-generator-impl/src/test/resources/mdsal600.yang
@@ -0,0 +1,31 @@
+module mdsal600 {
+  prefix mdsal600;
+  namespace mdsal600;
+
+  container foo {
+    leaf bar {
+      type enumeration {
+        enum foo {
+          value 0;
+          description "does not even matter";
+        }
+        enum bar {
+          value 1;
+        }
+      }
+    }
+
+    leaf baz {
+      type enumeration {
+        enum foo {
+          value 0;
+        }
+        enum bar {
+          value 1;
+          reference "is useless as well";
+        }
+      }
+    }
+  }
+}
+ 

diff --git a/binding/mdsal-binding-runtime-api/src/main/java/org/opendaylight/mdsal/binding/runtime/api/BindingRuntimeTypes.java b/binding/mdsal-binding-runtime-api/src/main/java/org/opendaylight/mdsal/binding/runtime/api/BindingRuntimeTypes.java
index 3ab49d66e5751e79822291fd13de9b6c003c3d50..70622a9da979e995959e50f5ffbf95d84a463bc4 100644 (file)
--- a/binding/mdsal-binding-runtime-api/src/main/java/org/opendaylight/mdsal/binding/runtime/api/BindingRuntimeTypes.java
+++ b/binding/mdsal-binding-runtime-api/src/main/java/org/opendaylight/mdsal/binding/runtime/api/BindingRuntimeTypes.java
@@ -12,7 +12,6 @@ import static java.util.Objects.requireNonNull;
 import com.google.common.annotations.Beta;
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.BiMap;
-import com.google.common.collect.ImmutableBiMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMultimap;
@@ -20,6 +19,7 @@ import com.google.common.collect.Multimap;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.VarHandle;
 import java.util.Collection;
+import java.util.IdentityHashMap;
 import java.util.Map;
 import java.util.Optional;
 import org.eclipse.jdt.annotation.NonNull;
@@ -32,12 +32,15 @@ import org.opendaylight.yangtools.yang.model.api.EffectiveModelContext;
 import org.opendaylight.yangtools.yang.model.api.EffectiveModelContextProvider;
 import org.opendaylight.yangtools.yang.model.api.SchemaNode;
 import org.opendaylight.yangtools.yang.model.api.stmt.SchemaNodeIdentifier.Absolute;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * The result of BindingGenerator run. Contains mapping between Types and SchemaNodes.
  */
 @Beta
 public final class BindingRuntimeTypes implements EffectiveModelContextProvider, Immutable {
+    private static final Logger LOG = LoggerFactory.getLogger(BindingRuntimeTypes.class);
     private static final VarHandle TYPE_TO_IDENTIFIER;
 
     static {
@@ -51,9 +54,11 @@ public final class BindingRuntimeTypes implements EffectiveModelContextProvider,
 
     private final @NonNull EffectiveModelContext schemaContext;
     private final ImmutableMap<Type, AugmentationSchemaNode> typeToAugmentation;
-    private final ImmutableBiMap<Type, WithStatus> typeToSchema;
+    private final ImmutableMap<Type, WithStatus> typeToSchema;
     private final ImmutableMultimap<Type, Type> choiceToCases;
     private final ImmutableMap<QName, Type> identities;
+    // Not Immutable as we use two different implementations
+    private final Map<WithStatus, Type> schemaToType;
 
     @SuppressWarnings("unused")
     // Accessed via TYPE_TO_IDENTIFIER
@@ -61,13 +66,33 @@ public final class BindingRuntimeTypes implements EffectiveModelContextProvider,
 
     public BindingRuntimeTypes(final EffectiveModelContext schemaContext,
             final Map<Type, AugmentationSchemaNode> typeToAugmentation,
-            final BiMap<Type, WithStatus> typeToDefiningSchema, final Multimap<Type, Type> choiceToCases,
-            final Map<QName, Type> identities) {
+            final Map<Type, WithStatus> typeToSchema, final Map<WithStatus, Type> schemaToType,
+            final Multimap<Type, Type> choiceToCases, final Map<QName, Type> identities) {
         this.schemaContext = requireNonNull(schemaContext);
         this.typeToAugmentation = ImmutableMap.copyOf(typeToAugmentation);
-        this.typeToSchema = ImmutableBiMap.copyOf(typeToDefiningSchema);
+        this.typeToSchema = ImmutableMap.copyOf(typeToSchema);
         this.choiceToCases = ImmutableMultimap.copyOf(choiceToCases);
         this.identities = ImmutableMap.copyOf(identities);
+
+        // Careful to use identity for SchemaNodes, but only if needed
+        // FIXME: 8.0.0: YT should be switching to identity for equals(), so this should become unnecessary
+        Map<WithStatus, Type> copy;
+        try {
+            copy = ImmutableMap.copyOf(schemaToType);
+        } catch (IllegalArgumentException e) {
+            LOG.debug("Equality-duplicates found in {}", schemaToType.keySet());
+            copy = new IdentityHashMap<>(schemaToType);
+        }
+
+        this.schemaToType = copy;
+    }
+
+    public BindingRuntimeTypes(final EffectiveModelContext schemaContext,
+            final Map<Type, AugmentationSchemaNode> typeToAugmentation,
+            final BiMap<Type, WithStatus> typeToDefiningSchema,
+            final Multimap<Type, Type> choiceToCases, final Map<QName, Type> identities) {
+        this(schemaContext, typeToAugmentation, typeToDefiningSchema, typeToDefiningSchema.inverse(), choiceToCases,
+            identities);
     }
 
     @Override
@@ -94,7 +119,7 @@ public final class BindingRuntimeTypes implements EffectiveModelContextProvider,
     }
 
     public Optional<Type> findType(final WithStatus schema) {
-        return Optional.ofNullable(typeToSchema.inverse().get(schema));
+        return Optional.ofNullable(schemaToType.get(schema));
     }
 
     public Multimap<Type, Type> getChoiceToCases() {

diff --git a/binding/mdsal-binding-test-model/src/main/yang/mdsal600.yang b/binding/mdsal-binding-test-model/src/main/yang/mdsal600.yang
new file mode 100644 (file)
index 0000000..3d008e1
--- /dev/null
+++ b/binding/mdsal-binding-test-model/src/main/yang/mdsal600.yang
@@ -0,0 +1,31 @@
+module mdsal600 {
+  prefix mdsal600;
+  namespace mdsal600;
+
+  container foo {
+    leaf bar {
+      type enumeration {
+        enum foo {
+          value 0;
+          description "does not even matter";
+        }
+        enum bar {
+          value 1;
+        }
+      }
+    }
+
+    leaf baz {
+      type enumeration {
+        enum foo {
+          value 0;
+        }
+        enum bar {
+          value 1;
+          reference "is useless as well";
+        }
+      }
+    }
+  }
+}
+