From dc1068db999a2bb1924bd81299cd74eba22b528f Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 9 Mar 2018 16:55:44 +0100 Subject: [PATCH] Add alternative enum assigned name mapping This patch corrects enumeration mapping rules so that any unicode string can be mapped to a valid Java identifier as per JLS8. This is done by attempting to apply className() mapping to all names and using that if it results in valid non-conflicting identifiers. If a conflict is detected, we use a bijective mapping, which encodes any conflicting characters using an $-based escaping scheme. JIRA: MDSAL-309 Change-Id: Ic51405e533eff9af6afc2abaa8d0cc193d718e64 Signed-off-by: Robert Varga (cherry picked from commit 53433dfe15da970a6af8362ad21911a82943de7d) --- .../data/codec/impl/EnumerationCodec.java | 39 ++++---- .../model/api/type/builder/EnumBuilder.java | 7 -- .../type/builder/EnumerationBuilderImpl.java | 43 ++++---- .../builder/EnumerationBuilderImplTest.java | 6 +- .../src/main/yang/opendaylight-mdsal309.yang | 62 ++++++++++++ .../yang/binding/BindingMapping.java | 97 +++++++++++++++++-- .../yang/binding/BindingMappingTest.java | 56 ++++++++++- 7 files changed, 256 insertions(+), 54 deletions(-) create mode 100644 binding/mdsal-binding-test-model/src/main/yang/opendaylight-mdsal309.yang diff --git a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/yangtools/binding/data/codec/impl/EnumerationCodec.java b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/yangtools/binding/data/codec/impl/EnumerationCodec.java index ecb8f6818f..e254560df6 100644 --- a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/yangtools/binding/data/codec/impl/EnumerationCodec.java +++ b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/yangtools/binding/data/codec/impl/EnumerationCodec.java @@ -7,11 +7,15 @@ */ package org.opendaylight.yangtools.binding.data.codec.impl; -import com.google.common.base.Preconditions; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; +import static java.util.Objects.requireNonNull; + +import com.google.common.collect.BiMap; import com.google.common.collect.ImmutableBiMap; -import java.util.HashMap; -import java.util.Map; +import com.google.common.collect.ImmutableBiMap.Builder; import java.util.concurrent.Callable; +import java.util.stream.Collectors; import org.opendaylight.yangtools.binding.data.codec.impl.ValueTypeCodec.SchemaUnawareCodec; import org.opendaylight.yangtools.yang.binding.BindingMapping; import org.opendaylight.yangtools.yang.model.api.type.EnumTypeDefinition; @@ -20,41 +24,42 @@ import org.opendaylight.yangtools.yang.model.api.type.EnumTypeDefinition.EnumPai final class EnumerationCodec extends ReflectionBasedCodec implements SchemaUnawareCodec { private final ImmutableBiMap> yangValueToBinding; - EnumerationCodec(final Class> enumeration, final Map> schema) { + EnumerationCodec(final Class> enumeration, final ImmutableBiMap> schema) { super(enumeration); - yangValueToBinding = ImmutableBiMap.copyOf(schema); + yangValueToBinding = requireNonNull(schema); } static Callable loader(final Class returnType, final EnumTypeDefinition enumSchema) { - Preconditions.checkArgument(Enum.class.isAssignableFrom(returnType)); + checkArgument(Enum.class.isAssignableFrom(returnType)); @SuppressWarnings({ "rawtypes", "unchecked" }) final Class> enumType = (Class) returnType; return () -> { - Map> nameToValue = new HashMap<>(); + final BiMap identifierToYang = BindingMapping.mapEnumAssignedNames( + enumSchema.getValues().stream().map(EnumPair::getName).collect(Collectors.toList())).inverse(); + + final Builder> builder = ImmutableBiMap.builder(); for (Enum enumValue : enumType.getEnumConstants()) { - nameToValue.put(enumValue.toString(), enumValue); + final String yangName = identifierToYang.get(enumValue.name()); + checkState(yangName != null, "Failed to find enumeration constant %s in mapping %s", enumValue, + identifierToYang); + builder.put(yangName, enumValue); } - Map> yangNameToBinding = new HashMap<>(); - for (EnumPair yangValue : enumSchema.getValues()) { - final String bindingName = BindingMapping.getClassName(yangValue.getName()); - final Enum bindingVal = nameToValue.get(bindingName); - yangNameToBinding.put(yangValue.getName(), bindingVal); - } - return new EnumerationCodec(enumType, yangNameToBinding); + + return new EnumerationCodec(enumType, builder.build()); }; } @Override public Object deserialize(final Object input) { Enum value = yangValueToBinding.get(input); - Preconditions.checkArgument(value != null, "Invalid enumeration value %s. Valid values are %s", input, + checkArgument(value != null, "Invalid enumeration value %s. Valid values are %s", input, yangValueToBinding.keySet()); return value; } @Override public Object serialize(final Object input) { - Preconditions.checkArgument(getTypeClass().isInstance(input), "Input must be instance of %s", getTypeClass()); + checkArgument(getTypeClass().isInstance(input), "Input must be instance of %s", getTypeClass()); return yangValueToBinding.inverse().get(input); } } \ No newline at end of file diff --git a/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/EnumBuilder.java b/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/EnumBuilder.java index 225f61aafe..ef95d8cc67 100644 --- a/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/EnumBuilder.java +++ b/binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/EnumBuilder.java @@ -34,13 +34,6 @@ public interface EnumBuilder extends Type { */ AnnotationTypeBuilder addAnnotation(final String packageName, final String name); - /** - * - * @param name - * @param value - */ - void addValue(final String name, final int value, final String description); - /** * * @param definingType diff --git a/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImpl.java b/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImpl.java index 72ebc0487f..07a704047b 100644 --- a/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImpl.java +++ b/binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImpl.java @@ -7,11 +7,16 @@ */ package org.opendaylight.mdsal.binding.model.util.generated.type.builder; +import static java.util.Objects.requireNonNull; + +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; +import java.util.stream.Collectors; import org.opendaylight.mdsal.binding.model.api.AnnotationType; import org.opendaylight.mdsal.binding.model.api.Constant; import org.opendaylight.mdsal.binding.model.api.Enumeration; @@ -67,7 +72,7 @@ public final class EnumerationBuilderImpl extends AbstractBaseType implements En @Override public AnnotationTypeBuilder addAnnotation(final String packageName, final String name) { - if ((packageName != null) && (name != null)) { + if (packageName != null && name != null) { final AnnotationTypeBuilder builder = new AnnotationTypeBuilderImpl(packageName, name); if (!this.annotationBuilders.contains(builder)) { this.annotationBuilders = LazyCollections.lazyAdd(this.annotationBuilders, builder); @@ -77,17 +82,17 @@ public final class EnumerationBuilderImpl extends AbstractBaseType implements En return null; } - @Override - public void addValue(final String name, final int value, final String description) { - final EnumPairImpl p = new EnumPairImpl(name, value, description); + @VisibleForTesting + void addValue(final String name, final String mappedName, final int value, final String description) { + final EnumPairImpl p = new EnumPairImpl(name, mappedName, value, description); this.values = LazyCollections.lazyAdd(this.values, p); this.unmodifiableValues = Collections.unmodifiableList(this.values); } @Override public Enumeration toInstance(final Type definingType) { - return new EnumerationImpl(definingType, this.annotationBuilders, this.packageName, this.name, this.unmodifiableValues, - this.description, this.reference, this.moduleName, this.schemaPath); + return new EnumerationImpl(definingType, this.annotationBuilders, this.packageName, this.name, + this.unmodifiableValues, this.description, this.reference, this.moduleName, this.schemaPath); } /* @@ -111,14 +116,13 @@ public final class EnumerationBuilderImpl extends AbstractBaseType implements En @Override public void updateEnumPairsFromEnumTypeDef(final EnumTypeDefinition enumTypeDef) { final List enums = enumTypeDef.getValues(); - if (enums != null) { - for (final EnumPair enumPair : enums) { - if (enumPair != null) { - this.addValue(enumPair.getName(), enumPair.getValue(), enumPair.getDescription()); - } - } - } + final Map valueIds = BindingMapping.mapEnumAssignedNames(enums.stream().map(EnumPair::getName) + .collect(Collectors.toList())); + for (EnumPair enumPair : enums) { + addValue(enumPair.getName(), valueIds.get(enumPair.getName()), enumPair.getValue(), + enumPair.getDescription()); + } } private static final class EnumPairImpl implements Enumeration.Pair { @@ -128,10 +132,9 @@ public final class EnumerationBuilderImpl extends AbstractBaseType implements En private final int value; private final String description; - public EnumPairImpl(final String name, final int value, final String description) { - super(); - this.name = name; - this.mappedName = BindingMapping.getClassName(name); + EnumPairImpl(final String name, final String mappedName, final int value, final String description) { + this.name = requireNonNull(name); + this.mappedName = requireNonNull(mappedName); this.value = value; this.description = description; } @@ -160,8 +163,8 @@ public final class EnumerationBuilderImpl extends AbstractBaseType implements En public int hashCode() { final int prime = 31; int result = 1; - result = (prime * result) + Objects.hashCode(this.name); - result = (prime * result) + Objects.hashCode(this.value); + result = prime * result + Objects.hashCode(this.name); + result = prime * result + Objects.hashCode(this.value); return result; } @@ -281,7 +284,7 @@ public final class EnumerationBuilderImpl extends AbstractBaseType implements En builder.append(" ("); builder.append(valPair.getValue()); - if (i == (this.values.size() - 1)) { + if (i == this.values.size() - 1) { builder.append(" );"); } else { builder.append(" ),"); diff --git a/binding/mdsal-binding-generator-util/src/test/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImplTest.java b/binding/mdsal-binding-generator-util/src/test/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImplTest.java index 89f1514795..648223ace5 100644 --- a/binding/mdsal-binding-generator-util/src/test/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImplTest.java +++ b/binding/mdsal-binding-generator-util/src/test/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImplTest.java @@ -47,7 +47,7 @@ public class EnumerationBuilderImplTest { enumerationBuilder.setModuleName(moduleName); enumerationBuilder.setReference(reference); enumerationBuilder.setSchemaPath(Collections.singletonList(qName)); - enumerationBuilder.addValue(valueName, value, valueDescription); + enumerationBuilder.addValue(valueName, valueName, value, valueDescription); enumerationBuilder.addAnnotation(packageName, "TestAnnotation"); enumerationBuilderSame = new EnumerationBuilderImpl(packageName, name); enumerationBuilderOtherName = new EnumerationBuilderImpl(packageName, "SomeOtherName"); @@ -107,13 +107,13 @@ public class EnumerationBuilderImplTest { final Enumeration enumerationOtherName = enumerationBuilderOtherName.toInstance(enumerationBuilderOtherName); assertNotEquals(enumeration, enumerationOtherName); - enumerationBuilderSame.addValue(valueName, value, valueDescription); + enumerationBuilderSame.addValue(valueName, valueName, value, valueDescription); final Enumeration enumerationSame = enumerationBuilderSame.toInstance(enumerationBuilderSame); assertEquals(enumeration, enumerationSame); final EnumerationBuilderImpl enumerationBuilderSame1 = new EnumerationBuilderImpl(packageName, name); final Enumeration enumerationSame1 = enumerationBuilderSame1.toInstance(enumerationBuilderSame1); - enumerationBuilderSame1.addValue(valueName, 14, valueDescription); + enumerationBuilderSame1.addValue(valueName, valueName, 14, valueDescription); // Enums are equal thanks to same package name and local name assertEquals(enumeration, enumerationSame1); } diff --git a/binding/mdsal-binding-test-model/src/main/yang/opendaylight-mdsal309.yang b/binding/mdsal-binding-test-model/src/main/yang/opendaylight-mdsal309.yang new file mode 100644 index 0000000000..bb65485046 --- /dev/null +++ b/binding/mdsal-binding-test-model/src/main/yang/opendaylight-mdsal309.yang @@ -0,0 +1,62 @@ +module opendaylight-mdsal309 { + namespace "urn:test:opendaylight-mdsal309"; + prefix mdsal309; + + description "Test model for enumeration name mapping hell."; + + leaf simple { + type enumeration { + enum "simple"; + enum "a-z"; + enum "ľaľaho papľuhu"; + } + } + + leaf classConflictDash { + type enumeration { + enum "a-z"; + enum "AZ"; + } + } + + leaf classConflictCaps { + type enumeration { + enum "a-z"; + enum "A-z"; + } + } + + leaf specialCharAsterisk { + type enumeration { + enum "*"; + } + } + + leaf specialCharDot { + type enumeration { + enum "."; + } + } + + leaf specialCharQuestionMark { + type enumeration { + enum "?"; + } + } + + leaf specialCharSpace { + type enumeration { + enum " "; + } + } + + leaf simpleWithSpecial { + type enumeration { + enum "a-z"; + enum "ľaľaho"; + enum "papľuhu"; + enum "*"; + } + } +} + diff --git a/binding/yang-binding/src/main/java/org/opendaylight/yangtools/yang/binding/BindingMapping.java b/binding/yang-binding/src/main/java/org/opendaylight/yangtools/yang/binding/BindingMapping.java index 72f63cc060..42a635e922 100644 --- a/binding/yang-binding/src/main/java/org/opendaylight/yangtools/yang/binding/BindingMapping.java +++ b/binding/yang-binding/src/main/java/org/opendaylight/yangtools/yang/binding/BindingMapping.java @@ -8,12 +8,17 @@ package org.opendaylight.yangtools.yang.binding; import static com.google.common.base.Preconditions.checkArgument; + import com.google.common.base.CharMatcher; import com.google.common.base.Splitter; +import com.google.common.collect.BiMap; +import com.google.common.collect.HashBiMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Interner; import com.google.common.collect.Interners; import java.text.SimpleDateFormat; +import java.util.Collection; +import java.util.Locale; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -24,12 +29,19 @@ public final class BindingMapping { public static final String VERSION = "0.6"; - public static final Set JAVA_RESERVED_WORDS = ImmutableSet.of("abstract", "assert", "boolean", "break", - "byte", "case", "catch", "char", "class", "const", "continue", "default", "double", "do", "else", "enum", - "extends", "false", "final", "finally", "float", "for", "goto", "if", "implements", "import", "instanceof", - "int", "interface", "long", "native", "new", "null", "package", "private", "protected", "public", "return", - "short", "static", "strictfp", "super", "switch", "synchronized", "this", "throw", "throws", "transient", - "true", "try", "void", "volatile", "while"); + public static final Set JAVA_RESERVED_WORDS = ImmutableSet.of( + // https://docs.oracle.com/javase/specs/jls/se9/html/jls-3.html#jls-3.9 + "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", "class", "const", "continue", + "default", "do", "double", "else", "enum", "extends", "final", "finally", "float", "for", "goto", "if", + "implements", "import", "instanceof", "int", "interface", "long", "native", "new", "package", "private", + "protected", "public", "return", "short", "static", "strictfp", "super", "switch", "synchronized", "this", + "throw", "throws", "transient", "try", "void", "volatile", "while", + // FIXME: _ is excluded to retain compatibility with previous releases + // "_", + // https://docs.oracle.com/javase/specs/jls/se9/html/jls-3.html#jls-3.10.3 + "false", "true", + // https://docs.oracle.com/javase/specs/jls/se9/html/jls-3.html#jls-3.10.7 + "null"); public static final String DATA_ROOT_SUFFIX = "Data"; public static final String RPC_SERVICE_SUFFIX = "Service"; @@ -243,4 +255,77 @@ public final class BindingMapping { } return s.substring(0, 1).toLowerCase() + s.substring(1); } + + /** + * Returns Java identifiers, conforming to JLS8 Section 3.8 to use for specified YANG assigned names + * (RFC7950 Section 9.6.4). This method considers two distinct encodings: one the pre-Fluorine mapping, which is + * okay and convenient for sane strings, and an escaping-based bijective mapping which works for all possible + * Unicode strings. + * + * @param assignedNames Collection of assigned names + * @return A BiMap keyed by assigned name, with Java identifiers as values + * @throws NullPointerException if assignedNames is null or contains null items + * @throws IllegalArgumentException if any of the names is empty + */ + public static BiMap mapEnumAssignedNames(final Collection assignedNames) { + /* + * Original mapping assumed strings encountered are identifiers, hence it used getClassName to map the names + * and that function is not an injection -- this is evidenced in MDSAL-208 and results in a failure to compile + * generated code. If we encounter such a conflict or if the result is not a valid identifier (like '*'), we + * abort and switch the mapping schema to mapEnumAssignedName(), which is a bijection. + * + * Note that assignedNames can contain duplicates, which must not trigger a duplication fallback. + */ + final BiMap javaToYang = HashBiMap.create(assignedNames.size()); + boolean valid = true; + for (String name : assignedNames) { + checkArgument(!name.isEmpty()); + if (!javaToYang.containsValue(name)) { + final String mappedName = getClassName(name); + if (!isValidJavaIdentifier(mappedName) || javaToYang.forcePut(mappedName, name) != null) { + valid = false; + break; + } + } + } + + if (!valid) { + // Fall back to bijective mapping + javaToYang.clear(); + for (String name : assignedNames) { + javaToYang.put(mapEnumAssignedName(name), name); + } + } + + return javaToYang.inverse(); + } + + // See https://docs.oracle.com/javase/specs/jls/se9/html/jls-3.html#jls-3.8 + private static boolean isValidJavaIdentifier(final String str) { + return !str.isEmpty() && !JAVA_RESERVED_WORDS.contains(str) + && Character.isJavaIdentifierStart(str.codePointAt(0)) + && str.codePoints().skip(1).allMatch(Character::isJavaIdentifierPart); + } + + private static String mapEnumAssignedName(final String assignedName) { + checkArgument(!assignedName.isEmpty()); + + // Mapping rules: + // - if the string is a valid java identifier and does not contain '$', use it as-is + if (assignedName.indexOf('$') == -1 && isValidJavaIdentifier(assignedName)) { + return assignedName; + } + + // - otherwise prefix it with '$' and replace any invalid character (including '$') with '$XX$', where XX is + // hex-encoded unicode codepoint (including plane, stripping leading zeroes) + final StringBuilder sb = new StringBuilder().append('$'); + assignedName.codePoints().forEachOrdered(codePoint -> { + if (codePoint == '$' || !Character.isJavaIdentifierPart(codePoint)) { + sb.append('$').append(Integer.toHexString(codePoint).toUpperCase(Locale.ROOT)).append('$'); + } else { + sb.appendCodePoint(codePoint); + } + }); + return sb.toString(); + } } diff --git a/binding/yang-binding/src/test/java/org/opendaylight/yangtools/yang/binding/BindingMappingTest.java b/binding/yang-binding/src/test/java/org/opendaylight/yangtools/yang/binding/BindingMappingTest.java index 0b91deef0d..7ec20f8cda 100644 --- a/binding/yang-binding/src/test/java/org/opendaylight/yangtools/yang/binding/BindingMappingTest.java +++ b/binding/yang-binding/src/test/java/org/opendaylight/yangtools/yang/binding/BindingMappingTest.java @@ -7,6 +7,7 @@ */ package org.opendaylight.yangtools.yang.binding; +import static com.google.common.collect.ImmutableList.of; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; @@ -16,6 +17,9 @@ import static org.junit.Assert.fail; import java.lang.reflect.Constructor; import java.net.URI; import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import org.junit.Test; import org.opendaylight.yangtools.yang.common.QName; import org.opendaylight.yangtools.yang.common.QNameModule; @@ -56,4 +60,54 @@ public class BindingMappingTest { throw e.getCause(); } } -} \ No newline at end of file + + @Test + public void mapEnumAssignedNamesTest() { + // Okay identifier + assertEqualMapping("__", "__"); + assertEqualMapping("True", "true"); + assertEqualMapping("ĽaľahoPapľuhu", "ľaľaho papľuhu"); + + // Contains a '$', but that's okay + assertEqualMapping("$", "$"); + assertEqualMapping("$abc", "$abc"); + assertEqualMapping("A$bc", "a$bc"); + assertEqualMapping("Ab$c", "ab$c"); + assertEqualMapping("Abc$", "abc$"); + + // Mostly okay, but numbers need to be prefixed + assertEqualMapping(of("AZ", "_09"), of("a-z", "0-9")); + + // Invalid identifier (conflicts with a Java 9 keyword) + assertEqualMapping("_", "_"); + + // Invalid characters, fall back to bijection + assertEqualMapping("$$2A$", "*"); + assertEqualMapping("$$2E$", "."); + assertEqualMapping("$$2F$", "/"); + assertEqualMapping("$$3F$", "?"); + assertEqualMapping("$a$2A$a", "a*a"); + + // Conflict, fallback to bijection + assertEqualMapping(of("_09", "$0$2D$9"), of("_09", "0-9")); + assertEqualMapping(of("$09", "$0$2D$9"), of("09", "0-9")); + assertEqualMapping(of("aZ", "$a$2D$z"), of("aZ", "a-z")); + assertEqualMapping(of("$a2$2E$5", "a25"), of("a2.5", "a25")); + assertEqualMapping(of("$a2$2E$5", "$a2$2D$5"), of("a2.5", "a2-5")); + assertEqualMapping(of("$ľaľaho$20$papľuhu", "$ľaľaho$20$$20$papľuhu"), of("ľaľaho papľuhu", "ľaľaho papľuhu")); + } + + private static void assertEqualMapping(final String mapped, final String yang) { + assertEqualMapping(of(mapped), of(yang)); + } + + private static void assertEqualMapping(final List mapped, final List yang) { + assertEquals(mapped.size(), yang.size()); + final Map expected = new HashMap<>(); + for (int i = 0; i < mapped.size(); ++i) { + expected.put(yang.get(i), mapped.get(i)); + } + + assertEquals(expected, BindingMapping.mapEnumAssignedNames(yang)); + } +} -- 2.36.6