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>

9 files changed, 158 insertions, 38 deletions

diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java
index 213208064..3de9131c0 100644
--- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java
+++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java
@@ -26,6 +26,7 @@ import com.google.common.collect.Multimap;
 import com.google.common.util.concurrent.CheckedFuture;
 import io.fd.honeycomb.data.DataModification;
 import io.fd.honeycomb.data.ReadableDataManager;
+import io.fd.honeycomb.translate.MappingContext;
 import io.fd.honeycomb.translate.TranslationException;
 import io.fd.honeycomb.translate.util.RWUtils;
 import io.fd.honeycomb.translate.util.TransactionMappingContext;
@@ -126,7 +127,8 @@ public final class ModifiableDataTreeDelegator extends ModifiableDataTreeManager
             final WriterRegistry.DataObjectUpdates baUpdates = toBindingAware(modificationDiff.getUpdates());
             LOG.debug("ConfigDataTree.modify() extracted updates={}", baUpdates);
 
-            try (final WriteContext ctx = getTransactionWriteContext()) {
+            WriteContext ctx = getTransactionWriteContext();
+            try {
                 writerRegistry.update(baUpdates, ctx);
 
                 final CheckedFuture<Void, TransactionCommitFailedException> contextUpdateResult =
@@ -139,15 +141,18 @@ public final class ModifiableDataTreeDelegator extends ModifiableDataTreeManager
                 LOG.info("Trying to revert successful changes for current transaction");
 
                 try {
-                    e.revertChanges();
+                    // attempt revert with fresh context, to allow write logic used context-binded data
+                    e.revertChanges(getRevertTransactionContext(ctx.getMappingContext()));
                     LOG.info("Changes successfully reverted");
                 } catch (WriterRegistry.Reverter.RevertFailedException revertFailedException) {
                     // fail with failed revert
                     LOG.error("Failed to revert successful changes", revertFailedException);
                     throw revertFailedException;
                 }
-
-                throw e; // fail with success revert
+                // fail with success revert
+                // not passing the cause,its logged above and it would be logged after transaction
+                // ended again(prevent double logging of same error
+                throw new WriterRegistry.Reverter.RevertSuccessException(e.getFailedIds());
             } catch (TransactionCommitFailedException e) {
                 // TODO HONEYCOMB-162 revert should probably occur when context is not written successfully
                 final String msg = "Error while updating mapping context data";
@@ -156,9 +161,27 @@ public final class ModifiableDataTreeDelegator extends ModifiableDataTreeManager
             } catch (TranslationException e) {
                 LOG.error("Error while processing data change (updates={})", baUpdates, e);
                 throw e;
+            } finally {
+                // Using finally instead of try-with-resources in order to leave ctx open for BulkUpdateException catch
+                // block. The context is needed there, but try-with-resources closes the resource before handling ex.
+                LOG.debug("Closing write context {}", ctx);
+                ctx.close();
             }
         }
 
+        /**
+         * Creates inverted transaction context for reverting of proceeded changes.
+         * Invert before/after transaction and reuse affected mapping context created by previous updates
+         * to access all data created by previous updates
+         * */
+        private TransactionWriteContext getRevertTransactionContext(final MappingContext affectedMappingContext){
+            // Before Tx == after partial update
+            final DOMDataReadOnlyTransaction beforeTx = ReadOnlyTransaction.create(this, EMPTY_OPERATIONAL);
+            // After Tx == before partial update
+            final DOMDataReadOnlyTransaction afterTx = ReadOnlyTransaction.create(untouchedModification, EMPTY_OPERATIONAL);
+            return new TransactionWriteContext(serializer, beforeTx, afterTx, affectedMappingContext);
+        }
+
         private TransactionWriteContext getTransactionWriteContext() {
             // Before Tx must use modification
             final DOMDataReadOnlyTransaction beforeTx = ReadOnlyTransaction.create(untouchedModification, EMPTY_OPERATIONAL);
diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java
index 2cbe1ec8b..93043ce07 100644
--- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java
+++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java
@@ -142,7 +142,7 @@ final class WriteTransaction implements DOMDataWriteTransaction {
             status = COMMITED;
         } catch (DataValidationFailedException | TranslationException e) {
             status = FAILED;
-            LOG.error("Failed modify data tree", e);
+            LOG.debug("Submit failed", e);
             return Futures.immediateFailedCheckedFuture(
                 new TransactionCommitFailedException("Failed to validate DataTreeModification", e));
         }
diff --git a/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java b/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java
index 0a7c85b3b..322328508 100644
--- a/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java
+++ b/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java
@@ -49,6 +49,8 @@ import java.util.HashMap;
 import java.util.Map;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
 import org.mockito.Mock;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException;
@@ -76,6 +78,9 @@ public class ModifiableDataTreeDelegatorTest {
     @Mock
     private org.opendaylight.controller.md.sal.binding.api.ReadWriteTransaction tx;
 
+    @Captor
+    private ArgumentCaptor<WriteContext> writeContextCaptor;
+
     private ModifiableDataTreeManager configDataTree;
 
     static final InstanceIdentifier<?> DEFAULT_ID = InstanceIdentifier.create(DataObject.class);
@@ -149,12 +154,11 @@ public class ModifiableDataTreeDelegatorTest {
             dataModification.write(ModificationDiffTest.NESTED_LIST_ID, nestedList);
             dataModification.validate();
             dataModification.commit();
-            fail("WriterRegistry.BulkUpdateException was expected");
-        } catch (WriterRegistry.BulkUpdateException e) {
+            fail("WriterRegistry.RevertSuccessException was expected");
+        } catch (WriterRegistry.Reverter.RevertSuccessException e) {
             verify(writer).update(any(WriterRegistry.DataObjectUpdates.class), any(WriteContext.class));
             assertThat(e.getFailedIds(), hasItem(DEFAULT_ID));
-            verify(reverter).revert();
-            assertEquals(failedOnUpdateException, e.getCause());
+            verify(reverter).revert(any(WriteContext.class));
         }
     }
 
@@ -171,7 +175,7 @@ public class ModifiableDataTreeDelegatorTest {
         // Fail on revert:
         final TranslationException failedOnRevertException = new TranslationException("revert failed");
         doThrow(new WriterRegistry.Reverter.RevertFailedException(Collections.emptySet(), failedOnRevertException))
-                .when(reverter).revert();
+                .when(reverter).revert(any(WriteContext.class));
 
         try {
             // Run the test
@@ -182,7 +186,7 @@ public class ModifiableDataTreeDelegatorTest {
             fail("WriterRegistry.Reverter.RevertFailedException was expected");
         } catch (WriterRegistry.Reverter.RevertFailedException e) {
             verify(writer).update(any(WriterRegistry.DataObjectUpdates.class), any(WriteContext.class));
-            verify(reverter).revert();
+            verify(reverter).revert(any(WriteContext.class));
             assertEquals(failedOnRevertException, e.getCause());
         }
     }
diff --git a/infra/translate-api/src/main/java/io/fd/honeycomb/translate/write/registry/WriterRegistry.java b/infra/translate-api/src/main/java/io/fd/honeycomb/translate/write/registry/WriterRegistry.java
index d281ee558..51e6415a2 100644
--- a/infra/translate-api/src/main/java/io/fd/honeycomb/translate/write/registry/WriterRegistry.java
+++ b/infra/translate-api/src/main/java/io/fd/honeycomb/translate/write/registry/WriterRegistry.java
@@ -152,10 +152,12 @@ public interface WriterRegistry {
         /**
          * Reverts changes that were successfully applied during bulk update before failure occurred.
          *
+         * @param writeContext Non-closed {@code WriteContext} to be used by reverting logic.<br> <b>Do not use same
+         *                     write context as was used in previous write</b>
          * @throws Reverter.RevertFailedException if revert fails
          */
-        public void revertChanges() throws Reverter.RevertFailedException {
-            reverter.revert();
+        public void revertChanges(@Nonnull final WriteContext writeContext) throws Reverter.RevertFailedException {
+            reverter.revert(writeContext);
         }
 
         public Set<InstanceIdentifier<?>> getFailedIds() {
@@ -172,10 +174,13 @@ public interface WriterRegistry {
         /**
          * Reverts changes that were successfully applied during bulk update before failure occurred. Changes are
          * reverted in reverse order they were applied.
+         * Used {@code WriteContext} needs to be in non-closed state, creating fresh one for revert
+         * is recommended, same way as for write, to allow {@code Reverter} use same logic as write.
          *
+         * @param writeContext Non-closed {@code WriteContext} to be used by reverting logic
          * @throws RevertFailedException if not all of applied changes were successfully reverted
          */
-        void revert() throws RevertFailedException;
+        void revert(@Nonnull final WriteContext writeContext) throws RevertFailedException;
 
         /**
          * Thrown when some of the changes applied during bulk update were not reverted.
@@ -209,5 +214,29 @@ public interface WriterRegistry {
                 return notRevertedChanges;
             }
         }
+
+        /**
+         * Thrown after bulk operation was successfully reverted,
+         * to maintain marking of transaction as failed,without double logging of
+         * cause of update fail(its logged before reverting in ModifiableDataTreeDelegator
+         */
+        @Beta
+        class RevertSuccessException extends TranslationException {
+            private final Set<InstanceIdentifier<?>> failedIds;
+
+            /**
+             * Constructs an RevertSuccessException.
+             *
+             * @param failedIds instance identifiers of the data objects that were not processed during bulk update.
+             */
+            public RevertSuccessException(@Nonnull final Set<InstanceIdentifier<?>> failedIds) {
+                super("Bulk update failed for: " + failedIds);
+                this.failedIds = failedIds;
+            }
+
+            public Set<InstanceIdentifier<?>> getFailedIds() {
+                return failedIds;
+            }
+        }
     }
 }
\ No newline at end of file
diff --git a/infra/translate-api/src/test/java/io/fd/honeycomb/translate/write/registry/BulkUpdateExceptionTest.java b/infra/translate-api/src/test/java/io/fd/honeycomb/translate/write/registry/BulkUpdateExceptionTest.java
index c1880ca26..ae0c36d5b 100644
--- a/infra/translate-api/src/test/java/io/fd/honeycomb/translate/write/registry/BulkUpdateExceptionTest.java
+++ b/infra/translate-api/src/test/java/io/fd/honeycomb/translate/write/registry/BulkUpdateExceptionTest.java
@@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals;
 import static org.mockito.Mockito.verify;
 
 import com.google.common.collect.Sets;
+import io.fd.honeycomb.translate.write.WriteContext;
 import java.util.HashSet;
 import org.junit.Before;
 import org.junit.Test;
@@ -31,6 +32,10 @@ import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 public class BulkUpdateExceptionTest {
 
     private InstanceIdentifier<?> id = InstanceIdentifier.create(DataObject.class);
+
+    @Mock
+    private WriteContext writeContext;
+
     @Mock
     private WriterRegistry.Reverter reverter;
 
@@ -47,7 +52,7 @@ public class BulkUpdateExceptionTest {
 
         assertEquals(failedIds, bulkUpdateException.getFailedIds());
 
-        bulkUpdateException.revertChanges();
-        verify(reverter).revert();
+        bulkUpdateException.revertChanges(writeContext);
+        verify(reverter).revert(writeContext);
     }
 }
\ No newline at end of file
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);