HONEYCOMB-289 - Type-aware support for DumpCacheManager

Standard cache key factory made type-aware
Added checking for type of returned data from cache

Change-Id: Ie4d31a9d2b0d25c4b2f4ea66be98060f449007b6
Signed-off-by: Jan Srnicek <jsrnicek@cisco.com>

5 files changed, 130 insertions, 60 deletions

diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/CacheKeyFactory.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/CacheKeyFactory.java
index 1b444ba3c..bf4659e89 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/CacheKeyFactory.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/CacheKeyFactory.java
@@ -27,5 +27,12 @@ public interface CacheKeyFactory {
     /**
      * Construct key accordingly to provided {@code InstanceIdentifier<?>}
      */
+    @Nonnull
     String createKey(@Nonnull final InstanceIdentifier<?> actualContextIdentifier);
+
+    /**
+     * Returns type of data, for which is this factory creating keys
+     */
+    @Nonnull
+    Class<?> getCachedDataType();
 }
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManager.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManager.java
index f1b265dec..adcd32e0c 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManager.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManager.java
@@ -17,6 +17,8 @@
 package io.fd.honeycomb.translate.util.read.cache;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.nonNull;
 
 import com.google.common.base.Optional;
 import io.fd.honeycomb.translate.ModificationCache;
@@ -41,11 +43,13 @@ public final class DumpCacheManager<T, U> {
     private final EntityDumpExecutor<T, U> dumpExecutor;
     private final EntityDumpPostProcessingFunction<T> postProcessor;
     private final CacheKeyFactory cacheKeyFactory;
+    private final Class<?> acceptOnly;
 
     private DumpCacheManager(DumpCacheManagerBuilder<T, U> builder) {
         this.dumpExecutor = builder.dumpExecutor;
         this.postProcessor = builder.postProcessingFunction;
         this.cacheKeyFactory = builder.cacheKeyFactory;
+        this.acceptOnly = builder.acceptOnly;
     }
 
     /**
@@ -79,6 +83,12 @@ public final class DumpCacheManager<T, U> {
             cache.put(entityKey, dump);
             return Optional.of(dump);
         } else {
+            // if specified, check whether data returned from cache can be used as result of this dump manager
+            // used as a secondary check if cache does not have any data of different type stored under the same key
+            checkState(acceptOnly.isInstance(dump),
+                    "This dump manager accepts only %s as data, but %s was returned from cache",
+                    acceptOnly, dump.getClass());
+
             LOG.debug("Cached instance of dump was found for KEY[{}]", entityKey);
             return Optional.of(dump);
         }
@@ -86,18 +96,14 @@ public final class DumpCacheManager<T, U> {
 
     public static final class DumpCacheManagerBuilder<T, U> {
 
-        private static final CacheKeyFactory DEFAULT_CACHE_KEY_FACTORY_INSTANCE = new IdentifierCacheKeyFactory();
-
         private EntityDumpExecutor<T, U> dumpExecutor;
         private EntityDumpPostProcessingFunction<T> postProcessingFunction;
         private CacheKeyFactory cacheKeyFactory;
+        private Class<?> acceptOnly;
 
         public DumpCacheManagerBuilder() {
             // for cases when user does not set specific post-processor
             postProcessingFunction = new NoopDumpPostProcessingFunction<T>();
-
-            //use no additional scopes version by default
-            cacheKeyFactory = DEFAULT_CACHE_KEY_FACTORY_INSTANCE;
         }
 
         public DumpCacheManagerBuilder<T, U> withExecutor(@Nonnull final EntityDumpExecutor<T, U> executor) {
@@ -111,17 +117,37 @@ public final class DumpCacheManager<T, U> {
             return this;
         }
 
+        /**
+         * Key providing unique(type-aware) keys.
+         */
         public DumpCacheManagerBuilder<T, U> withCacheKeyFactory(@Nonnull final CacheKeyFactory cacheKeyFactory) {
             this.cacheKeyFactory = cacheKeyFactory;
             return this;
         }
 
+        /**
+         * If modification returns object of different type that this, throw exception to prevent processing data
+         * of different type.
+         */
+        public DumpCacheManagerBuilder<T, U> acceptOnly(@Nonnull final Class<?> acceptOnly) {
+            this.acceptOnly = acceptOnly;
+            return this;
+        }
+
         public DumpCacheManager<T, U> build() {
             checkNotNull(dumpExecutor, "Dump executor cannot be null");
             checkNotNull(postProcessingFunction,
                     "Dump post-processor cannot be null cannot be null, default implementation is used when not set explicitly");
-            checkNotNull(cacheKeyFactory,
-                    "Cache key factory cannot be null, default non-extended implementation is used when not set explicitly");
+
+            if (acceptOnly != null) {
+                cacheKeyFactory = new TypeAwareIdentifierCacheKeyFactory(acceptOnly);
+            } else if (cacheKeyFactory != null) {
+                acceptOnly = cacheKeyFactory.getCachedDataType();
+            } else {
+                throw new IllegalStateException(
+                        "Invalid combination - either acceptOnly type must be defined[defined=" + nonNull(acceptOnly) +
+                                "], or type-aware cache key factory[defined=" + nonNull(cacheKeyFactory) + "]");
+            }
 
             return new DumpCacheManager<>(this);
         }
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/IdentifierCacheKeyFactory.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/TypeAwareIdentifierCacheKeyFactory.java
index 51f47e137..ba4e7e493 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/IdentifierCacheKeyFactory.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/cache/TypeAwareIdentifierCacheKeyFactory.java
@@ -32,31 +32,51 @@ import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 /**
  * Factory providing cache keys to easier switching between scopes of caching
  */
-public final class IdentifierCacheKeyFactory implements CacheKeyFactory {
+public final class TypeAwareIdentifierCacheKeyFactory implements CacheKeyFactory {
 
     private static final String KEY_PARTS_SEPARATOR = "|";
 
-    // should be Set<Class<? extends DataObject & Identificable<?>>>, but that's not possible for wildcards
+    // should be Set<Class<? extends DataObject & Identifiable<?>>>, but that's not possible for wildcards
     private final Set<Class<? extends DataObject>> additionalKeyTypes;
+    // factory must be aware of type of data, to prevent creating same key for same identifier but different data
+    private final Class<?> type;
 
     /**
      * Construct simple cache key factory
      */
-    public IdentifierCacheKeyFactory() {
-        this(Collections.emptySet());
+    public TypeAwareIdentifierCacheKeyFactory(@Nonnull final Class<?> type) {
+        this(type, Collections.emptySet());
     }
 
     /**
      * @param additionalKeyTypes Additional types from path of cached type, that are specifying scope
      */
-    public IdentifierCacheKeyFactory(@Nonnull final Set<Class<? extends DataObject>> additionalKeyTypes) {
+    public TypeAwareIdentifierCacheKeyFactory(@Nonnull final Class<?> type,
+                                              @Nonnull final Set<Class<? extends DataObject>> additionalKeyTypes) {
         // verify that all are non-null and identifiable
+        this.type = checkNotNull(type, "Type cannot be null");
         this.additionalKeyTypes = checkNotNull(additionalKeyTypes, "Additional key types can't be null").stream()
-                .map(IdentifierCacheKeyFactory::verifyNotNull)
-                .map(IdentifierCacheKeyFactory::verifyIsIdentifiable)
+                .map(TypeAwareIdentifierCacheKeyFactory::verifyNotNull)
+                .map(TypeAwareIdentifierCacheKeyFactory::verifyIsIdentifiable)
                 .collect(Collectors.toSet());
     }
 
+    private static String bindKeyString(IdentifiableItem identifiableItem) {
+        return String.format("%s[%s]", identifiableItem.getType().getTypeName(), identifiableItem.getKey());
+    }
+
+    private static Class<? extends DataObject> verifyNotNull(final Class<? extends DataObject> type) {
+        return checkNotNull(type, "Cannot use null as key");
+    }
+
+    /**
+     * Initial check if provided scope variables are identifiable aka. can be used to create unique cache key
+     */
+    private static Class<? extends DataObject> verifyIsIdentifiable(final Class<? extends DataObject> type) {
+        checkArgument(Identifiable.class.isAssignableFrom(type), "Type %s is not Identifiable", type);
+        return type;
+    }
+
     @Override
     public String createKey(@Nonnull final InstanceIdentifier<?> actualContextIdentifier) {
 
@@ -64,18 +84,25 @@ public final class IdentifierCacheKeyFactory implements CacheKeyFactory {
 
         // easiest case when only simple key is needed
         if (additionalKeyTypes.isEmpty()) {
-            return actualContextIdentifier.getTargetType().toString();
+            return String
+                    .join(KEY_PARTS_SEPARATOR, type.getTypeName(), actualContextIdentifier.getTargetType().toString());
         }
 
         checkArgument(isUniqueKeyConstructable(actualContextIdentifier),
                 "Unable to construct unique key, required key types : %s, provided paths : %s", additionalKeyTypes,
                 actualContextIdentifier.getPathArguments());
 
+        // joins unique key in form : type | additional keys | actual context
         return String
-                .join(KEY_PARTS_SEPARATOR, additionalKeys(actualContextIdentifier),
+                .join(KEY_PARTS_SEPARATOR, type.getTypeName(), additionalKeys(actualContextIdentifier),
                         actualContextIdentifier.getTargetType().toString());
     }
 
+    @Override
+    public Class<?> getCachedDataType() {
+        return type;
+    }
+
     /**
      * Verifies that all requested key parts have keys
      */
@@ -94,29 +121,12 @@ public final class IdentifierCacheKeyFactory implements CacheKeyFactory {
         return pathArgument instanceof IdentifiableItem;
     }
 
-
     private String additionalKeys(final InstanceIdentifier<?> actualContextIdentifier) {
         return StreamSupport.stream(actualContextIdentifier.getPathArguments().spliterator(), false)
                 .filter(this::isAdditionalScope)
                 .filter(this::isIdentifiable)
                 .map(IdentifiableItem.class::cast)
-                .map(IdentifierCacheKeyFactory::bindKeyString)
+                .map(TypeAwareIdentifierCacheKeyFactory::bindKeyString)
                 .collect(Collectors.joining(KEY_PARTS_SEPARATOR));
     }
-
-    private static String bindKeyString(IdentifiableItem identifiableItem) {
-        return String.format("%s[%s]", identifiableItem.getType().getTypeName(), identifiableItem.getKey());
-    }
-
-    private static Class<? extends DataObject> verifyNotNull(final Class<? extends DataObject> type) {
-        return checkNotNull(type, "Cannot use null as key");
-    }
-
-    /**
-     * Initial check if provided scope variables are identifiable aka. can be used to create unique cache key
-     */
-    private static Class<? extends DataObject> verifyIsIdentifiable(final Class<? extends DataObject> type) {
-        checkArgument(Identifiable.class.isAssignableFrom(type), "Type %s is not Identifiable", type);
-        return type;
-    }
 }
diff --git a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManagerTest.java b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManagerTest.java
index 3639439ce..74aef67d0 100644
--- a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManagerTest.java
+++ b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/DumpCacheManagerTest.java
@@ -18,6 +18,7 @@ package io.fd.honeycomb.translate.util.read.cache;
 
 import static io.fd.honeycomb.translate.util.read.cache.EntityDumpExecutor.NO_PARAMS;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.when;
 
 import com.google.common.base.Optional;
@@ -35,13 +36,9 @@ import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 
 public class DumpCacheManagerTest {
 
-    private interface DataObj extends DataObject {}
-
     @Mock
     private EntityDumpExecutor<IpDetailsReplyDump, Void> executor;
-
     private InstanceIdentifier<DataObj> identifier;
-
     private DumpCacheManager<IpDetailsReplyDump, Void> managerPositive;
     private DumpCacheManager<IpDetailsReplyDump, Void> managerPositiveWithPostProcessing;
     private DumpCacheManager<IpDetailsReplyDump, Void> managerNegative;
@@ -54,23 +51,26 @@ public class DumpCacheManagerTest {
         managerPositive =
                 new DumpCacheManager.DumpCacheManagerBuilder<IpDetailsReplyDump, Void>()
                         .withExecutor(executor)
+                        .acceptOnly(IpDetailsReplyDump.class)
                         .build();
 
         managerPositiveWithPostProcessing =
                 new DumpCacheManager.DumpCacheManagerBuilder<IpDetailsReplyDump, Void>()
                         .withExecutor(executor)
+                        .acceptOnly(IpDetailsReplyDump.class)
                         .withPostProcessingFunction(createPostProcessor())
                         .build();
 
         managerNegative =
                 new DumpCacheManager.DumpCacheManagerBuilder<IpDetailsReplyDump, Void>()
                         .withExecutor(executor)
+                        .acceptOnly(IpDetailsReplyDump.class)
                         .build();
 
         cache = new ModificationCache();
         identifier = InstanceIdentifier.create(DataObj.class);
         //manager uses this implementation by default, so it can be used to test behaviour
-        cacheKeyFactory = new IdentifierCacheKeyFactory();
+        cacheKeyFactory = new TypeAwareIdentifierCacheKeyFactory(IpDetailsReplyDump.class);
 
     }
 
@@ -131,6 +131,28 @@ public class DumpCacheManagerTest {
         assertEquals(7, optionalDump.get().ipDetails.get(0).swIfIndex);
     }
 
+    @Test
+    public void testSameKeyDifferentTypes() throws ReadFailedException {
+        final DumpCacheManager<String, Void> stringManager =
+                new DumpCacheManager.DumpCacheManagerBuilder<String, Void>()
+                        .withExecutor((InstanceIdentifier, Void) -> "value")
+                        .acceptOnly(String.class)
+                        .build();
+
+        final DumpCacheManager<Integer, Void> intManager = new DumpCacheManager.DumpCacheManagerBuilder<Integer, Void>()
+                .acceptOnly(Integer.class)
+                .withExecutor((InstanceIdentifier, Void) -> 3).build();
+
+        final Optional<String> stringDump = stringManager.getDump(identifier, cache, NO_PARAMS);
+        final Optional<Integer> integerDump = intManager.getDump(identifier, cache, NO_PARAMS);
+
+        assertTrue(stringDump.isPresent());
+        assertTrue(integerDump.isPresent());
+        assertEquals("value", stringDump.get());
+        assertEquals(3, integerDump.get().intValue());
+
+    }
+
     private EntityDumpPostProcessingFunction<IpDetailsReplyDump> createPostProcessor() {
         return ipDetailsReplyDump -> {
             IpDetailsReplyDump modified = new IpDetailsReplyDump();
@@ -146,6 +168,9 @@ public class DumpCacheManagerTest {
         };
     }
 
+    private interface DataObj extends DataObject {
+    }
+
     private static final class IpDetailsReplyDump {
         List<IpDetails> ipDetails = new ArrayList<>();
 
diff --git a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/IdentifierCacheKeyFactoryTest.java b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/TypeAwareIdentifierCacheKeyFactoryTest.java
index e7eae552c..305fba143 100644
--- a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/IdentifierCacheKeyFactoryTest.java
+++ b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/read/cache/TypeAwareIdentifierCacheKeyFactoryTest.java
@@ -28,31 +28,15 @@ import org.opendaylight.yangtools.yang.binding.Identifiable;
 import org.opendaylight.yangtools.yang.binding.Identifier;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 
-public class IdentifierCacheKeyFactoryTest {
-
-    private interface SuperDataObject extends DataObject {
-    }
-
-    private interface DataObjectParent extends DataObject, ChildOf<SuperDataObject>, Identifiable<DataObjectParentKey> {
-    }
-
-    private class DataObjectParentKey implements Identifier<DataObjectParent> {
-    }
-
-    private interface DataObjectChild extends DataObject, ChildOf<DataObjectParent>, Identifiable<DataObjectChildKey> {
-    }
-
-    private class DataObjectChildKey implements Identifier<DataObjectChild> {
-    }
+public class TypeAwareIdentifierCacheKeyFactoryTest {
 
     private DataObjectParentKey parentKey;
     private DataObjectChildKey childKey;
     private InstanceIdentifier<DataObjectChild> identifierBothKeyed;
     private InstanceIdentifier<DataObjectChild> identifierOneMissing;
     private InstanceIdentifier<DataObjectChild> identifierNoneKeyed;
-
-    private IdentifierCacheKeyFactory simpleKeyFactory;
-    private IdentifierCacheKeyFactory complexKeyFactory;
+    private TypeAwareIdentifierCacheKeyFactory simpleKeyFactory;
+    private TypeAwareIdentifierCacheKeyFactory complexKeyFactory;
 
     @Before
     public void init() {
@@ -64,8 +48,9 @@ public class IdentifierCacheKeyFactoryTest {
         identifierNoneKeyed = InstanceIdentifier.create(SuperDataObject.class).child(DataObjectParent.class)
                 .child(DataObjectChild.class);
 
-        complexKeyFactory = new IdentifierCacheKeyFactory(ImmutableSet.of(DataObjectParent.class));
-        simpleKeyFactory = new IdentifierCacheKeyFactory();
+        complexKeyFactory =
+                new TypeAwareIdentifierCacheKeyFactory(String.class, ImmutableSet.of(DataObjectParent.class));
+        simpleKeyFactory = new TypeAwareIdentifierCacheKeyFactory(String.class);
     }
 
     @Test
@@ -127,6 +112,7 @@ public class IdentifierCacheKeyFactoryTest {
     }
 
     private void verifyComplexKey(final String key) {
+        assertTrue(key.contains(String.class.getTypeName()));
         assertTrue(key.contains(DataObjectParent.class.getTypeName()));
         assertTrue(key.contains(parentKey.toString()));
         assertTrue(key.contains(DataObjectChild.class.getTypeName()));
@@ -135,10 +121,26 @@ public class IdentifierCacheKeyFactoryTest {
     }
 
     private void verifySimpleKey(final String key) {
+        assertTrue(key.contains(String.class.getTypeName()));
         assertFalse(key.contains(DataObjectParent.class.getTypeName()));
         assertFalse(key.contains(parentKey.toString()));
         assertTrue(key.contains(DataObjectChild.class.getTypeName()));
         assertFalse(key.contains(childKey.toString()));
         assertFalse(key.contains(SuperDataObject.class.getTypeName()));
     }
+
+    private interface SuperDataObject extends DataObject {
+    }
+
+    private interface DataObjectParent extends DataObject, ChildOf<SuperDataObject>, Identifiable<DataObjectParentKey> {
+    }
+
+    private interface DataObjectChild extends DataObject, ChildOf<DataObjectParent>, Identifiable<DataObjectChildKey> {
+    }
+
+    private class DataObjectParentKey implements Identifier<DataObjectParent> {
+    }
+
+    private class DataObjectChildKey implements Identifier<DataObjectChild> {
+    }
 }
\ No newline at end of file