MDSAL-269: fix missing identityref union members

Due to a API consistency issue in binding specification typedefs
which point to leafref and identityref types do not get a generated
encapsulation class.

BUG-8449/MDSAL-253 has dealt with the leafref case, but has omitted
identityref case. This patch corrects the situation, providing a bit
of background and a testcase.

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

diff --git a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/TypeProviderImpl.java b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/TypeProviderImpl.java
index cd1492ee39a0772c06547ae3be0e95ac0f64b413..7c72adff8f88fc35359e3f887efc577dded33c72 100644 (file)
--- a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/TypeProviderImpl.java
+++ b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/TypeProviderImpl.java
@@ -425,15 +425,65 @@ public final class TypeProviderImpl implements TypeProvider {
                 "Type Definitions Local Name cannot be NULL!");
 
         final TypeDefinition<?> baseTypeDef = baseTypeDefForExtendedType(typeDefinition);
-        if (!(baseTypeDef instanceof LeafrefTypeDefinition) && !(baseTypeDef instanceof IdentityrefTypeDefinition)) {
-            final Module module = findParentModule(schemaContext, parentNode);
+        if (baseTypeDef instanceof LeafrefTypeDefinition || baseTypeDef instanceof IdentityrefTypeDefinition) {
+            /*
+             * This is backwards compatibility baggage from way back when. The problem at hand is inconsistency between
+             * the fact that identity is mapped to a Class, which is also returned from leaves which specify it like
+             * this:
+             *
+             *     identity iden;
+             *
+             *     container foo {
+             *         leaf foo {
+             *             type identityref {
+             *                 base iden;
+             *             }
+             *         }
+             *     }
+             *
+             * This results in getFoo() returning Class<? extends Iden>, which looks fine on the surface, but gets more
+             * dicey when we throw in:
+             *
+             *     typedef bar-ref {
+             *         type identityref {
+             *             base iden;
+             *         }
+             *     }
+             *
+             *     container bar {
+             *         leaf bar {
+             *             type bar-ref;
+             *         }
+             *     }
+             *
+             * Now we have competing requirements: typedef would like us to use encapsulation to capture the defined
+             * type, while getBar() wants us to retain shape with getFoo(), as it should not matter how the identityref
+             * is formed.
+             *
+             * In this particular case getFoo() won just after the Binding Spec was frozen, hence we do not generate
+             * an encapsulation for identityref typedefs.
+             *
+             * In case you are thinking we could get by having foo-ref map to a subclass of Iden, that is not a good
+             * option, as it would look as though it is the product of a different construct:
+             *
+             *     identity bar-ref {
+             *         base iden;
+             *     }
+             *
+             * Leading to a rather nice namespace clash and also slight incompatibility with unknown third-party
+             * sub-identities of iden.
+             *
+             * The story behind leafrefs is probably similar, but that needs to be ascertained.
+             */
+            return null;
+        }
 
-            if (module != null) {
-                final Map<Optional<Revision>, Map<String, Type>> modulesByDate = genTypeDefsContextMap.get(module.getName());
-                final Map<String, Type> genTOs = modulesByDate.get(module.getRevision());
-                if (genTOs != null) {
-                    return genTOs.get(typeDefinition.getQName().getLocalName());
-                }
+        final Module module = findParentModule(schemaContext, parentNode);
+        if (module != null) {
+            final Map<Optional<Revision>, Map<String, Type>> modulesByDate = genTypeDefsContextMap.get(module.getName());
+            final Map<String, Type> genTOs = modulesByDate.get(module.getRevision());
+            if (genTOs != null) {
+                return genTOs.get(typeDefinition.getQName().getLocalName());
             }
         }
         return null;
@@ -734,6 +784,7 @@ public final class TypeProviderImpl implements TypeProvider {
         if (basePackageName != null && moduleName != null && typedef != null && typedef.getQName() != null) {
             final String typedefName = typedef.getQName().getLocalName();
             final TypeDefinition<?> innerTypeDefinition = typedef.getBaseType();
+            // See generatedTypeForExtendedDefinitionType() above for rationale behind this special case.
             if (!(innerTypeDefinition instanceof LeafrefTypeDefinition)
                     && !(innerTypeDefinition instanceof IdentityrefTypeDefinition)) {
                 Type returnType = null;
@@ -911,12 +962,14 @@ public final class TypeProviderImpl implements TypeProvider {
         final List<String> regularExpressions = new ArrayList<>();
         for (final TypeDefinition<?> unionType : unionTypes) {
             final String unionTypeName = unionType.getQName().getLocalName();
-            if (unionType.getBaseType() != null) {
-                resolveExtendedSubtypeAsUnion(unionGenTOBuilder, unionType, regularExpressions,
-                        parentNode);
+
+            // If we have a base type we should follow the type definition backwards, except for identityrefs, as those
+            // do not follow type encapsulation -- we use the general case for that.
+            if (unionType.getBaseType() != null  && !(unionType instanceof IdentityrefTypeDefinition)) {
+                resolveExtendedSubtypeAsUnion(unionGenTOBuilder, unionType, regularExpressions, parentNode);
             } else if (unionType instanceof UnionTypeDefinition) {
-                generatedTOBuilders.addAll(resolveUnionSubtypeAsUnion(unionGenTOBuilder, (UnionTypeDefinition) unionType,
-                        basePackageName, parentNode));
+                generatedTOBuilders.addAll(resolveUnionSubtypeAsUnion(unionGenTOBuilder,
+                    (UnionTypeDefinition) unionType, basePackageName, parentNode));
             } else if (unionType instanceof EnumTypeDefinition) {
                 final Enumeration enumeration = addInnerEnumerationToTypeBuilder((EnumTypeDefinition) unionType,
                         unionTypeName, unionGenTOBuilder);

diff --git a/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal269Test.java b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal269Test.java
new file mode 100644 (file)
index 0000000..76e6eb6
--- /dev/null
+++ b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal269Test.java
@@ -0,0 +1,51 @@
+/*
+ * 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.mdsal.binding.generator.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Iterator;
+import java.util.List;
+import org.junit.Test;
+import org.opendaylight.mdsal.binding.model.api.GeneratedProperty;
+import org.opendaylight.mdsal.binding.model.api.GeneratedTransferObject;
+import org.opendaylight.mdsal.binding.model.api.Type;
+import org.opendaylight.yangtools.yang.model.api.SchemaContext;
+import org.opendaylight.yangtools.yang.test.util.YangParserTestUtils;
+
+public class Mdsal269Test {
+
+    @Test
+    public void mdsal269Test() {
+        final SchemaContext context = YangParserTestUtils.parseYangResource("/mdsal269.yang");
+
+        final List<Type> generateTypes = new BindingGeneratorImpl(false).generateTypes(context);
+        assertNotNull(generateTypes);
+        assertEquals(4, generateTypes.size());
+
+        final Type mplsLabelType = generateTypes.stream().filter(type -> type.getFullyQualifiedName()
+            .equals("org.opendaylight.yang.gen.v1.mdsal269.rev180130.MplsLabel")).findFirst().get();
+
+        assertTrue(mplsLabelType instanceof GeneratedTransferObject);
+        final GeneratedTransferObject gto = (GeneratedTransferObject) mplsLabelType;
+        final Iterator<GeneratedProperty> it = gto.getEqualsIdentifiers().iterator();
+        final GeneratedProperty special = it.next();
+        final GeneratedProperty general = it.next();
+        assertFalse(it.hasNext());
+
+        assertEquals("mplsLabelGeneralUse", general.getName());
+        assertEquals("org.opendaylight.yang.gen.v1.mdsal269.rev180130.MplsLabelGeneralUse",
+            general.getReturnType().toString());
+
+        assertEquals("mplsLabelSpecialPurpose", special.getName());
+        assertEquals("Type (java.lang.Class)", special.getReturnType().toString());
+    }
+}

diff --git a/binding/mdsal-binding-generator-impl/src/test/resources/mdsal269.yang b/binding/mdsal-binding-generator-impl/src/test/resources/mdsal269.yang
new file mode 100644 (file)
index 0000000..58dfdc8
--- /dev/null
+++ b/binding/mdsal-binding-generator-impl/src/test/resources/mdsal269.yang
@@ -0,0 +1,30 @@
+module mdsal269 {
+    namespace "mdsal269";
+    prefix "mdsal269";
+
+    revision "2018-01-30";
+
+    identity mpls-label-special-purpose-value {
+
+    }
+
+    typedef mpls-label-special-purpose {
+        type identityref {
+            base mpls-label-special-purpose-value;
+        }
+    }
+
+    typedef mpls-label-general-use {
+        type uint32 {
+            range "16..1048575";
+        }
+    }
+
+    typedef mpls-label {
+        type union {
+            type mpls-label-special-purpose;
+            type mpls-label-general-use;
+        }
+    }
+}
+