Do not support unions with complex types 69/100569/2
authorRobert Varga <robert.varga@pantheon.tech>
Wed, 13 Apr 2022 12:12:03 +0000 (14:12 +0200)
committerRobert Varga <nite@hq.sk>
Thu, 14 Apr 2022 07:47:50 +0000 (07:47 +0000)
Our falling back to string representation is actively hurtful, as we end
up admitting the wrong type. We really should be treating union as a
complex type and not handle it at all, but that may break users more
than we want to in one release.

JIRA: YANGTOOLS-1427
Change-Id: I944e5869a718a768799bf334e4291cd0eb861e98
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BitsStringCodec.java
data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BooleanStringCodec.java
data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/DecimalStringCodec.java
data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/EmptyStringCodec.java
data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/EnumStringCodec.java
data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/TypeDefinitionAwareCodec.java
data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/UnionStringCodec.java

index 509110cb84e5eb595d3f70e33f19cde126a69006..d35f4b8a2dedf2f3d47d43d5c4f83b48a4b8fe0c 100644 (file)
@@ -18,7 +18,6 @@ import com.google.common.collect.ImmutableSet;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
-import org.eclipse.jdt.annotation.NonNull;
 import org.opendaylight.yangtools.yang.data.api.codec.BitsCodec;
 import org.opendaylight.yangtools.yang.model.api.type.BitsTypeDefinition;
 import org.opendaylight.yangtools.yang.model.api.type.BitsTypeDefinition.Bit;
@@ -36,13 +35,13 @@ public final class BitsStringCodec extends TypeDefinitionAwareCodec<Set<String>,
     private final ImmutableSet<String> validBits;
 
     @SuppressWarnings("unchecked")
-    private BitsStringCodec(final @NonNull BitsTypeDefinition typeDef) {
-        super(typeDef, (Class<Set<String>>) (Class<?>) Set.class);
+    private BitsStringCodec(final BitsTypeDefinition typeDef) {
+        super(requireNonNull(typeDef), (Class<Set<String>>) (Class<?>) Set.class);
         validBits = ImmutableSet.copyOf(Collections2.transform(typeDef.getBits(), Bit::getName));
     }
 
     public static BitsStringCodec from(final BitsTypeDefinition type) {
-        return new BitsStringCodec(requireNonNull(type));
+        return new BitsStringCodec(type);
     }
 
     @Override
index 643b4a8413821d791e43ffc94d6c8f611b98c245..068297015cc03003640bd039cee93b576a37db46 100644 (file)
@@ -20,12 +20,12 @@ import org.opendaylight.yangtools.yang.model.api.type.BooleanTypeDefinition;
 @Beta
 public final class BooleanStringCodec extends TypeDefinitionAwareCodec<Boolean, BooleanTypeDefinition>
         implements BooleanCodec<String> {
-    private BooleanStringCodec(final @NonNull BooleanTypeDefinition typeDef) {
-        super(typeDef, Boolean.class);
+    private BooleanStringCodec(final BooleanTypeDefinition typeDef) {
+        super(requireNonNull(typeDef), Boolean.class);
     }
 
-    public static @NonNull BooleanStringCodec from(final BooleanTypeDefinition normalizedType) {
-        return new BooleanStringCodec(requireNonNull(normalizedType));
+    public static @NonNull BooleanStringCodec from(final BooleanTypeDefinition typeDef) {
+        return new BooleanStringCodec(typeDef);
     }
 
     @Override
index 35a5c87f8ddbd2fa3503c811cc2f599474a78019..c3724ac142eb7dbeb38612c01629e0d0728c8011 100644 (file)
@@ -22,11 +22,11 @@ import org.opendaylight.yangtools.yang.model.api.type.DecimalTypeDefinition;
 public final class DecimalStringCodec extends TypeDefinitionAwareCodec<Decimal64, DecimalTypeDefinition>
         implements DecimalCodec<String> {
     private DecimalStringCodec(final DecimalTypeDefinition typeDef) {
-        super(typeDef, Decimal64.class);
+        super(requireNonNull(typeDef), Decimal64.class);
     }
 
     public static @NonNull DecimalStringCodec from(final DecimalTypeDefinition type) {
-        return new DecimalStringCodec(requireNonNull(type));
+        return new DecimalStringCodec(type);
     }
 
     @Override
index 62655f9c6f88137567e5446174201d42efbacb72..3522b2e3b619a67df8ff901f3e2d349329daa6ef 100644 (file)
@@ -9,7 +9,6 @@ package org.opendaylight.yangtools.yang.data.impl.codec;
 
 import static com.google.common.base.Preconditions.checkArgument;
 
-import org.eclipse.jdt.annotation.NonNull;
 import org.opendaylight.yangtools.yang.common.Empty;
 import org.opendaylight.yangtools.yang.data.api.codec.EmptyCodec;
 import org.opendaylight.yangtools.yang.model.api.type.EmptyTypeDefinition;
@@ -29,7 +28,7 @@ final class EmptyStringCodec extends TypeDefinitionAwareCodec<Empty, EmptyTypeDe
     }
 
     @Override
-    protected @NonNull String serializeImpl(final Empty input) {
+    protected String serializeImpl(final Empty input) {
         return "";
     }
 }
\ No newline at end of file
index 4f05d2852936ce459266966aacfa75047aca26ee..b811558228f5decf6f3920be7d1f6773efe67e36 100644 (file)
@@ -13,7 +13,6 @@ import static java.util.Objects.requireNonNull;
 import com.google.common.annotations.Beta;
 import com.google.common.base.Functions;
 import com.google.common.collect.ImmutableMap;
-import org.eclipse.jdt.annotation.NonNull;
 import org.opendaylight.yangtools.yang.data.api.codec.EnumCodec;
 import org.opendaylight.yangtools.yang.model.api.type.EnumTypeDefinition;
 
@@ -25,16 +24,16 @@ public final class EnumStringCodec extends TypeDefinitionAwareCodec<String, Enum
         implements EnumCodec<String> {
     private final ImmutableMap<String, String> values;
 
-    private EnumStringCodec(final @NonNull EnumTypeDefinition typeDef) {
-        super(typeDef, String.class);
+    private EnumStringCodec(final EnumTypeDefinition typeDef) {
+        super(requireNonNull(typeDef), String.class);
         values = typeDef.getValues().stream()
                 // Intern the String to get wide reuse
                 .map(pair -> pair.getName().intern())
                 .collect(ImmutableMap.toImmutableMap(Functions.identity(), Functions.identity()));
     }
 
-    public static EnumStringCodec from(final EnumTypeDefinition normalizedType) {
-        return new EnumStringCodec(requireNonNull(normalizedType));
+    public static EnumStringCodec from(final EnumTypeDefinition typeDef) {
+        return new EnumStringCodec(typeDef);
     }
 
     @Override
index b563bff000df4d720b0586ed984715f7ddd97887..81f6d8f8e6f95665be0e5a14359d328d1985013e 100644 (file)
@@ -29,8 +29,18 @@ import org.opendaylight.yangtools.yang.model.api.type.Uint32TypeDefinition;
 import org.opendaylight.yangtools.yang.model.api.type.Uint64TypeDefinition;
 import org.opendaylight.yangtools.yang.model.api.type.Uint8TypeDefinition;
 import org.opendaylight.yangtools.yang.model.api.type.UnionTypeDefinition;
+import org.slf4j.LoggerFactory;
 
 public abstract class TypeDefinitionAwareCodec<J, T extends TypeDefinition<T>> extends AbstractDataStringCodec<J> {
+    private static final boolean ENABLE_UNION_CODEC =
+        !Boolean.getBoolean("org.opendaylight.yangtools.yang.data.impl.codec.disable-union");
+
+    static {
+        if (!ENABLE_UNION_CODEC) {
+            LoggerFactory.getLogger(TypeDefinitionAwareCodec.class).info("Support for unions is disabled");
+        }
+    }
+
     private final @NonNull Class<J> inputClass;
     private final @Nullable T typeDefinition;
 
@@ -45,6 +55,7 @@ public abstract class TypeDefinitionAwareCodec<J, T extends TypeDefinition<T>> e
         return inputClass;
     }
 
+    // FIXME: is this even useful?
     public Optional<T> getTypeDefinition() {
         return Optional.ofNullable(typeDefinition);
     }
@@ -78,8 +89,6 @@ public abstract class TypeDefinitionAwareCodec<J, T extends TypeDefinition<T>> e
             return AbstractIntegerStringCodec.from((Int64TypeDefinition) typeDefinition);
         } else if (typeDefinition instanceof StringTypeDefinition) {
             return StringStringCodec.from((StringTypeDefinition)typeDefinition);
-        } else if (typeDefinition instanceof UnionTypeDefinition) {
-            return UnionStringCodec.from((UnionTypeDefinition)typeDefinition);
         } else if (typeDefinition instanceof Uint8TypeDefinition) {
             return AbstractIntegerStringCodec.from((Uint8TypeDefinition) typeDefinition);
         } else if (typeDefinition instanceof Uint16TypeDefinition) {
@@ -88,6 +97,8 @@ public abstract class TypeDefinitionAwareCodec<J, T extends TypeDefinition<T>> e
             return AbstractIntegerStringCodec.from((Uint32TypeDefinition) typeDefinition);
         } else if (typeDefinition instanceof Uint64TypeDefinition) {
             return AbstractIntegerStringCodec.from((Uint64TypeDefinition) typeDefinition);
+        } else if (ENABLE_UNION_CODEC && typeDefinition instanceof UnionTypeDefinition) {
+            return UnionStringCodec.from((UnionTypeDefinition)typeDefinition);
         } else {
             return null;
         }
index 3053b1af96bf99d0b718692a873252501df0a455..d5a47c0e37437180bacf27375c3f8552354b5366 100644 (file)
@@ -9,9 +9,12 @@ package org.opendaylight.yangtools.yang.data.impl.codec;
 
 import static java.util.Objects.requireNonNull;
 
+import com.google.common.collect.ImmutableList;
+import java.util.ArrayList;
 import java.util.Base64;
+import java.util.List;
+import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.yangtools.yang.data.api.codec.UnionCodec;
-import org.opendaylight.yangtools.yang.model.api.TypeDefinition;
 import org.opendaylight.yangtools.yang.model.api.type.UnionTypeDefinition;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -20,36 +23,49 @@ final class UnionStringCodec extends TypeDefinitionAwareCodec<Object, UnionTypeD
         implements UnionCodec<String> {
     private static final Logger LOG = LoggerFactory.getLogger(UnionStringCodec.class);
 
-    UnionStringCodec(final UnionTypeDefinition typeDef) {
+    private final ImmutableList<TypeDefinitionAwareCodec<Object, ?>> codecs;
+
+    private UnionStringCodec(final UnionTypeDefinition typeDef,
+            final ImmutableList<TypeDefinitionAwareCodec<Object, ?>> codecs) {
         super(requireNonNull(typeDef), Object.class);
+        this.codecs = requireNonNull(codecs);
     }
 
-    static TypeDefinitionAwareCodec<?, UnionTypeDefinition> from(final UnionTypeDefinition normalizedType) {
-        return new UnionStringCodec(normalizedType);
+    static @Nullable TypeDefinitionAwareCodec<?, UnionTypeDefinition> from(final UnionTypeDefinition typeDef) {
+        final var types = typeDef.getTypes();
+        final var builder = ImmutableList.<TypeDefinitionAwareCodec<Object, ?>>builderWithExpectedSize(types.size());
+        for (var type : types) {
+            final var codec = from(type);
+            if (codec == null) {
+                LOG.debug("Cannot handle {} because of unhandled component {}", typeDef, type);
+                return null;
+            }
+            builder.add(codec);
+        }
+        return new UnionStringCodec(typeDef, builder.build());
     }
 
     @Override
-    @SuppressWarnings("checkstyle:illegalCatch")
     protected Object deserializeImpl(final String stringRepresentation) {
-        for (final TypeDefinition<?> type : getTypeDefinition().get().getTypes()) {
-            final TypeDefinitionAwareCodec<Object, ?> typeAwareCodec = from(type);
-            if (typeAwareCodec == null) {
-                /*
-                 * This is a type for which we have no codec (eg identity ref) so we'll say it's
-                 * valid
-                 */
-                return stringRepresentation;
-            }
-
+        List<IllegalArgumentException> suppressed = null;
+        for (var codec : codecs) {
             try {
-                return typeAwareCodec.deserialize(stringRepresentation);
-            } catch (final Exception e) {
-                LOG.debug("Value {} did not matched representation for {}",stringRepresentation,type,e);
+                return codec.deserialize(stringRepresentation);
+            } catch (final IllegalArgumentException e) {
                 // invalid - try the next union type.
+                LOG.debug("Value {} did not match codec {}", stringRepresentation, codec, e);
+                if (suppressed == null) {
+                    suppressed = new ArrayList<>();
+                }
+                suppressed.add(e);
             }
         }
 
-        throw new IllegalArgumentException("Invalid value \"" + stringRepresentation + "\" for union type.");
+        final var ex = new IllegalArgumentException("Invalid value \"" + stringRepresentation + "\" for union type.");
+        if (suppressed != null) {
+            suppressed.forEach(ex::addSuppressed);
+        }
+        throw ex;
     }
 
     @Override