From 93048afc8037fd8bb631150361b05e4ab482a623 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Wed, 13 Apr 2022 14:12:03 +0200 Subject: [PATCH] Do not support unions with complex types 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 --- .../yang/data/impl/codec/BitsStringCodec.java | 7 ++- .../data/impl/codec/BooleanStringCodec.java | 8 +-- .../data/impl/codec/DecimalStringCodec.java | 4 +- .../data/impl/codec/EmptyStringCodec.java | 3 +- .../yang/data/impl/codec/EnumStringCodec.java | 9 ++-- .../impl/codec/TypeDefinitionAwareCodec.java | 15 +++++- .../data/impl/codec/UnionStringCodec.java | 54 ++++++++++++------- 7 files changed, 62 insertions(+), 38 deletions(-) diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BitsStringCodec.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BitsStringCodec.java index 509110cb84..d35f4b8a2d 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BitsStringCodec.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BitsStringCodec.java @@ -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, private final ImmutableSet validBits; @SuppressWarnings("unchecked") - private BitsStringCodec(final @NonNull BitsTypeDefinition typeDef) { - super(typeDef, (Class>) (Class) Set.class); + private BitsStringCodec(final BitsTypeDefinition typeDef) { + super(requireNonNull(typeDef), (Class>) (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 diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BooleanStringCodec.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BooleanStringCodec.java index 643b4a8413..068297015c 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BooleanStringCodec.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/BooleanStringCodec.java @@ -20,12 +20,12 @@ import org.opendaylight.yangtools.yang.model.api.type.BooleanTypeDefinition; @Beta public final class BooleanStringCodec extends TypeDefinitionAwareCodec implements BooleanCodec { - 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 diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/DecimalStringCodec.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/DecimalStringCodec.java index 35a5c87f8d..c3724ac142 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/DecimalStringCodec.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/DecimalStringCodec.java @@ -22,11 +22,11 @@ import org.opendaylight.yangtools.yang.model.api.type.DecimalTypeDefinition; public final class DecimalStringCodec extends TypeDefinitionAwareCodec implements DecimalCodec { 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 diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/EmptyStringCodec.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/EmptyStringCodec.java index 62655f9c6f..3522b2e3b6 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/EmptyStringCodec.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/EmptyStringCodec.java @@ -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 { private final ImmutableMap 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 diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/TypeDefinitionAwareCodec.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/TypeDefinitionAwareCodec.java index b563bff000..81f6d8f8e6 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/TypeDefinitionAwareCodec.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/TypeDefinitionAwareCodec.java @@ -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> extends AbstractDataStringCodec { + 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 inputClass; private final @Nullable T typeDefinition; @@ -45,6 +55,7 @@ public abstract class TypeDefinitionAwareCodec> e return inputClass; } + // FIXME: is this even useful? public Optional getTypeDefinition() { return Optional.ofNullable(typeDefinition); } @@ -78,8 +89,6 @@ public abstract class TypeDefinitionAwareCodec> 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> 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; } diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/UnionStringCodec.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/UnionStringCodec.java index 3053b1af96..d5a47c0e37 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/UnionStringCodec.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/codec/UnionStringCodec.java @@ -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 { private static final Logger LOG = LoggerFactory.getLogger(UnionStringCodec.class); - UnionStringCodec(final UnionTypeDefinition typeDef) { + private final ImmutableList> codecs; + + private UnionStringCodec(final UnionTypeDefinition typeDef, + final ImmutableList> codecs) { super(requireNonNull(typeDef), Object.class); + this.codecs = requireNonNull(codecs); } - static TypeDefinitionAwareCodec from(final UnionTypeDefinition normalizedType) { - return new UnionStringCodec(normalizedType); + static @Nullable TypeDefinitionAwareCodec from(final UnionTypeDefinition typeDef) { + final var types = typeDef.getTypes(); + final var builder = ImmutableList.>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 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 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 -- 2.36.6