HONEYCOMB-194 Increase data-impl coverage to 87%

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

4 files changed, 73 insertions, 48 deletions

diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/DataBroker.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/DataBroker.java
index eb19c9dd1..5246ca15b 100644
--- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/DataBroker.java
+++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/DataBroker.java
@@ -166,7 +166,7 @@ public class DataBroker implements DOMDataBroker, Closeable {
     }
 
     /**
-     * Transaction factory specific for Honeycomb's context pipeline (config: none, operational: read+write)
+     * Transaction factory specific for Honeycomb's context pipeline (config: none, operational: read+write.
      */
     private static class ContextPipelineTxFactory implements TransactionFactory {
         private final ModifiableDataManager operationalDataTree;
diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/PersistingDataTreeAdapter.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/PersistingDataTreeAdapter.java
index 459971436..356ca51c1 100644
--- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/PersistingDataTreeAdapter.java
+++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/PersistingDataTreeAdapter.java
@@ -19,6 +19,8 @@ package io.fd.honeycomb.data.impl;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.MoreObjects;
 import com.google.common.base.Optional;
 import io.fd.honeycomb.translate.util.JsonUtils;
 import java.io.IOException;
@@ -42,13 +44,12 @@ import org.slf4j.LoggerFactory;
  * Adapter for a DataTree that stores current state of data in backing DataTree on each successful commit.
  * Uses JSON format.
  */
-public class PersistingDataTreeAdapter implements DataTree, AutoCloseable {
+public class PersistingDataTreeAdapter implements DataTree {
 
     private static final Logger LOG = LoggerFactory.getLogger(PersistingDataTreeAdapter.class);
 
     private final DataTree delegateDependency;
-    private final SchemaService schemaServiceDependency;
-    private final Path path;
+    private final JsonPersister persister;
 
     /**
      * Create new Persisting DataTree adapter
@@ -61,30 +62,13 @@ public class PersistingDataTreeAdapter implements DataTree, AutoCloseable {
     public PersistingDataTreeAdapter(@Nonnull final DataTree delegate,
                                      @Nonnull final SchemaService schemaService,
                                      @Nonnull final Path persistPath) {
-        this.path = testPersistPath(checkNotNull(persistPath, "persistPath is null"));
-        this.delegateDependency = checkNotNull(delegate, "delegate is null");
-        this.schemaServiceDependency = checkNotNull(schemaService, "schemaService is null");
+        this(delegate, new JsonPersister(persistPath, schemaService));
     }
 
-    /**
-     * Test whether file at persistPath exists and is readable or create it along with its parent structure
-     */
-    private Path testPersistPath(final Path persistPath) {
-        try {
-            checkArgument(!Files.isDirectory(persistPath), "Path %s points to a directory", persistPath);
-            if(Files.exists(persistPath)) {
-                checkArgument(Files.isReadable(persistPath),
-                    "Provided path %s points to existing, but non-readable file", persistPath);
-                return persistPath;
-            }
-            Files.createDirectories(persistPath.getParent());
-            Files.write(persistPath, new byte[]{}, StandardOpenOption.CREATE);
-        } catch (IOException | UnsupportedOperationException e) {
-            LOG.warn("Provided path for persistence: {} is not usable", persistPath, e);
-            throw new IllegalArgumentException("Path " + persistPath + " cannot be used as ");
-        }
-
-        return persistPath;
+    public PersistingDataTreeAdapter(final DataTree delegate,
+                                     final JsonPersister persister) {
+        this.delegateDependency = checkNotNull(delegate, "delegate is null");
+        this.persister = persister;
     }
 
     @Override
@@ -103,27 +87,68 @@ public class PersistingDataTreeAdapter implements DataTree, AutoCloseable {
         delegateDependency.commit(dataTreeCandidate);
         LOG.debug("Delegate commit successful. Persisting data");
 
-        // TODO HONEYCOMB-163 doing full read and full write might not be the fastest way of persisting data here
+        // FIXME doing full read and full write might not be the fastest way of persisting data here
         final DataTreeSnapshot dataTreeSnapshot = delegateDependency.takeSnapshot();
 
-        // TODO HONEYCOMB-163 this can be handled in background by a dedicated thread + a limited blocking queue
-        // TODO HONEYCOMB-163 enable configurable granularity for persists. Maybe doing write on every modification is too much
+        // TODO this can be handled in background by a dedicated thread + a limited blocking queue
+        // TODO enable configurable granularity for persists. Maybe doing write on every modification is too much
         // and we could do bulk persist
-        persistCurrentData(dataTreeSnapshot.readNode(YangInstanceIdentifier.EMPTY));
+        persister.persistCurrentData(dataTreeSnapshot.readNode(YangInstanceIdentifier.EMPTY));
     }
 
-    private void persistCurrentData(final Optional<NormalizedNode<?, ?>> currentRoot) {
-        if (currentRoot.isPresent()) {
+    @VisibleForTesting
+    static class JsonPersister {
+
+        private final Path path;
+        private final SchemaService schemaServiceDependency;
+
+        JsonPersister(final Path persistPath, final SchemaService schemaService) {
+            this.path = testPersistPath(checkNotNull(persistPath, "persistPath is null"));
+            this.schemaServiceDependency = checkNotNull(schemaService, "schemaService is null");
+        }
+
+        void persistCurrentData(final Optional<NormalizedNode<?, ?>> currentRoot) {
+            if (currentRoot.isPresent()) {
+                try {
+                    LOG.trace("Persisting current data: {} into: {}", currentRoot.get(), path);
+                    JsonUtils.writeJsonRoot(currentRoot.get(), schemaServiceDependency.getGlobalContext(),
+                            Files.newOutputStream(path, StandardOpenOption.CREATE,
+                                    StandardOpenOption.TRUNCATE_EXISTING));
+                    LOG.trace("Data persisted successfully in {}", path);
+                } catch (IOException e) {
+                    throw new IllegalStateException("Unable to persist current data", e);
+                }
+            } else {
+                LOG.debug("Skipping persistence, since there's no data to persist");
+            }
+        }
+
+        /**
+         * Test whether file at persistPath exists and is readable or create it along with its parent structure.
+         */
+        private static Path testPersistPath(final Path persistPath) {
             try {
-                LOG.trace("Persisting current data: {} into: {}", currentRoot.get(), path);
-                JsonUtils.writeJsonRoot(currentRoot.get(), schemaServiceDependency.getGlobalContext(),
-                    Files.newOutputStream(path, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING));
-                LOG.trace("Data persisted successfully in {}", path);
-            } catch (IOException e) {
-                throw new IllegalStateException("Unable to persist current data", e);
+                checkArgument(!Files.isDirectory(persistPath), "Path %s points to a directory", persistPath);
+                if (Files.exists(persistPath)) {
+                    checkArgument(Files.isReadable(persistPath),
+                            "Provided path %s points to existing, but non-readable file", persistPath);
+                    return persistPath;
+                }
+                Files.createDirectories(persistPath.getParent());
+                Files.write(persistPath, new byte[]{}, StandardOpenOption.CREATE);
+            } catch (IOException | UnsupportedOperationException e) {
+                LOG.warn("Provided path for persistence: {} is not usable", persistPath, e);
+                throw new IllegalArgumentException("Path " + persistPath + " cannot be used as ");
             }
-        } else {
-            LOG.debug("Skipping persistence, since there's no data to persist");
+
+            return persistPath;
+        }
+
+        @Override
+        public String toString() {
+            return MoreObjects.toStringHelper(this)
+                    .add("path", path)
+                    .toString();
         }
     }
 
@@ -143,9 +168,4 @@ public class PersistingDataTreeAdapter implements DataTree, AutoCloseable {
         return delegateDependency.prepare(dataTreeModification);
     }
 
-    @Override
-    public void close() throws Exception {
-        LOG.trace("Closing {} for {}", getClass().getSimpleName(), path);
-        // NOOP
-    }
 }
diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadOnlyTransaction.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadOnlyTransaction.java
index 00d105e26..e21098a3b 100644
--- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadOnlyTransaction.java
+++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadOnlyTransaction.java
@@ -72,10 +72,10 @@ final class ReadOnlyTransaction implements DOMDataReadOnlyTransaction {
         checkState(!closed, "Transaction has been closed");
 
         if (store == LogicalDatastoreType.OPERATIONAL) {
-            checkArgument(operationalData != null, "{} reads not supported", store);
+            checkArgument(operationalData != null, "%s reads not supported", store);
             return operationalData.read(path);
         } else {
-            checkArgument(configSnapshot != null, "{} reads not supported", store);
+            checkArgument(configSnapshot != null, "%s reads not supported", store);
             return configSnapshot.read(path);
         }
     }
diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadableDataTreeDelegator.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadableDataTreeDelegator.java
index ae29fa08b..a7ad88c9f 100644
--- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadableDataTreeDelegator.java
+++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ReadableDataTreeDelegator.java
@@ -19,6 +19,7 @@ package io.fd.honeycomb.data.impl;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.Iterables.getOnlyElement;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
@@ -178,11 +179,14 @@ public final class ReadableDataTreeDelegator implements ReadableDataManager {
         }
     }
 
-    private static DataContainerChild<?, ?> wrapListIntoMixinNode(
+    @VisibleForTesting
+    static DataContainerChild<?, ?> wrapListIntoMixinNode(
             final Collection<NormalizedNode<?, ?>> normalizedRootElements, final ListSchemaNode listSchema) {
         if (listSchema.getKeyDefinition().isEmpty()) {
             final CollectionNodeBuilder<UnkeyedListEntryNode, UnkeyedListNode> listBuilder =
                     Builders.unkeyedListBuilder();
+            listBuilder.withNodeIdentifier(new YangInstanceIdentifier.NodeIdentifier(listSchema.getQName()));
+
             for (NormalizedNode<?, ?> normalizedRootElement : normalizedRootElements) {
                 listBuilder.withChild((UnkeyedListEntryNode) normalizedRootElement);
             }
@@ -192,6 +196,7 @@ public final class ReadableDataTreeDelegator implements ReadableDataManager {
                     listSchema.isUserOrdered()
                             ? Builders.orderedMapBuilder()
                             : Builders.mapBuilder();
+            listBuilder.withNodeIdentifier(new YangInstanceIdentifier.NodeIdentifier(listSchema.getQName()));
 
             for (NormalizedNode<?, ?> normalizedRootElement : normalizedRootElements) {
                 listBuilder.withChild((MapEntryNode) normalizedRootElement);