NamingContext.getNameIfPresent should not fail if name is missing

Also makes InterfaceChangeNotificationProducer notification translation
code more defensive.

The issue was revealed by HC2VPP-216 and HC2VPP-220.

Change-Id: I20792a51743ae621d86c1b9066d680bc2303ed82
Signed-off-by: Marek Gradzki <mgradzki@cisco.com>

3 files changed, 60 insertions, 6 deletions

diff --git a/v3po/v3po2vpp/src/main/java/io/fd/hc2vpp/v3po/notification/InterfaceChangeNotificationProducer.java b/v3po/v3po2vpp/src/main/java/io/fd/hc2vpp/v3po/notification/InterfaceChangeNotificationProducer.java
index ffab221ee..b4ea48cf7 100644
--- a/v3po/v3po2vpp/src/main/java/io/fd/hc2vpp/v3po/notification/InterfaceChangeNotificationProducer.java
+++ b/v3po/v3po2vpp/src/main/java/io/fd/hc2vpp/v3po/notification/InterfaceChangeNotificationProducer.java
@@ -80,7 +80,13 @@ public final class InterfaceChangeNotificationProducer implements ManagedNotific
                 swInterfaceSetFlagsNotification -> {
                     LOG.trace("Interface notification received: {}", swInterfaceSetFlagsNotification);
                     // TODO HONEYCOMB-166 this should be lazy
-                    collector.onNotification(transformNotification(swInterfaceSetFlagsNotification));
+                    try {
+                        collector.onNotification(transformNotification(swInterfaceSetFlagsNotification));
+                    } catch (Exception e) {
+                        // There is no need to propagate exception to jvpp rx thread in case of unexpected failures.
+                        // We can't do much about it, so lets log the exception.
+                        LOG.warn("Failed to process interface notification {}", swInterfaceSetFlagsNotification, e);
+                    }
                 }
         );
     }
diff --git a/vpp-common/vpp-translate-utils/src/main/java/io/fd/hc2vpp/common/translate/util/NamingContext.java b/vpp-common/vpp-translate-utils/src/main/java/io/fd/hc2vpp/common/translate/util/NamingContext.java
index 5e0eea71b..8965de616 100644
--- a/vpp-common/vpp-translate-utils/src/main/java/io/fd/hc2vpp/common/translate/util/NamingContext.java
+++ b/vpp-common/vpp-translate-utils/src/main/java/io/fd/hc2vpp/common/translate/util/NamingContext.java
@@ -22,7 +22,9 @@ import static com.google.common.base.Preconditions.checkState;
 import com.google.common.base.Optional;
 import io.fd.honeycomb.translate.MappingContext;
 import io.fd.honeycomb.translate.util.RWUtils;
+import java.util.List;
 import java.util.stream.Collector;
+import java.util.stream.Collectors;
 import javax.annotation.Nonnull;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.Contexts;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.NamingContextKey;
@@ -92,13 +94,21 @@ public final class NamingContext implements AutoCloseable {
     public synchronized Optional<String> getNameIfPresent(final int index,
                                                           @Nonnull final MappingContext mappingContext) {
         final Optional<Mappings> read = mappingContext.read(namingContextIid.child(Mappings.class));
+        if (!read.isPresent()) {
+            return Optional.absent();
+        }
 
-        return read.isPresent()
-                ? Optional.of(read.get().getMapping().stream()
+        final List<Mapping> mappings = read.get().getMapping().stream()
                 .filter(mapping -> mapping.getIndex().equals(index))
-                .collect(SINGLE_ITEM_COLLECTOR)
-                .getName())
-                : Optional.absent();
+                .collect(Collectors.toList());
+
+        if (mappings.size() > 1) {
+            throw new IllegalStateException("Multiple mappings defined with index=" + index + ": " + mappings);
+        } else if (mappings.size() == 1) {
+            return Optional.of(mappings.get(0).getName());
+        } else {
+            return Optional.absent();
+        }
     }
 
     /**
diff --git a/vpp-common/vpp-translate-utils/src/test/java/io/fd/hc2vpp/common/translate/util/NamingContextTest.java b/vpp-common/vpp-translate-utils/src/test/java/io/fd/hc2vpp/common/translate/util/NamingContextTest.java
index 4e66315e1..b9c0f2c0f 100644
--- a/vpp-common/vpp-translate-utils/src/test/java/io/fd/hc2vpp/common/translate/util/NamingContextTest.java
+++ b/vpp-common/vpp-translate-utils/src/test/java/io/fd/hc2vpp/common/translate/util/NamingContextTest.java
@@ -17,11 +17,14 @@
 package io.fd.hc2vpp.common.translate.util;
 
 import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.common.base.Optional;
+import com.google.common.collect.Lists;
 import io.fd.honeycomb.test.tools.HoneycombTestRunner;
 import io.fd.honeycomb.test.tools.annotations.InjectTestData;
 import io.fd.honeycomb.test.tools.annotations.InjectablesProcessor;
@@ -40,6 +43,7 @@ import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.cont
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.Contexts;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.NamingContextKey;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.naming.context.Mappings;
+import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.naming.context.MappingsBuilder;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.naming.context.mappings.Mapping;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.naming.context.mappings.MappingBuilder;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.naming.context.mappings.MappingKey;
@@ -104,6 +108,40 @@ public class NamingContextTest implements InjectablesProcessor {
                 .build());
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void getAndThrow() {
+        when(mappingContext.read(any())).thenReturn(Optional.absent());
+        namingContext.getIndex("non-existing", mappingContext);
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void getNameIfPresentFails() {
+        final Mapping mapping1 = mock(Mapping.class);
+        final Mapping mapping2 = mock(Mapping.class);
+        final Mappings mappings = new MappingsBuilder().setMapping(Lists.newArrayList(mapping1, mapping2)).build();
+        when(mappingContext.read(namingContextIid.child(Mappings.class))).thenReturn(Optional.of(mappings));
+
+        namingContext.getNameIfPresent(0, mappingContext);
+    }
+
+    @Test
+    public void getNameIfPresentReturnsAbsent() {
+        final Mapping mapping1 = new MappingBuilder().setIndex(1).setName(NAME_1).build();
+        final Mappings mappings = new MappingsBuilder().setMapping(Lists.newArrayList(mapping1)).build();
+        when(mappingContext.read(namingContextIid.child(Mappings.class))).thenReturn(Optional.of(mappings));
+
+        assertEquals(Optional.absent(), namingContext.getNameIfPresent(0, mappingContext));
+    }
+
+    @Test
+    public void getNameIfPresent() {
+        final Mapping mapping1 = new MappingBuilder().setIndex(1).setName(NAME_1).build();
+        final Mappings mappings = new MappingsBuilder().setMapping(Lists.newArrayList(mapping1)).build();
+        when(mappingContext.read(namingContextIid.child(Mappings.class))).thenReturn(Optional.of(mappings));
+
+        assertEquals(Optional.of(NAME_1), namingContext.getNameIfPresent(1, mappingContext));
+    }
+
     private Mapping filterForParent(final String parent) {
         return mappings.getMapping().stream()
                 .filter(mapping -> mapping.getName().equals(parent))