HONEYCOMB-255 - Cutting identifiers to prevent failing of reverts

Mapping allready processes changes for reverting by InstanceIdentifier
instead of using KeyedInstanceIdentifier(to prevent failing to identify
handleable nodes)
Modified logging to prevent double/triple logging of detailed cause
of failed bulk update
Reusing WriteContext for revert(removed try with resource to prevent
closing of write context before revert)

Change-Id: Ie939ebe443629f9cdad5b5b449aa8c5dac40ea67
Signed-off-by: Jan Srnicek <jsrnicek@cisco.com>
Signed-off-by: Maros Marsalek <mmarsale@cisco.com>

4 files changed, 81 insertions, 22 deletions

diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java
index 0128ee4a8..dd397a605 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java
@@ -108,9 +108,6 @@ public final class TransactionWriteContext implements WriteContext {
         return mappingContext;
     }
 
-    /**
-     * Does not close the transactions.
-     */
     @Override
     public void close() {
         ctx.close();
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java
index ab80eb4ac..e5b829d6a 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java
@@ -17,6 +17,8 @@
 package io.fd.honeycomb.translate.util.write.registry;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static io.fd.honeycomb.translate.util.RWUtils.makeIidWildcarded;
 
 import com.google.common.base.Optional;
 import com.google.common.collect.HashMultimap;
@@ -26,9 +28,9 @@ import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
 import io.fd.honeycomb.translate.TranslationException;
-import io.fd.honeycomb.translate.write.WriteContext;
 import io.fd.honeycomb.translate.util.RWUtils;
 import io.fd.honeycomb.translate.write.DataObjectUpdate;
+import io.fd.honeycomb.translate.write.WriteContext;
 import io.fd.honeycomb.translate.write.WriteFailedException;
 import io.fd.honeycomb.translate.write.Writer;
 import io.fd.honeycomb.translate.write.registry.WriterRegistry;
@@ -211,11 +213,11 @@ final class FlatWriterRegistry implements WriterRegistry {
                     LOG.trace("Update successful for type: {}", writerType);
                     LOG.debug("Update successful for: {}", singleUpdate);
                 } catch (Exception e) {
-                    LOG.error("Error while processing data change of: {} (updates={})", writerType, writersData, e);
+                    // do not log this exception here,its logged in ModifiableDataTreeDelegator
 
                     final Reverter reverter = attemptRevert
-                            ? new ReverterImpl(processedNodes, updates, writersOrder, ctx)
-                            : () -> {}; // NOOP reverter
+                            ? new ReverterImpl(processedNodes, updates, writersOrder)
+                            : (final WriteContext writeContext) -> {};//NOOP reverter
 
                     // Find out which changes left unprocessed
                     final Set<InstanceIdentifier<?>> unprocessedChanges = updates.values().stream()
@@ -258,30 +260,29 @@ final class FlatWriterRegistry implements WriterRegistry {
         private final Collection<InstanceIdentifier<?>> processedNodes;
         private final Multimap<InstanceIdentifier<?>, ? extends DataObjectUpdate> updates;
         private final Set<InstanceIdentifier<?>> revertDeleteOrder;
-        private final WriteContext ctx;
 
         ReverterImpl(final Collection<InstanceIdentifier<?>> processedNodes,
                      final Multimap<InstanceIdentifier<?>, ? extends DataObjectUpdate> updates,
-                     final Set<InstanceIdentifier<?>> writersOrderOriginal,
-                     final WriteContext ctx) {
+                     final Set<InstanceIdentifier<?>> writersOrderOriginal) {
             this.processedNodes = processedNodes;
             this.updates = updates;
             // Use opposite ordering when executing revert
             this.revertDeleteOrder =  writersOrderOriginal == FlatWriterRegistry.this.writersOrder
                     ? FlatWriterRegistry.this.writersOrderReversed
                     : FlatWriterRegistry.this.writersOrder;
-            this.ctx = ctx;
         }
 
         @Override
-        public void revert() throws RevertFailedException {
+        public void revert(@Nonnull final WriteContext writeContext) throws RevertFailedException {
+            checkNotNull(writeContext, "Cannot revert changes for null context");
+
             Multimap<InstanceIdentifier<?>, DataObjectUpdate> updatesToRevert =
                     filterAndRevertProcessed(updates, processedNodes);
 
             LOG.info("Attempting revert for changes: {}", updatesToRevert);
             try {
                 // Perform reversed bulk update without revert attempt
-                bulkUpdate(updatesToRevert, ctx, true, revertDeleteOrder);
+                bulkUpdate(updatesToRevert, writeContext, true, revertDeleteOrder);
                 LOG.info("Revert successful");
             } catch (BulkUpdateException e) {
                 LOG.error("Revert failed", e);
@@ -298,11 +299,12 @@ final class FlatWriterRegistry implements WriterRegistry {
                 final Collection<InstanceIdentifier<?>> processedNodes) {
             final Multimap<InstanceIdentifier<?>, DataObjectUpdate> filtered = HashMultimap.create();
             for (InstanceIdentifier<?> processedNode : processedNodes) {
-                final InstanceIdentifier<?> wildcardedIid = RWUtils.makeIidWildcarded(processedNode);
+                final InstanceIdentifier<?> wildcardedIid = makeIidWildcarded(processedNode);
                 if (updates.containsKey(wildcardedIid)) {
                     updates.get(wildcardedIid).stream()
                             .filter(dataObjectUpdate -> processedNode.contains(dataObjectUpdate.getId()))
-                            .forEach(dataObjectUpdate -> filtered.put(processedNode, dataObjectUpdate.reverse()));
+                            // putting under unkeyed identifier, to prevent failing of checkAllTypesCanBeHandled
+                            .forEach(dataObjectUpdate -> filtered.put(wildcardedIid, dataObjectUpdate.reverse()));
                 }
             }
             return filtered;
diff --git a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/DataObjects.java b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/DataObjects.java
index f2f664efe..8dd3d2065 100644
--- a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/DataObjects.java
+++ b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/DataObjects.java
@@ -60,5 +60,17 @@ public class DataObjects {
         }
     }
 
-    public static class DataObjectKey implements Identifier<DataObjectK> {}
+    public interface DataObject1ChildK extends DataObject, ChildOf<DataObject1>, Identifiable<DataObject1ChildKey> {
+        // needs to be defined like this to have paths totally equal after cutting path for internally keyed id inside infra
+        InstanceIdentifier<DataObject1ChildK> IID =
+                RWUtils.makeIidWildcarded(InstanceIdentifier.create(DataObject1.class).child(DataObject1ChildK.class));
+        InstanceIdentifier<DataObject1ChildK> INTERNALLY_KEYED_IID = InstanceIdentifier.create(DataObject1.class)
+                .child(DataObject1ChildK.class, new DataObject1ChildKey());
+    }
+
+    public static class DataObject1ChildKey implements Identifier<DataObject1ChildK> {
+    }
+
+    public static class DataObjectKey implements Identifier<DataObjectK> {
+    }
 }
diff --git a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistryTest.java b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistryTest.java
index 6b75f923e..fdd2eaddb 100644
--- a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistryTest.java
+++ b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistryTest.java
@@ -1,10 +1,10 @@
 package io.fd.honeycomb.translate.util.write.registry;
 
-import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
@@ -18,10 +18,11 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.Multimap;
 import io.fd.honeycomb.translate.util.DataObjects;
+import io.fd.honeycomb.translate.util.DataObjects.DataObject1;
 import io.fd.honeycomb.translate.write.DataObjectUpdate;
 import io.fd.honeycomb.translate.write.WriteContext;
+import io.fd.honeycomb.translate.write.WriteFailedException;
 import io.fd.honeycomb.translate.write.Writer;
-import io.fd.honeycomb.translate.util.DataObjects.DataObject1;
 import io.fd.honeycomb.translate.write.registry.WriterRegistry;
 import org.hamcrest.CoreMatchers;
 import org.junit.Before;
@@ -41,7 +42,11 @@ public class FlatWriterRegistryTest {
     @Mock
     private Writer<DataObjects.DataObject3> writer3;
     @Mock
+    private Writer<DataObjects.DataObject1ChildK> writer4;
+    @Mock
     private WriteContext ctx;
+    @Mock
+    private WriteContext revertWriteContext;
 
     @Before
     public void setUp() throws Exception {
@@ -215,12 +220,13 @@ public class FlatWriterRegistryTest {
             inOrder.verify(writer3)
                 .update(any(InstanceIdentifier.class), any(DataObject.class), any(DataObject.class), any(WriteContext.class));
 
-            e.revertChanges();
+            e.revertChanges(revertWriteContext);
             // Revert changes. Successful updates are iterated in reverse
+            // also binding other write context,to verify if update context is not reused
             inOrder.verify(writer2)
-                    .update(iid2, after2, before2, ctx);
+                    .update(iid2, after2, before2, revertWriteContext);
             inOrder.verify(writer1)
-                    .update(any(InstanceIdentifier.class), any(DataObject.class), any(DataObject.class), any(WriteContext.class));
+                    .update(any(InstanceIdentifier.class), any(DataObject.class), any(DataObject.class), eq(revertWriteContext));
             verifyNoMoreInteractions(writer3);
         }
     }
@@ -248,7 +254,7 @@ public class FlatWriterRegistryTest {
             doThrow(new RuntimeException()).when(writer1)
                     .update(any(InstanceIdentifier.class), any(DataObject.class), any(DataObject.class), any(WriteContext.class));
             try {
-                e.revertChanges();
+                e.revertChanges(revertWriteContext);
             } catch (WriterRegistry.Reverter.RevertFailedException e1) {
                 assertThat(e1.getNotRevertedChanges().size(), is(1));
                 assertThat(e1.getNotRevertedChanges(), CoreMatchers
@@ -257,6 +263,48 @@ public class FlatWriterRegistryTest {
         }
     }
 
+    @Test
+    public void testMutlipleUpdatesWithOneKeyedContainer() throws Exception {
+        final InstanceIdentifier internallyKeyedIdentifier = InstanceIdentifier.create(DataObject1.class)
+                .child(DataObjects.DataObject1ChildK.class, new DataObjects.DataObject1ChildKey());
+
+        final FlatWriterRegistry flatWriterRegistry =
+                new FlatWriterRegistry(
+                        ImmutableMap.of(DataObjects.DataObject1.IID, writer1, DataObjects.DataObject1ChildK.IID,writer4));
+
+        // Writer1 always fails
+        doThrow(new RuntimeException()).when(writer1)
+                .update(any(InstanceIdentifier.class), any(DataObject.class), any(DataObject.class),
+                        any(WriteContext.class));
+
+        final Multimap<InstanceIdentifier<?>, DataObjectUpdate> updates = HashMultimap.create();
+        addKeyedUpdate(updates,DataObjects.DataObject1ChildK.class);
+        addUpdate(updates, DataObjects.DataObject1.class);
+        try {
+            flatWriterRegistry.update(new WriterRegistry.DataObjectUpdates(updates, ImmutableMultimap.of()), ctx);
+            fail("Bulk update should have failed on writer1");
+        } catch (WriterRegistry.BulkUpdateException e) {
+            // Writer1 always fails from now
+            doThrow(new RuntimeException()).when(writer1)
+                    .update(any(InstanceIdentifier.class), any(DataObject.class), any(DataObject.class),
+                            any(WriteContext.class));
+            try {
+                e.revertChanges(revertWriteContext);
+            } catch (WriterRegistry.Reverter.RevertFailedException e1) {
+                assertThat(e1.getNotRevertedChanges().size(), is(1));
+                assertThat(e1.getNotRevertedChanges(), CoreMatchers
+                        .hasItem(InstanceIdentifier.create(DataObjects.DataObject1.class)));
+            }
+        }
+    }
+
+    private <D extends DataObject> void addKeyedUpdate(final Multimap<InstanceIdentifier<?>, DataObjectUpdate> updates,
+                                                       final Class<D> type) throws Exception {
+        final InstanceIdentifier<D> iid = (InstanceIdentifier<D>) type.getDeclaredField("IID").get(null);
+        final InstanceIdentifier<D> keyedIid = (InstanceIdentifier<D>) type.getDeclaredField("INTERNALLY_KEYED_IID").get(null);
+        updates.put(iid, DataObjectUpdate.create(keyedIid, mock(type), mock(type)));
+    }
+
     private <D extends DataObject> void addUpdate(final Multimap<InstanceIdentifier<?>, DataObjectUpdate> updates,
                            final Class<D> type) throws Exception {
         final InstanceIdentifier<D> iid = (InstanceIdentifier<D>) type.getDeclaredField("IID").get(null);