Fix IPv4 read caching

Single cache key was used for each interface during a single read
returing same IPs for all the interfaces

Change-Id: I8cc05591b257d44a253cc23c9d79d9096459dcdd
Signed-off-by: Maros Marsalek <mmarsale@cisco.com>

2 files changed, 82 insertions, 24 deletions

diff --git a/v3po/v3po2vpp/src/main/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizer.java b/v3po/v3po2vpp/src/main/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizer.java
index 641516136..2121337ab 100644
--- a/v3po/v3po2vpp/src/main/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizer.java
+++ b/v3po/v3po2vpp/src/main/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizer.java
@@ -50,7 +50,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * Customizer for read operations for {@link Address} of {@link Ipv4}
+ * Customizer for read operations for {@link Address} of {@link Ipv4}.
  */
 public class Ipv4AddressCustomizer extends FutureJVppCustomizer
     implements ListReaderCustomizer<Address, AddressKey, AddressBuilder> {
@@ -92,8 +92,7 @@ public class Ipv4AddressCustomizer extends FutureJVppCustomizer
             builder.setIp(TranslateUtils.arrayToIpv4AddressNoZone(detail.ip))
                 .setSubnet(new PrefixLengthBuilder()
                     .setPrefixLength(Short.valueOf(detail.prefixLength)).build());
-            LOG.info("Address read successfull");
-
+            LOG.info("Address read successful");
         } else {
             LOG.warn("No address dump present");
         }
@@ -102,18 +101,22 @@ public class Ipv4AddressCustomizer extends FutureJVppCustomizer
     @Override
     public List<AddressKey> getAllIds(@Nonnull InstanceIdentifier<Address> id, @Nonnull ReadContext context)
         throws ReadFailedException {
+        // FIXME: this kind of logs provide very little information. At least the ID should be included so we know
+        // from the logs what exact data is being processed
+        // + Logs should be consistent in using punctuation
         LOG.debug("Extracting keys..");
 
         Optional<IpAddressDetailsReplyDump> dumpOptional = dumpAddresses(id, context);
 
         if (dumpOptional.isPresent() && dumpOptional.get().ipAddressDetails != null) {
 
-            List<IpAddressDetails> details = dumpOptional.get().ipAddressDetails;
-
-            return details.stream()
+            return dumpOptional.get().ipAddressDetails.stream()
                 .map(detail -> new AddressKey(TranslateUtils.arrayToIpv4AddressNoZone(detail.ip)))
                 .collect(Collectors.toList());
         } else {
+            // FIXME if this is expected then WARN should not be emitted
+            // FIXME if this is not expected, throw an exception instead
+            // Same in readCurrentAttributes()
             LOG.warn("No dump present");
             return Collections.emptyList();
         }
@@ -124,11 +127,14 @@ public class Ipv4AddressCustomizer extends FutureJVppCustomizer
         ((Ipv4Builder) builder).setAddress(readData);
     }
 
+    // TODO refactor after there is a more generic implementation of cache operations
+    // FIXME update TODO with what exactly should be refactored and how
     // TODO refactor after there is an more generic implementation of cache
     // operations
     private Optional<IpAddressDetailsReplyDump> dumpAddresses(InstanceIdentifier<Address> id, ReadContext ctx)
-        throws ReadFailedException {
-        Optional<IpAddressDetailsReplyDump> dumpFromCache = dumpAddressFromCache(ctx.getModificationCache());
+            throws ReadFailedException {
+        final String cacheKey = CACHE_KEY + id.firstKeyOf(Interface.class).getName();
+        Optional<IpAddressDetailsReplyDump> dumpFromCache = dumpAddressFromCache(cacheKey, ctx.getModificationCache());
 
         if (dumpFromCache.isPresent()) {
             return dumpFromCache;
@@ -142,15 +148,16 @@ public class Ipv4AddressCustomizer extends FutureJVppCustomizer
         }
 
         if (dumpFromOperational.isPresent()) {
-            ctx.getModificationCache().put(CACHE_KEY, dumpFromOperational.get());
+            ctx.getModificationCache().put(cacheKey, dumpFromOperational.get());
         }
 
         return dumpFromOperational;
     }
 
-    private Optional<IpAddressDetailsReplyDump> dumpAddressFromCache(ModificationCache cache) {
+    private Optional<IpAddressDetailsReplyDump> dumpAddressFromCache(final String cacheKey,
+                                                                     ModificationCache cache) {
         LOG.debug("Dumping from cache...");
-        return Optional.fromNullable((IpAddressDetailsReplyDump) cache.get(CACHE_KEY));
+        return Optional.fromNullable((IpAddressDetailsReplyDump) cache.get(cacheKey));
     }
 
     private Optional<IpAddressDetailsReplyDump> dumpAddressFromOperationalData(final InstanceIdentifier<Address> id,
diff --git a/v3po/v3po2vpp/src/test/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizerTest.java b/v3po/v3po2vpp/src/test/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizerTest.java
index 85f528e1e..584464645 100644
--- a/v3po/v3po2vpp/src/test/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizerTest.java
+++ b/v3po/v3po2vpp/src/test/java/io/fd/honeycomb/v3po/translate/v3po/interfacesstate/ip/Ipv4AddressCustomizerTest.java
@@ -19,7 +19,9 @@ package io.fd.honeycomb.v3po.translate.v3po.interfacesstate.ip;
 import static io.fd.honeycomb.v3po.translate.v3po.test.ContextTestUtils.getMapping;
 import static io.fd.honeycomb.v3po.translate.v3po.test.ContextTestUtils.getMappingIid;
 import static io.fd.honeycomb.v3po.translate.v3po.util.TranslateUtils.reverseBytes;
+import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -38,6 +40,7 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 import java.util.stream.Collectors;
+import org.hamcrest.CoreMatchers;
 import org.junit.Test;
 import org.mockito.Mockito;
 import org.opendaylight.yang.gen.v1.urn.honeycomb.params.xml.ns.yang.naming.context.rev160513.contexts.naming.context.Mappings;
@@ -64,7 +67,9 @@ import org.openvpp.jvpp.dto.IpAddressDump;
 public class Ipv4AddressCustomizerTest extends ListReaderCustomizerTest<Address, AddressKey, AddressBuilder> {
 
     private static final String IFACE_NAME = "eth0";
+    private static final String IFACE_2_NAME = "eth1";
     private static final int IFACE_ID = 1;
+    private static final int IFACE_2_ID = 2;
 
     private NamingContext interfacesContext;
 
@@ -80,24 +85,27 @@ public class Ipv4AddressCustomizerTest extends ListReaderCustomizerTest<Address,
     @Override
     protected RootReaderCustomizer<Address, AddressBuilder> initCustomizer() {
         final KeyedInstanceIdentifier<Mapping, MappingKey> eth0Id = getMappingIid(IFACE_NAME, "test-instance");
+        final KeyedInstanceIdentifier<Mapping, MappingKey> eth1Id = getMappingIid(IFACE_2_NAME, "test-instance");
         final Optional<Mapping> eth0 = getMapping(IFACE_NAME, IFACE_ID);
+        final Optional<Mapping> eth1 = getMapping(IFACE_2_NAME, IFACE_2_ID);
 
-        final List<Mapping> allMappings = Lists.newArrayList(getMapping(IFACE_NAME, IFACE_ID).get());
+        final List<Mapping> allMappings = Lists.newArrayList(eth0.get(), eth1.get());
         final Mappings allMappingsBaObject = new MappingsBuilder().setMapping(allMappings).build();
         doReturn(Optional.of(allMappingsBaObject)).when(mappingContext).read(eth0Id.firstIdentifierOf(Mappings.class));
 
         doReturn(eth0).when(mappingContext).read(eth0Id);
+        doReturn(eth1).when(mappingContext).read(eth1Id);
 
         return new Ipv4AddressCustomizer(api, interfacesContext);
     }
 
-    private static InstanceIdentifier<Address> getId(final String address) {
+    private static InstanceIdentifier<Address> getId(final String address, final String ifaceName) {
         return InstanceIdentifier.builder(InterfacesState.class)
-            .child(Interface.class, new InterfaceKey(IFACE_NAME))
-            .augmentation(Interface2.class)
-            .child(Ipv4.class)
-            .child(Address.class, new AddressKey(new Ipv4AddressNoZone(new Ipv4Address(address))))
-            .build();
+                .child(Interface.class, new InterfaceKey(ifaceName))
+                .augmentation(Interface2.class)
+                .child(Ipv4.class)
+                .child(Address.class, new AddressKey(new Ipv4AddressNoZone(new Ipv4Address(address))))
+                .build();
     }
 
     @Test
@@ -118,11 +126,11 @@ public class Ipv4AddressCustomizerTest extends ListReaderCustomizerTest<Address,
         IpAddressDetailsReplyDump reply = new IpAddressDetailsReplyDump();
         reply.ipAddressDetails = ImmutableList.of(detail1, detail2, detail3);
 
-        cache.put(Ipv4AddressCustomizer.class.getName(), reply);
+        cache.put(Ipv4AddressCustomizer.class.getName() + IFACE_NAME, reply);
         when(ctx.getModificationCache()).thenReturn(cache);
 
         final AddressBuilder builder = new AddressBuilder();
-        final InstanceIdentifier<Address> id = getId("192.168.2.1");
+        final InstanceIdentifier<Address> id = getId("192.168.2.1", IFACE_NAME);
 
         getCustomizer().readCurrentAttributes(id, builder, ctx);
 
@@ -130,6 +138,49 @@ public class Ipv4AddressCustomizerTest extends ListReaderCustomizerTest<Address,
     }
 
     @Test
+    public void testReadCurrentAttributesFor2Ifcs() throws ReadFailedException {
+        ModificationCache cache = new ModificationCache();
+
+        IpAddressDetails detail1 = new IpAddressDetails();
+        IpAddressDetails detail2 = new IpAddressDetails();
+
+        detail1.ip = reverseBytes(
+                TranslateUtils.ipv4AddressNoZoneToArray(new Ipv4AddressNoZone(new Ipv4Address("192.168.2.1"))));
+        detail2.ip = reverseBytes(
+                TranslateUtils.ipv4AddressNoZoneToArray(new Ipv4AddressNoZone(new Ipv4Address("192.168.2.2"))));
+
+        IpAddressDetailsReplyDump reply = new IpAddressDetailsReplyDump();
+        reply.ipAddressDetails = ImmutableList.of(detail1);
+        IpAddressDetailsReplyDump reply2 = new IpAddressDetailsReplyDump();
+        reply2.ipAddressDetails = ImmutableList.of(detail2);
+
+        CompletableFuture<IpAddressDetailsReplyDump> future = new CompletableFuture<>();
+        future.complete(reply);
+        CompletableFuture<IpAddressDetailsReplyDump> future2 = new CompletableFuture<>();
+        future2.complete(reply2);
+
+        when(api.ipAddressDump(Mockito.any(IpAddressDump.class))).thenReturn(future).thenReturn(future2);
+        when(ctx.getModificationCache()).thenReturn(cache);
+
+        final InstanceIdentifier<Address> id = getId("192.168.2.1", IFACE_NAME);
+        final InstanceIdentifier<Address> id2 = getId("192.168.2.2", IFACE_2_NAME);
+
+        final List<AddressKey> ifc1Ids = getCustomizer().getAllIds(id, ctx);
+        assertThat(ifc1Ids.size(), is(1));
+        assertThat(ifc1Ids, CoreMatchers.hasItem(new AddressKey(new Ipv4AddressNoZone("192.168.2.1"))));
+        final List<AddressKey> ifc2Ids = getCustomizer().getAllIds(id2, ctx);
+        assertThat(ifc2Ids.size(), is(1));
+        assertThat(ifc2Ids, CoreMatchers.hasItem(new AddressKey(new Ipv4AddressNoZone("192.168.2.2"))));
+
+        AddressBuilder builder = new AddressBuilder();
+        getCustomizer().readCurrentAttributes(id, builder, ctx);
+        assertEquals(builder.getIp().getValue(), "192.168.2.1");
+        builder = new AddressBuilder();
+        getCustomizer().readCurrentAttributes(id2, builder, ctx);
+        assertEquals(builder.getIp().getValue(), "192.168.2.2");
+    }
+
+    @Test
     public void testReadCurrentAttributesFromOperationalData() throws ReadFailedException {
         ModificationCache cache = new ModificationCache();
 
@@ -155,7 +206,7 @@ public class Ipv4AddressCustomizerTest extends ListReaderCustomizerTest<Address,
 
 
         final AddressBuilder builder = new AddressBuilder();
-        final InstanceIdentifier<Address> id = getId("192.168.2.1");
+        final InstanceIdentifier<Address> id = getId("192.168.2.1", IFACE_NAME);
 
         getCustomizer().readCurrentAttributes(id, builder, ctx);
 
@@ -180,10 +231,10 @@ public class Ipv4AddressCustomizerTest extends ListReaderCustomizerTest<Address,
         IpAddressDetailsReplyDump reply = new IpAddressDetailsReplyDump();
         reply.ipAddressDetails = ImmutableList.of(detail1, detail2, detail3);
 
-        cache.put(Ipv4AddressCustomizer.class.getName(), reply);
+        cache.put(Ipv4AddressCustomizer.class.getName() + IFACE_NAME, reply);
         when(ctx.getModificationCache()).thenReturn(cache);
 
-        final InstanceIdentifier<Address> id = getId("192.168.2.1");
+        final InstanceIdentifier<Address> id = getId("192.168.2.1", IFACE_NAME);
 
         List<Ipv4AddressNoZone> ids = getCustomizer().getAllIds(id, ctx).stream()
             .map(key -> key.getIp())
@@ -220,7 +271,7 @@ public class Ipv4AddressCustomizerTest extends ListReaderCustomizerTest<Address,
         when(api.ipAddressDump(Mockito.any(IpAddressDump.class))).thenReturn(future);
         when(ctx.getModificationCache()).thenReturn(cache);
 
-        final InstanceIdentifier<Address> id = getId("192.168.2.1");
+        final InstanceIdentifier<Address> id = getId("192.168.2.1", IFACE_NAME);
 
         List<Ipv4AddressNoZone> ids = getCustomizer().getAllIds(id, ctx).stream()
             .map(key -> key.getIp())