From a4150d21a0c40757d25a4dad02e7746bb1716d10 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 27 Jul 2018 21:32:04 +0200 Subject: [PATCH] Rework inlined union generation Unions internal to a leaf union end up being incompletely generated, as they lack stringValue(), hashCode(), equals() and do not correctly bind to its enclosing builder -- leading to a generated code not being compilable. There are multiple issues here, all of which are addressed in this patch: - hashCode/equals properties are created in the wrong place - property return type is set to the builder, not its product - union builder type is not set as a union - builders for nested types are not correctly emitted - Class/InterfaceTemplate use different logic to emit the inner classes JIRA: MDSAL-320 Change-Id: I626fb4ac42ae6528bc98b809bc33756e8daa08b9 Signed-off-by: Robert Varga --- .../api/type/builder/GeneratedTOBuilder.java | 2 + .../generator/impl/AbstractTypeGenerator.java | 87 ++++++++----------- .../yang/types/AbstractTypeProvider.java | 8 +- .../binding/generator/impl/Mdsal320Test.java | 73 ++++++++++++++++ .../binding/generator/impl/Mdsal324Test.java | 2 +- .../src/test/resources/mdsal320.yang | 26 ++++++ .../builder/AbstractGeneratedTOBuilder.java | 7 +- .../builder/CodegenGeneratedTOBuilder.java | 4 +- .../java/api/generator/BaseTemplate.xtend | 11 +++ .../java/api/generator/ClassTemplate.xtend | 4 +- .../api/generator/InterfaceTemplate.xtend | 13 +-- .../api/generator/test/CompilationTest.java | 9 ++ .../compilation/mdsal320/mdsal320.yang | 26 ++++++ 13 files changed, 198 insertions(+), 74 deletions(-) create mode 100644 binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal320Test.java create mode 100644 binding/mdsal-binding-generator-impl/src/test/resources/mdsal320.yang create mode 100644 binding/mdsal-binding-java-api-generator/src/test/resources/compilation/mdsal320/mdsal320.yang diff --git a/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/GeneratedTOBuilder.java b/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/GeneratedTOBuilder.java index 277ca1f798..a604022987 100644 --- a/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/GeneratedTOBuilder.java +++ b/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/GeneratedTOBuilder.java @@ -90,6 +90,8 @@ public interface GeneratedTOBuilder extends GeneratedTypeBuilderBase typeDef); + boolean isUnion(); + /** * * @param isUnion diff --git a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/AbstractTypeGenerator.java b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/AbstractTypeGenerator.java index 06d57f2544..fd242571c9 100644 --- a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/AbstractTypeGenerator.java +++ b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/generator/impl/AbstractTypeGenerator.java @@ -1378,13 +1378,10 @@ abstract class AbstractTypeGenerator { } typeProvider.putReferencedType(leaf.getPath(), returnType); } else if (typeDef instanceof UnionTypeDefinition) { - GeneratedTOBuilder genTOBuilder = addTOToTypeBuilder((UnionTypeDefinition) typeDef, typeBuilder, leaf, - parentModule); - if (genTOBuilder != null) { - returnType = createReturnTypeForUnion(genTOBuilder, typeDef, typeBuilder, parentModule); - // Store the inner type within the union so that we can find the reference for it - context.addInnerTypedefType(typeDef.getPath(), returnType); - } + final UnionTypeDefinition unionDef = (UnionTypeDefinition)typeDef; + returnType = addTOToTypeBuilder(unionDef, typeBuilder, leaf, parentModule); + // Store the inner type within the union so that we can find the reference for it + context.addInnerTypedefType(typeDef.getPath(), returnType); } else if (typeDef instanceof BitsTypeDefinition) { GeneratedTOBuilder genTOBuilder = addTOToTypeBuilder((BitsTypeDefinition) typeDef, typeBuilder, leaf, parentModule); @@ -1587,11 +1584,8 @@ abstract class AbstractTypeGenerator { returnType = new ReferencedTypeImpl(enumBuilder.getIdentifier()); typeProvider.putReferencedType(node.getPath(), returnType); } else if (typeDef instanceof UnionTypeDefinition) { - final GeneratedTOBuilder genTOBuilder = addTOToTypeBuilder((UnionTypeDefinition)typeDef, typeBuilder, - node, parentModule); - if (genTOBuilder != null) { - returnType = createReturnTypeForUnion(genTOBuilder, typeDef, typeBuilder, parentModule); - } + final UnionTypeDefinition unionDef = (UnionTypeDefinition)typeDef; + returnType = addTOToTypeBuilder(unionDef, typeBuilder, node, parentModule); } else if (typeDef instanceof BitsTypeDefinition) { final GeneratedTOBuilder genTOBuilder = addTOToTypeBuilder((BitsTypeDefinition)typeDef, typeBuilder, node, parentModule); @@ -1612,22 +1606,31 @@ abstract class AbstractTypeGenerator { return true; } - private Type createReturnTypeForUnion(final GeneratedTOBuilder genTOBuilder, final TypeDefinition typeDef, + private Type createReturnTypeForUnion(final GeneratedTOBuilder genTOBuilder, final UnionTypeDefinition typeDef, final GeneratedTypeBuilder typeBuilder, final Module parentModule) { - final GeneratedTOBuilder returnType = typeProvider.newGeneratedTOBuilder(genTOBuilder.getIdentifier()); - - addCodegenInformation(returnType, parentModule, typeDef); - returnType.setSchemaPath(typeDef.getPath()); - returnType.setModuleName(parentModule.getName()); + final GeneratedTOBuilder returnTypeBuilder = typeProvider.newGeneratedTOBuilder(genTOBuilder.getIdentifier()); + returnTypeBuilder.setIsUnion(true); + addCodegenInformation(returnTypeBuilder, parentModule, typeDef); + returnTypeBuilder.setSchemaPath(typeDef.getPath()); + returnTypeBuilder.setModuleName(parentModule.getName()); + final GeneratedTransferObject returnType = returnTypeBuilder.build(); genTOBuilder.setTypedef(true); genTOBuilder.setIsUnion(true); AbstractTypeProvider.addUnitsToGenTO(genTOBuilder, typeDef.getUnits().orElse(null)); + createUnionBuilder(genTOBuilder, typeBuilder, returnType, parentModule); + return returnType; + } - - final GeneratedTOBuilder unionBuilder = createUnionBuilder(genTOBuilder, typeBuilder); - + private void createUnionBuilder(final GeneratedTOBuilder genTOBuilder, final GeneratedTypeBuilder typeBuilder, + final GeneratedTransferObject returnType, final Module parentModule) { + // Append enclosing path hierarchy without dots + final StringBuilder sb = new StringBuilder(); + genTOBuilder.getIdentifier().localNameComponents().forEach(sb::append); + final GeneratedTOBuilder unionBuilder = typeProvider.newGeneratedTOBuilder( + JavaTypeName.create(typeBuilder.getPackageName(), sb.append("Builder").toString())); + unionBuilder.setIsUnionBuilder(true); final MethodSignatureBuilder method = unionBuilder.addMethod("getDefaultInstance"); method.setReturnType(returnType); @@ -1635,25 +1638,13 @@ abstract class AbstractTypeGenerator { method.setAccessModifier(AccessModifier.PUBLIC); method.setStatic(true); + final GeneratedTransferObject unionBuilderType = unionBuilder.build(); final Set types = typeProvider.getAdditionalTypes().get(parentModule); if (types == null) { - typeProvider.getAdditionalTypes().put(parentModule, - Sets.newHashSet(unionBuilder.build())); + typeProvider.getAdditionalTypes().put(parentModule, Sets.newHashSet(unionBuilderType)); } else { - types.add(unionBuilder.build()); + types.add(unionBuilderType); } - return returnType.build(); - } - - private GeneratedTOBuilder createUnionBuilder(final GeneratedTOBuilder genTOBuilder, - final GeneratedTypeBuilder typeBuilder) { - // Append enclosing path hierarchy without dots - final StringBuilder sb = new StringBuilder(); - genTOBuilder.getIdentifier().localNameComponents().forEach(sb::append); - final GeneratedTOBuilder unionBuilder = typeProvider.newGeneratedTOBuilder( - JavaTypeName.create(typeBuilder.getPackageName(), sb.append("Builder").toString())); - unionBuilder.setIsUnionBuilder(true); - return unionBuilder; } private GeneratedTypeBuilder addDefaultInterfaceDefinition(final ModuleContext context, @@ -1667,7 +1658,6 @@ abstract class AbstractTypeGenerator { return addDefaultInterfaceDefinition(packageName, schemaNode, baseInterface, context); } - /** * Instantiates generated type builder with packageName and * schemaNode. @@ -1967,7 +1957,7 @@ abstract class AbstractTypeGenerator { * parent module * @return generated TO builder for typeDef */ - private GeneratedTOBuilder addTOToTypeBuilder(final UnionTypeDefinition typeDef, + private Type addTOToTypeBuilder(final UnionTypeDefinition typeDef, final GeneratedTypeBuilder typeBuilder, final DataSchemaNode leaf, final Module parentModule) { final List types = typeProvider.provideGeneratedTOBuildersForUnionTypeDef( typeBuilder.getIdentifier().createEnclosed(BindingMapping.getClassName(leaf.getQName())), @@ -1976,12 +1966,17 @@ abstract class AbstractTypeGenerator { checkState(!types.isEmpty(), "No GeneratedTOBuilder objects generated from union %s", typeDef); final List genTOBuilders = new ArrayList<>(types); final GeneratedTOBuilder resultTOBuilder = types.remove(0); - for (final GeneratedTOBuilder genTOBuilder : types) { - resultTOBuilder.addEnclosingTransferObject(genTOBuilder); + types.forEach(resultTOBuilder::addEnclosingTransferObject); + genTOBuilders.forEach(typeBuilder::addEnclosingTransferObject); + + for (GeneratedTOBuilder builder : types) { + if (builder.isUnion()) { + final GeneratedTransferObject type = builder.build(); + createUnionBuilder(builder, typeBuilder, type, parentModule); + } } - processEnclosedTOBuilderes(typeBuilder, genTOBuilders); - return resultTOBuilder; + return createReturnTypeForUnion(resultTOBuilder, typeDef, typeBuilder, parentModule); } /** @@ -2014,14 +2009,6 @@ abstract class AbstractTypeGenerator { } - private static GeneratedTOBuilder processEnclosedTOBuilderes(final GeneratedTypeBuilder typeBuilder, - final List genTOBuilders) { - for (final GeneratedTOBuilder genTOBuilder : genTOBuilders) { - typeBuilder.addEnclosingTransferObject(genTOBuilder); - } - return genTOBuilders.get(0); - } - /** * Adds the implemented types to type builder. * diff --git a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/AbstractTypeProvider.java b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/AbstractTypeProvider.java index 11a0e726de..1e990f5206 100644 --- a/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/AbstractTypeProvider.java +++ b/binding/mdsal-binding-generator-impl/src/main/java/org/opendaylight/mdsal/binding/yang/types/AbstractTypeProvider.java @@ -898,11 +898,10 @@ public abstract class AbstractTypeProvider implements TypeProvider { private GeneratedTransferObject wrapJavaTypeIntoTO(final String basePackageName, final TypeDefinition typedef, final Type javaType, final String moduleName) { Preconditions.checkNotNull(javaType, "javaType cannot be null"); - final String propertyName = "value"; final GeneratedTOBuilder genTOBuilder = typedefToTransferObject(basePackageName, typedef, moduleName); genTOBuilder.setRestrictions(BindingGeneratorUtil.getRestrictions(typedef)); - final GeneratedPropertyBuilder genPropBuilder = genTOBuilder.addProperty(propertyName); + final GeneratedPropertyBuilder genPropBuilder = genTOBuilder.addProperty("value"); genPropBuilder.setReturnType(javaType); genTOBuilder.addEqualsIdentity(genPropBuilder); genTOBuilder.addHashIdentity(genPropBuilder); @@ -965,12 +964,11 @@ public abstract class AbstractTypeProvider implements TypeProvider { final Module module = findParentModule(schemaContext, parentNode); final GeneratedTOBuilder unionGenTOBuilder = newGeneratedTOBuilder(typeName); + unionGenTOBuilder.setIsUnion(true); unionGenTOBuilder.setSchemaPath(typedef.getPath()); unionGenTOBuilder.setModuleName(module.getName()); addCodegenInformation(unionGenTOBuilder, typedef); - generatedTOBuilders.add(unionGenTOBuilder); - unionGenTOBuilder.setIsUnion(true); // Pattern string is the key, XSD regex is the value. The reason for this choice is that the pattern carries // also negation information and hence guarantees uniqueness. @@ -1030,7 +1028,7 @@ public abstract class AbstractTypeProvider implements TypeProvider { final GeneratedPropertyBuilder propertyBuilder; propertyBuilder = parentUnionGenTOBuilder.addProperty(BindingMapping.getPropertyName( newTOBuilderName.simpleName())); - propertyBuilder.setReturnType(subUnionGenTOBUilders.get(0)); + propertyBuilder.setReturnType(subUnionGenTOBUilders.get(0).build()); parentUnionGenTOBuilder.addEqualsIdentity(propertyBuilder); parentUnionGenTOBuilder.addToStringProperty(propertyBuilder); diff --git a/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal320Test.java b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal320Test.java new file mode 100644 index 0000000000..1bc39313d7 --- /dev/null +++ b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal320Test.java @@ -0,0 +1,73 @@ +/* + * 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.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.google.common.collect.Iterables; +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.GeneratedType; +import org.opendaylight.mdsal.binding.model.api.MethodSignature; +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 Mdsal320Test { + + @Test + public void mdsal320Test() { + final SchemaContext context = YangParserTestUtils.parseYangResource("/mdsal320.yang"); + + final List generateTypes = new BindingGeneratorImpl().generateTypes(context); + assertNotNull(generateTypes); + assertEquals(4, generateTypes.size()); + + final Type fooType = generateTypes.stream().filter(type -> type.getFullyQualifiedName() + .equals("org.opendaylight.yang.gen.v1.urn.odl.yt320.norev.Foo")).findFirst().get(); + assertTrue(fooType instanceof GeneratedType); + final GeneratedType foo = (GeneratedType) fooType; + + GeneratedTransferObject bar = null; + GeneratedTransferObject bar1 = null; + for (GeneratedType enc : foo.getEnclosedTypes()) { + switch (enc.getName()) { + case "Bar": + assertTrue(enc instanceof GeneratedTransferObject); + bar = (GeneratedTransferObject) enc; + break; + case "Bar$1": + assertTrue(enc instanceof GeneratedTransferObject); + bar1 = (GeneratedTransferObject) enc; + break; + default: + throw new IllegalStateException("Unexpected type " + enc); + } + } + assertNotNull(bar); + assertTrue(bar.isUnionType()); + assertNotNull(bar1); + assertTrue(bar1.isUnionType()); + + final MethodSignature getBar = Iterables.getOnlyElement(foo.getMethodDefinitions()); + final Type getBarType = getBar.getReturnType(); + assertTrue(getBarType instanceof GeneratedTransferObject); + final GeneratedTransferObject getBarTO = (GeneratedTransferObject) getBarType; + assertTrue(getBarTO.isUnionType()); + assertEquals(bar, getBarTO); + + final GeneratedProperty bar1Prop = bar.getProperties().stream().filter(prop -> "bar$1".equals(prop.getName())) + .findFirst().get(); + final Type bar1PropRet = bar1Prop.getReturnType(); + assertEquals(bar1, bar1PropRet); + } +} diff --git a/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal324Test.java b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal324Test.java index c5b5aabc38..914f57f8c2 100644 --- a/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal324Test.java +++ b/binding/mdsal-binding-generator-impl/src/test/java/org/opendaylight/mdsal/binding/generator/impl/Mdsal324Test.java @@ -23,6 +23,6 @@ public class Mdsal324Test { final SchemaContext context = YangParserTestUtils.parseYangResource("/mdsal324.yang"); final List generateTypes = new BindingGeneratorImpl().generateTypes(context); assertNotNull(generateTypes); - assertEquals(4, generateTypes.size()); + assertEquals(6, generateTypes.size()); } } diff --git a/binding/mdsal-binding-generator-impl/src/test/resources/mdsal320.yang b/binding/mdsal-binding-generator-impl/src/test/resources/mdsal320.yang new file mode 100644 index 0000000000..3c1f66cc01 --- /dev/null +++ b/binding/mdsal-binding-generator-impl/src/test/resources/mdsal320.yang @@ -0,0 +1,26 @@ +module yt320 { + namespace "urn:odl:yt320"; + prefix yt320; + + container foo { + leaf bar { + type union { + type enumeration { + enum "foo"; + } + type string { + length 2; + } + type union { + type enumeration { + enum bar; + } + type string { + length 1; + } + } + } + } + } +} + diff --git a/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/AbstractGeneratedTOBuilder.java b/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/AbstractGeneratedTOBuilder.java index c6786ece75..fb2378cef1 100644 --- a/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/AbstractGeneratedTOBuilder.java +++ b/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/AbstractGeneratedTOBuilder.java @@ -12,9 +12,9 @@ import java.util.Collections; import java.util.List; import org.opendaylight.mdsal.binding.model.api.GeneratedProperty; import org.opendaylight.mdsal.binding.model.api.GeneratedTransferObject; +import org.opendaylight.mdsal.binding.model.api.JavaTypeName; import org.opendaylight.mdsal.binding.model.api.ParameterizedType; import org.opendaylight.mdsal.binding.model.api.Type; -import org.opendaylight.mdsal.binding.model.api.JavaTypeName; import org.opendaylight.mdsal.binding.model.api.type.builder.GeneratedPropertyBuilder; import org.opendaylight.mdsal.binding.model.api.type.builder.GeneratedTOBuilder; import org.opendaylight.mdsal.binding.model.api.type.builder.MethodSignatureBuilder; @@ -132,6 +132,11 @@ abstract class AbstractGeneratedTOBuilder extends AbstractGeneratedTypeBuilder clazz, final String methodName, final String returnTypeStr) throws NoSuchMethodException { Method method = clazz.getMethod(methodName); diff --git a/binding/mdsal-binding-java-api-generator/src/test/resources/compilation/mdsal320/mdsal320.yang b/binding/mdsal-binding-java-api-generator/src/test/resources/compilation/mdsal320/mdsal320.yang new file mode 100644 index 0000000000..3c1f66cc01 --- /dev/null +++ b/binding/mdsal-binding-java-api-generator/src/test/resources/compilation/mdsal320/mdsal320.yang @@ -0,0 +1,26 @@ +module yt320 { + namespace "urn:odl:yt320"; + prefix yt320; + + container foo { + leaf bar { + type union { + type enumeration { + enum "foo"; + } + type string { + length 2; + } + type union { + type enumeration { + enum bar; + } + type string { + length 1; + } + } + } + } + } +} + -- 2.36.6