Bug 6037 - Check if delete request was successful

- waiting for result on mount point devices
- checking if data to delete exists, if not return error 404

Change-Id: I0f4c11cb3dd9c7fc560a4fee06df0d398c0c4f61
Signed-off-by: Ivan Hrasko <ihrasko@cisco.com>

diff --git a/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java b/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
index 7a1fb38ef98fae4fe8d35c50d19e0c02972af8cb..2d9528b59558bb15900bf26ebaaee98f0f144ef3 100644 (file)
--- a/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
+++ b/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
@@ -248,14 +248,14 @@ public class BrokerFacade {
     public CheckedFuture<Void, TransactionCommitFailedException> commitConfigurationDataDelete(
             final YangInstanceIdentifier path) {
         checkPreconditions();
-        return deleteDataViaTransaction(domDataBroker.newWriteOnlyTransaction(), CONFIGURATION, path);
+        return deleteDataViaTransaction(domDataBroker.newReadWriteTransaction(), CONFIGURATION, path);
     }
 
     public CheckedFuture<Void, TransactionCommitFailedException> commitConfigurationDataDelete(
             final DOMMountPoint mountPoint, final YangInstanceIdentifier path) {
         final Optional<DOMDataBroker> domDataBrokerService = mountPoint.getService(DOMDataBroker.class);
         if (domDataBrokerService.isPresent()) {
-            return deleteDataViaTransaction(domDataBrokerService.get().newWriteOnlyTransaction(), CONFIGURATION, path);
+            return deleteDataViaTransaction(domDataBrokerService.get().newReadWriteTransaction(), CONFIGURATION, path);
         }
         final String errMsg = "DOM data broker service isn't available for mount point " + path;
         LOG.warn(errMsg);
@@ -370,11 +370,28 @@ public class BrokerFacade {
         }
     }
 
-    private void checkItemDoesNotExists(final DOMDataReadWriteTransaction rWTransaction,final LogicalDatastoreType store, final YangInstanceIdentifier path) {
+    private void checkItemExists(final DOMDataReadWriteTransaction rWTransaction,
+                                 final LogicalDatastoreType store, final YangInstanceIdentifier path) {
+        final ListenableFuture<Boolean> futureDatastoreData = rWTransaction.exists(store, path);
+        try {
+            if (!futureDatastoreData.get()) {
+                final String errMsg = "Operation via Restconf was not executed because data does not exist";
+                LOG.trace("{}:{}", errMsg, path);
+                rWTransaction.cancel();
+                throw new RestconfDocumentedException("Data does not exist for path: " + path, ErrorType.PROTOCOL,
+                        ErrorTag.DATA_MISSING);
+            }
+        } catch (InterruptedException | ExecutionException e) {
+            LOG.warn("It wasn't possible to get data loaded from datastore at path {}", path, e);
+        }
+    }
+
+    private void checkItemDoesNotExists(final DOMDataReadWriteTransaction rWTransaction,
+                                        final LogicalDatastoreType store, final YangInstanceIdentifier path) {
         final ListenableFuture<Boolean> futureDatastoreData = rWTransaction.exists(store, path);
         try {
             if (futureDatastoreData.get()) {
-                final String errMsg = "Post Configuration via Restconf was not executed because data already exists";
+                final String errMsg = "Operation via Restconf was not executed because data already exists";
                 LOG.trace("{}:{}", errMsg, path);
                 rWTransaction.cancel();
                 throw new RestconfDocumentedException("Data already exists for path: " + path, ErrorType.PROTOCOL,
@@ -383,7 +400,6 @@ public class BrokerFacade {
         } catch (InterruptedException | ExecutionException e) {
             LOG.warn("It wasn't possible to get data loaded from datastore at path {}", path, e);
         }
-
     }
 
     private CheckedFuture<Void, TransactionCommitFailedException> putDataViaTransaction(
@@ -409,11 +425,12 @@ public class BrokerFacade {
     }
 
     private CheckedFuture<Void, TransactionCommitFailedException> deleteDataViaTransaction(
-            final DOMDataWriteTransaction writeTransaction, final LogicalDatastoreType datastore,
+            final DOMDataReadWriteTransaction readWriteTransaction, final LogicalDatastoreType datastore,
             final YangInstanceIdentifier path) {
         LOG.trace("Delete {} via Restconf: {}", datastore.name(), path);
-        writeTransaction.delete(datastore, path);
-        return writeTransaction.submit();
+        checkItemExists(readWriteTransaction, datastore, path);
+        readWriteTransaction.delete(datastore, path);
+        return readWriteTransaction.submit();
     }
 
     private void deleteDataWithinTransaction(

diff --git a/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/RestconfImpl.java b/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/RestconfImpl.java
index c292a382fc5bfa351759ff04ecd649c53f1e1fb2..069407034028eefd1cd32a1e0744f17e630da072 100644 (file)
--- a/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/RestconfImpl.java
+++ b/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/RestconfImpl.java
@@ -12,10 +12,8 @@ import com.google.common.base.CharMatcher;
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
-import com.google.common.base.Predicates;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
-import com.google.common.base.Throwables;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
@@ -74,7 +72,6 @@ import org.opendaylight.yangtools.yang.data.api.schema.LeafSetEntryNode;
 import org.opendaylight.yangtools.yang.data.api.schema.MapEntryNode;
 import org.opendaylight.yangtools.yang.data.api.schema.MapNode;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
-import org.opendaylight.yangtools.yang.data.api.schema.tree.ModifiedNodeDoesNotExistException;
 import org.opendaylight.yangtools.yang.data.impl.schema.Builders;
 import org.opendaylight.yangtools.yang.data.impl.schema.ImmutableNodes;
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.api.CollectionNodeBuilder;
@@ -912,20 +909,16 @@ public class RestconfImpl implements RestconfService {
 
         try {
             if (mountPoint != null) {
-                broker.commitConfigurationDataDelete(mountPoint, normalizedII);
+                broker.commitConfigurationDataDelete(mountPoint, normalizedII).checkedGet();
             } else {
-                broker.commitConfigurationDataDelete(normalizedII).get();
-            }
-        } catch (final Exception e) {
-            final Optional<Throwable> searchedException = Iterables.tryFind(Throwables.getCausalChain(e),
-                    Predicates.instanceOf(ModifiedNodeDoesNotExistException.class));
-            if (searchedException.isPresent()) {
-                throw new RestconfDocumentedException("Data specified for deleting doesn't exist.", ErrorType.APPLICATION, ErrorTag.DATA_MISSING);
+                broker.commitConfigurationDataDelete(normalizedII).checkedGet();
             }
+        } catch (final TransactionCommitFailedException e) {
             final String errMsg = "Error while deleting data";
             LOG.info(errMsg, e);
             throw new RestconfDocumentedException(errMsg, e);
         }
+
         return Response.status(Status.OK).build();
     }
 

diff --git a/opendaylight/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/BrokerFacadeTest.java b/opendaylight/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/BrokerFacadeTest.java
index 43b3ca7bec977c28e15f6482d28891dfc5d6756e..4cc2ec4d937d0973f91620cfb2c3082938c87cd9 100644 (file)
--- a/opendaylight/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/BrokerFacadeTest.java
+++ b/opendaylight/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/BrokerFacadeTest.java
@@ -11,6 +11,7 @@ package org.opendaylight.controller.sal.restconf.impl.test;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.inOrder;
@@ -47,6 +48,8 @@ import org.opendaylight.netconf.sal.restconf.impl.BrokerFacade;
 import org.opendaylight.netconf.sal.restconf.impl.ControllerContext;
 import org.opendaylight.netconf.sal.restconf.impl.RestconfDocumentedException;
 import org.opendaylight.netconf.sal.restconf.impl.RestconfError;
+import org.opendaylight.netconf.sal.restconf.impl.RestconfError.ErrorTag;
+import org.opendaylight.netconf.sal.restconf.impl.RestconfError.ErrorType;
 import org.opendaylight.netconf.sal.streams.listeners.ListenerAdapter;
 import org.opendaylight.netconf.sal.streams.listeners.Notificator;
 import org.opendaylight.yangtools.concepts.ListenerRegistration;
@@ -64,43 +67,26 @@ import org.opendaylight.yangtools.yang.model.api.SchemaPath;
  * @author Thomas Pantelis
  */
 public class BrokerFacadeTest {
-
-    @Mock
-    DOMDataBroker domDataBroker;
-
-    @Mock
-    ConsumerSession context;
-
-    @Mock
-    DOMRpcService mockRpcService;
-
-    @Mock
-    DOMMountPoint mockMountInstance;
-
-    BrokerFacade brokerFacade = BrokerFacade.getInstance();
-
-    NormalizedNode<?, ?> dummyNode = createDummyNode("test:module", "2014-01-09", "interfaces");
-    CheckedFuture<Optional<NormalizedNode<?, ?>>,ReadFailedException> dummyNodeInFuture = wrapDummyNode(dummyNode);
-
-    QName qname = TestUtils.buildQName("interfaces","test:module", "2014-01-09");
-
-    SchemaPath type = SchemaPath.create(true, qname);
-
-    YangInstanceIdentifier instanceID = YangInstanceIdentifier.builder().node(qname).build();
-
-    @Mock
-    DOMDataReadOnlyTransaction rTransaction;
-
-    @Mock
-    DOMDataWriteTransaction wTransaction;
-
-    @Mock
-    DOMDataReadWriteTransaction rwTransaction;
+    @Mock private DOMDataBroker domDataBroker;
+    @Mock private ConsumerSession context;
+    @Mock private DOMRpcService mockRpcService;
+    @Mock private DOMMountPoint mockMountInstance;
+
+    private final BrokerFacade brokerFacade = BrokerFacade.getInstance();
+    private final NormalizedNode<?, ?> dummyNode = createDummyNode("test:module", "2014-01-09", "interfaces");
+    private final CheckedFuture<Optional<NormalizedNode<?, ?>>, ReadFailedException> dummyNodeInFuture =
+            wrapDummyNode(dummyNode);
+    private final QName qname = TestUtils.buildQName("interfaces","test:module", "2014-01-09");
+    private final SchemaPath type = SchemaPath.create(true, qname);
+    private final YangInstanceIdentifier instanceID = YangInstanceIdentifier.builder().node(qname).build();
+
+    @Mock private DOMDataReadOnlyTransaction rTransaction;
+    @Mock private DOMDataWriteTransaction wTransaction;
+    @Mock private DOMDataReadWriteTransaction rwTransaction;
 
     @Before
     public void setUp() throws Exception {
         MockitoAnnotations.initMocks(this);
-        // TODO it is started before every test method
         brokerFacade.setDomDataBroker(domDataBroker);
         brokerFacade.setRpcService(mockRpcService);
         brokerFacade.setContext(context);
@@ -109,18 +95,17 @@ public class BrokerFacadeTest {
         when(domDataBroker.newReadWriteTransaction()).thenReturn(rwTransaction);
 
         ControllerContext.getInstance().setSchemas(TestUtils.loadSchemaContext("/full-versions/test-module"));
-
     }
 
-    private CheckedFuture<Optional<NormalizedNode<?, ?>>,ReadFailedException> wrapDummyNode(final NormalizedNode<?, ?> dummyNode) {
-        return  Futures.immediateCheckedFuture(Optional.<NormalizedNode<?, ?>> of(dummyNode));
+    private CheckedFuture<Optional<NormalizedNode<?, ?>>, ReadFailedException> wrapDummyNode(
+            final NormalizedNode<?, ?> dummyNode) {
+        return Futures.immediateCheckedFuture(Optional.<NormalizedNode<?, ?>> of(dummyNode));
     }
 
-    private CheckedFuture<Boolean,ReadFailedException> wrapExistence(final Boolean exists) {
-        return  Futures.immediateCheckedFuture(exists);
+    private CheckedFuture<Boolean, ReadFailedException> wrapExistence(final Boolean exists) {
+        return Futures.immediateCheckedFuture(exists);
     }
 
-
     /**
      * Value of this node shouldn't be important for testing purposes
      */
@@ -200,7 +185,6 @@ public class BrokerFacadeTest {
         when(rwTransaction.exists(eq(LogicalDatastoreType.CONFIGURATION), any(YangInstanceIdentifier.class))).thenReturn(
             wrapExistence(false));
 
-
         when(rwTransaction.submit()).thenReturn(expFuture);
 
         final CheckedFuture<Void, TransactionCommitFailedException> actualFuture = brokerFacade.commitConfigurationDataPost(
@@ -229,22 +213,61 @@ public class BrokerFacadeTest {
         }
     }
 
+    /**
+     * Positive test of delete operation when data to delete exits. Returned value and order of steps are validated.
+     */
     @Test
-    public void testCommitConfigurationDataDelete() {
-        @SuppressWarnings("unchecked")
-        final CheckedFuture<Void, TransactionCommitFailedException> expFuture = mock(CheckedFuture.class);
+    public void testCommitConfigurationDataDelete() throws Exception {
+        // assume that data to delete exists
+        prepareDataForDelete(true);
 
-        when(wTransaction.submit()).thenReturn(expFuture);
+        // expected result
+        final CheckedFuture<Void, TransactionCommitFailedException> expFuture = mock(CheckedFuture.class);
+        when(rwTransaction.submit()).thenReturn(expFuture);
 
+        // test
         final CheckedFuture<Void, TransactionCommitFailedException> actualFuture = brokerFacade
                 .commitConfigurationDataDelete(instanceID);
 
+        // verify result and interactions
         assertSame("commitConfigurationDataDelete", expFuture, actualFuture);
 
-        final InOrder inOrder = inOrder(domDataBroker, wTransaction);
-        inOrder.verify(domDataBroker).newWriteOnlyTransaction();
-        inOrder.verify(wTransaction).delete(eq(LogicalDatastoreType.CONFIGURATION), any(YangInstanceIdentifier.class));
-        inOrder.verify(wTransaction).submit();
+        // check exists, delete, submit
+        final InOrder inOrder = inOrder(domDataBroker, rwTransaction);
+        inOrder.verify(rwTransaction).exists(LogicalDatastoreType.CONFIGURATION, instanceID);
+        inOrder.verify(rwTransaction).delete(LogicalDatastoreType.CONFIGURATION, instanceID);
+        inOrder.verify(rwTransaction).submit();
+    }
+
+    /**
+     * Negative test of delete operation when data to delete does not exist. Error 404 should be returned.
+     */
+    @Test
+    public void testCommitConfigurationDataDeleteNoData() throws Exception {
+        // assume that data to delete does not exist
+        prepareDataForDelete(false);
+
+        // try to delete and expect 404 error
+        try {
+            brokerFacade.commitConfigurationDataDelete(instanceID);
+            fail("Delete operation should fail due to missing data");
+        } catch (final RestconfDocumentedException e) {
+            assertEquals(ErrorType.PROTOCOL, e.getErrors().get(0).getErrorType());
+            assertEquals(ErrorTag.DATA_MISSING, e.getErrors().get(0).getErrorTag());
+            assertEquals(404, e.getErrors().get(0).getErrorTag().getStatusCode());
+        }
+    }
+
+    /**
+     * Prepare conditions to test delete operation. Data to delete exists or does not exist according to value of
+     * {@code assumeDataExists} parameter.
+     * @param assumeDataExists
+     * @throws Exception
+     */
+    private void prepareDataForDelete(final boolean assumeDataExists) throws Exception {
+        final CheckedFuture<Boolean, ReadFailedException> checkDataExistFuture = mock(CheckedFuture.class);
+        when(checkDataExistFuture.get()).thenReturn(assumeDataExists);
+        when(rwTransaction.exists(LogicalDatastoreType.CONFIGURATION, instanceID)).thenReturn(checkDataExistFuture);
     }
 
     @SuppressWarnings("unchecked")
@@ -267,6 +290,5 @@ public class BrokerFacadeTest {
 
         brokerFacade.registerToListenDataChanges(LogicalDatastoreType.CONFIGURATION, DataChangeScope.BASE, listener);
         verifyNoMoreInteractions(domDataBroker);
-
     }
 }