Add alternative enum assigned name mapping 17/69317/3
authorRobert Varga <robert.varga@pantheon.tech>
Fri, 9 Mar 2018 15:55:44 +0000 (16:55 +0100)
committerRobert Varga <robert.varga@pantheon.tech>
Sat, 10 Mar 2018 08:18:47 +0000 (09:18 +0100)
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 <robert.varga@pantheon.tech>
binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/EnumerationCodec.java
binding/mdsal-binding-generator-api/src/main/java/org/opendaylight/mdsal/binding/model/api/type/builder/EnumBuilder.java
binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/AbstractEnumerationBuilder.java
binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/CodegenEnumerationBuilder.java
binding/mdsal-binding-generator-util/src/main/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/RuntimeEnumerationBuilder.java
binding/mdsal-binding-generator-util/src/test/java/org/opendaylight/mdsal/binding/model/util/generated/type/builder/EnumerationBuilderImplTest.java
binding/mdsal-binding-test-model/src/main/yang/opendaylight-mdsal309.yang [new file with mode: 0644]
binding/yang-binding/src/main/java/org/opendaylight/yangtools/yang/binding/BindingMapping.java
binding/yang-binding/src/test/java/org/opendaylight/yangtools/yang/binding/BindingMappingTest.java

index 26a72e37e5511f10e83926c2ad529657fd107221..ebe9c7d5d5c399d2f294a3c6da231138fb61da54 100644 (file)
@@ -7,11 +7,15 @@
  */
 package org.opendaylight.mdsal.binding.dom.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.mdsal.binding.dom.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<String, Enum<?>> yangValueToBinding;
 
-    EnumerationCodec(final Class<? extends Enum<?>> enumeration, final Map<String, Enum<?>> schema) {
+    EnumerationCodec(final Class<? extends Enum<?>> enumeration, final ImmutableBiMap<String, Enum<?>> schema) {
         super(enumeration);
-        yangValueToBinding = ImmutableBiMap.copyOf(schema);
+        yangValueToBinding = requireNonNull(schema);
     }
 
     static Callable<EnumerationCodec> loader(final Class<?> returnType, final EnumTypeDefinition enumSchema) {
-        Preconditions.checkArgument(Enum.class.isAssignableFrom(returnType));
+        checkArgument(Enum.class.isAssignableFrom(returnType));
         @SuppressWarnings({ "rawtypes", "unchecked" })
         final Class<? extends Enum<?>> enumType = (Class) returnType;
         return () -> {
-            Map<String, Enum<?>> nameToValue = new HashMap<>();
+            final BiMap<String, String> identifierToYang = BindingMapping.mapEnumAssignedNames(
+                enumSchema.getValues().stream().map(EnumPair::getName).collect(Collectors.toList())).inverse();
+
+            final Builder<String, Enum<?>> 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<String, Enum<?>> 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
index 225f61aafe58567652c57b3e97a1996be88dd762..ef95d8cc6731af434d56a5ad1858240b2f0dd3e6 100644 (file)
@@ -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
index 395d8fbd9d546f5d861fa3b8a12ac61815cd8055..aacf856c254f9d855fbc2e76c2d7ae089aebfb46 100644 (file)
@@ -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;
@@ -50,9 +55,9 @@ public abstract class AbstractEnumerationBuilder extends AbstractBaseType implem
         return null;
     }
 
-    @Override
-    public final void addValue(final String name, final int value, final String description) {
-        values = LazyCollections.lazyAdd(values, createEnumPair(name, value, description));
+    @VisibleForTesting
+    final void addValue(final String name, final String mappedName, final int value, final String description) {
+        values = LazyCollections.lazyAdd(values, createEnumPair(name, mappedName, value, description));
     }
 
     public abstract void setReference(String reference);
@@ -61,7 +66,7 @@ public abstract class AbstractEnumerationBuilder extends AbstractBaseType implem
 
     public abstract void setSchemaPath(SchemaPath schemaPath);
 
-    abstract AbstractPair createEnumPair(String name, int value, String description);
+    abstract AbstractPair createEnumPair(String name, String mappedName, int value, String description);
 
     /*
      * (non-Javadoc)
@@ -84,12 +89,12 @@ public abstract class AbstractEnumerationBuilder extends AbstractBaseType implem
     @Override
     public final void updateEnumPairsFromEnumTypeDef(final EnumTypeDefinition enumTypeDef) {
         final List<EnumPair> enums = enumTypeDef.getValues();
-        if (enums != null) {
-            for (EnumPair enumPair : enums) {
-                if (enumPair != null) {
-                    addValue(enumPair.getName(), enumPair.getValue(), enumPair.getDescription().orElse(null));
-                }
-            }
+        final Map<String, String> 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().orElse(null));
         }
     }
 
@@ -98,9 +103,9 @@ public abstract class AbstractEnumerationBuilder extends AbstractBaseType implem
         private final String mappedName;
         private final int value;
 
-        AbstractPair(final String name, final int value) {
-            this.name = name;
-            this.mappedName = BindingMapping.getClassName(name);
+        AbstractPair(final String name, final String mappedName, final int value) {
+            this.name = requireNonNull(name);
+            this.mappedName = requireNonNull(mappedName);
             this.value = value;
         }
 
index dd30d6fbf50f62dea23b3320ee6f9e4332226cc1..5c00bd5c7c12028bb65757384f354747825f95b5 100644 (file)
@@ -52,15 +52,15 @@ public final class CodegenEnumerationBuilder extends AbstractEnumerationBuilder
     }
 
     @Override
-    EnumPair createEnumPair(final String name, final int value, final String description) {
-        return new EnumPair(name, value, description);
+    EnumPair createEnumPair(final String name, final String mappedName, final int value, final String description) {
+        return new EnumPair(name, mappedName, value, description);
     }
 
     private static final class EnumPair extends AbstractPair {
         private final String description;
 
-        EnumPair(final String name, final int value, final String description) {
-            super(name, value);
+        EnumPair(final String name, final String mappedName, final int value, final String description) {
+            super(name, mappedName, value);
             this.description = description;
         }
 
index d24761367cfe84615c728ce50869bb5834f8e493..0ead6c65a6e12492a9a5f1b417658ded77bf2b27 100644 (file)
@@ -46,13 +46,13 @@ public final class RuntimeEnumerationBuilder extends AbstractEnumerationBuilder
     }
 
     @Override
-    EnumPair createEnumPair(final String name, final int value, final String description) {
-        return new EnumPair(name, value);
+    EnumPair createEnumPair(final String name, final String mappedName, final int value, final String description) {
+        return new EnumPair(name, mappedName, value);
     }
 
     private static final class EnumPair extends AbstractPair {
-        EnumPair(final String name, final int value) {
-            super(name, value);
+        EnumPair(final String name, final String mappedName, final int value) {
+            super(name, mappedName, value);
         }
 
         @Override
index 85fc36b6229a2db6f5f4cbde54cd5fa22467d526..1357bb225f40b51fba0610039365ef06e3f1e213 100644 (file)
@@ -47,7 +47,7 @@ public class EnumerationBuilderImplTest {
         enumerationBuilder.setModuleName(moduleName);
         enumerationBuilder.setReference(reference);
         enumerationBuilder.setSchemaPath(SchemaPath.create(true, qName));
-        enumerationBuilder.addValue(valueName, value, valueDescription);
+        enumerationBuilder.addValue(valueName, valueName, value, valueDescription);
         enumerationBuilder.addAnnotation(packageName, "TestAnnotation");
         enumerationBuilderSame = new CodegenEnumerationBuilder(packageName, name);
         enumerationBuilderOtherName = new CodegenEnumerationBuilder(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 CodegenEnumerationBuilder enumerationBuilderSame1 = new CodegenEnumerationBuilder(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 (file)
index 0000000..bb65485
--- /dev/null
@@ -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 "*";
+        }
+    }
+}
+
index afd2ce91d79bd00485d43c27f2ae1333d603b30e..133ae4c1c5411aa8212989a1eb304f544e605d7c 100644 (file)
@@ -10,11 +10,14 @@ 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.Preconditions;
 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.util.Collection;
+import java.util.Locale;
 import java.util.Optional;
 import java.util.Set;
 import java.util.regex.Matcher;
@@ -27,12 +30,19 @@ public final class BindingMapping {
 
     public static final String VERSION = "0.6";
 
-    public static final Set<String> 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<String> 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";
@@ -110,7 +120,7 @@ public final class BindingMapping {
             // Revision is in format 2017-10-26, we want the output to be 171026, which is a matter of picking the
             // right characters.
             final String rev = optRev.get().toString();
-            Preconditions.checkArgument(rev.length() == 10, "Unsupported revision %s", rev);
+            checkArgument(rev.length() == 10, "Unsupported revision %s", rev);
             packageNameBuilder.append("rev");
             packageNameBuilder.append(rev.substring(2, 4)).append(rev.substring(5, 7)).append(rev.substring(8));
         } else {
@@ -305,4 +315,77 @@ public final class BindingMapping {
         }
         return str.substring(0, 1).toLowerCase() + str.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<String, String> mapEnumAssignedNames(final Collection<String> 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<String, String> 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();
+    }
 }
index 3d9c59e2363cdbb44b215727b8aefb96c9e8ae7e..89619593c5711467c0b81942742868f9b192c340 100644 (file)
@@ -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.lang.reflect.InvocationTargetException;
 import java.net.URI;
+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;
@@ -57,4 +61,40 @@ public class BindingMappingTest {
             throw e.getCause();
         }
     }
+
+    @Test
+    public void mapEnumAssignedNamesTest() {
+        // Okay identifier
+        assertEqualMapping(of("__"), of("__"));
+        assertEqualMapping(of("True"), of("true"));
+        assertEqualMapping(of("ĽaľahoPapľuhu"), of("ľaľaho papľuhu"));
+
+        // 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(of("_"), of("_"));
+
+        // Invalid characters, fall back to bijection
+        assertEqualMapping(of("$$2A$"), of("*"));
+        assertEqualMapping(of("$$2E$"), of("."));
+        assertEqualMapping(of("$$2F$"), of("/"));
+        assertEqualMapping(of("$$3F$"), of("?"));
+        assertEqualMapping(of("$a$2A$a"), of("a*a"));
+
+        // Conflict, fallback to bijection
+        assertEqualMapping(of("aZ", "$a$2D$z"), of("aZ", "a-z"));
+        assertEqualMapping(of("$a2$2E$5", "a25"), of("a2.5", "a25"));
+        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 List<String> mapped, final List<String> yang) {
+        assertEquals(mapped.size(), yang.size());
+        final Map<String, String> expected = new HashMap<>();
+        for (int i = 0; i < mapped.size(); ++i) {
+            expected.put(yang.get(i), mapped.get(i));
+        }
+
+        assertEquals(expected, BindingMapping.mapEnumAssignedNames(yang));
+    }
 }