[#1858] Don't treat removeCmdHandlingAdapterInstance 404 as error.

Command handling adapter instance entries may expire. Therefore, getting a NOT FOUND result when invoking removeCommandHandlingAdapterInstance() shouldn't be treated as an error. Signed-off-by: Carsten Lohmann <[email protected]>
eclipse-hono · Apr 28, 2020 · 57b0b83 · 57b0b83
1 parent 6ab8cb1
commit 57b0b83
Show file tree

Hide file tree

Showing 11 changed files with 139 additions and 82 deletions.
diff --git a/...org/eclipse/hono/deviceconnection/infinispan/client/CacheBasedDeviceConnectionClient.java b/...org/eclipse/hono/deviceconnection/infinispan/client/CacheBasedDeviceConnectionClient.java
@@ -122,7 +122,7 @@ public Future<Void> setCommandHandlingAdapterInstance(final String deviceId, fin
     }
 
     @Override
-    public Future<Void> removeCommandHandlingAdapterInstance(final String deviceId, final String adapterInstanceId,
+    public Future<Boolean> removeCommandHandlingAdapterInstance(final String deviceId, final String adapterInstanceId,
             final SpanContext context) {
         return cache.removeCommandHandlingAdapterInstance(tenantId, deviceId, adapterInstanceId, context);
     }

diff --git a/...a/org/eclipse/hono/deviceconnection/infinispan/client/CacheBasedDeviceConnectionInfo.java b/...a/org/eclipse/hono/deviceconnection/infinispan/client/CacheBasedDeviceConnectionInfo.java
@@ -162,7 +162,7 @@ public Future<Void> setCommandHandlingAdapterInstance(final String tenantId, fin
     }
 
     @Override
-    public Future<Void> removeCommandHandlingAdapterInstance(final String tenantId, final String deviceId,
+    public Future<Boolean> removeCommandHandlingAdapterInstance(final String tenantId, final String deviceId,
             final String adapterInstanceId, final SpanContext context) {
         Objects.requireNonNull(tenantId);
         Objects.requireNonNull(deviceId);
@@ -177,16 +177,15 @@ public Future<Void> removeCommandHandlingAdapterInstance(final String tenantId,
                             tenantId, deviceId, adapterInstanceId, t);
                     return Future.failedFuture(new ServerErrorException(HttpURLConnection.HTTP_INTERNAL_ERROR, t));
                 })
-                .compose(removed -> {
+                .map(removed -> {
                     if (!removed) {
                         LOG.debug("command handling adapter instance was not removed, key not mapped or value didn't match [tenant: {}, device-id: {}, adapter-instance: {}]",
                                 tenantId, deviceId, adapterInstanceId);
-                        return Future.failedFuture(new ClientErrorException(HttpURLConnection.HTTP_NOT_FOUND));
                     } else {
                         LOG.debug("removed command handling adapter instance [tenant: {}, device-id: {}, adapter-instance: {}]",
                                 tenantId, deviceId, adapterInstanceId);
-                        return Future.succeededFuture();
                     }
+                    return removed;
                 });
 
     }

diff --git a/...c/main/java/org/eclipse/hono/deviceconnection/infinispan/client/DeviceConnectionInfo.java b/...c/main/java/org/eclipse/hono/deviceconnection/infinispan/client/DeviceConnectionInfo.java
@@ -99,13 +99,14 @@ Future<Void> setCommandHandlingAdapterInstance(String tenantId, String deviceId,
      * @param context The currently active OpenTracing span context or {@code null} if no span is currently active.
      *            Implementing classes should use this as the parent for any span they create for tracing
      *            the execution of this operation.
-     * @return A future indicating the outcome of the operation.
+     * @return A future indicating the outcome of the operation, with its value indicating whether the protocol
+     *         adapter instance value was removed or not.
      *         <p>
-     *         The future will be succeeded if the entry was successfully removed.
-     *         Otherwise the future will be failed with a {@link org.eclipse.hono.client.ServiceInvocationException}.
+     *         The future will be failed with a {@link org.eclipse.hono.client.ServiceInvocationException} if there
+     *         was an error removing the value.
      * @throws NullPointerException if any of the parameters except context is {@code null}.
      */
-    Future<Void> removeCommandHandlingAdapterInstance(String tenantId, String deviceId, String adapterInstanceId, SpanContext context);
+    Future<Boolean> removeCommandHandlingAdapterInstance(String tenantId, String deviceId, String adapterInstanceId, SpanContext context);
 
     /**
      * Gets information about the adapter instances that can handle a command for the given device.

diff --git a/...g/eclipse/hono/deviceconnection/infinispan/client/CacheBasedDeviceConnectionInfoTest.java b/...g/eclipse/hono/deviceconnection/infinispan/client/CacheBasedDeviceConnectionInfoTest.java
@@ -227,11 +227,14 @@ public void testRemoveCommandHandlingAdapterInstanceSucceeds(final VertxTestCont
         info.setCommandHandlingAdapterInstance(Constants.DEFAULT_TENANT, deviceId, adapterInstance, null, spanContext)
         .compose(v -> info.removeCommandHandlingAdapterInstance(Constants.DEFAULT_TENANT, deviceId,
                 adapterInstance, spanContext))
-        .setHandler(ctx.succeeding(result -> ctx.completeNow()));
+        .setHandler(ctx.succeeding(result -> ctx.verify(() -> {
+            assertThat(result).isTrue();
+            ctx.completeNow();
+        })));
     }
 
     /**
-     * Verifies that the <em>removeCommandHandlingAdapterInstance</em> operation fails with a NOT_FOUND status if
+     * Verifies that the <em>removeCommandHandlingAdapterInstance</em> operation result is <em>false</em> if
      * no entry was registered for the device. Only an adapter instance for another device of the tenant was
      * registered.
      *
@@ -244,16 +247,15 @@ public void testRemoveCommandHandlingAdapterInstanceFailsForOtherDevice(final Ve
         info.setCommandHandlingAdapterInstance(Constants.DEFAULT_TENANT, deviceId, adapterInstance, null, spanContext)
         .compose(v -> {
             return info.removeCommandHandlingAdapterInstance(Constants.DEFAULT_TENANT, "otherDevice", adapterInstance, spanContext);
-        }).setHandler(ctx.failing(t -> ctx.verify(() -> {
-            assertThat(t).isInstanceOf(ServiceInvocationException.class);
-            assertThat(((ServiceInvocationException) t).getErrorCode()).isEqualTo(HttpURLConnection.HTTP_NOT_FOUND);
+        }).setHandler(ctx.succeeding(result -> ctx.verify(() -> {
+            assertThat(result).isFalse();
             ctx.completeNow();
         })));
     }
 
     /**
-     * Verifies that the <em>removeCommandHandlingAdapterInstance</em> operation fails with a NOT_FOUND status
-     * if the given adapter instance parameter doesn't match the one of the entry registered for the given device.
+     * Verifies that the <em>removeCommandHandlingAdapterInstance</em> operation result is <em>false</em> if
+     * the given adapter instance parameter doesn't match the one of the entry registered for the given device.
      *
      * @param ctx The vert.x context.
      */
@@ -264,9 +266,8 @@ public void testRemoveCommandHandlingAdapterInstanceFailsForOtherAdapterInstance
         info.setCommandHandlingAdapterInstance(Constants.DEFAULT_TENANT, deviceId, adapterInstance, null, spanContext)
         .compose(v -> {
             return info.removeCommandHandlingAdapterInstance(Constants.DEFAULT_TENANT, deviceId, "otherAdapterInstance", spanContext);
-        }).setHandler(ctx.failing(t -> ctx.verify(() -> {
-            assertThat(t).isInstanceOf(ServiceInvocationException.class);
-            assertThat(((ServiceInvocationException) t).getErrorCode()).isEqualTo(HttpURLConnection.HTTP_NOT_FOUND);
+        }).setHandler(ctx.succeeding(result -> ctx.verify(() -> {
+            assertThat(result).isFalse();
             ctx.completeNow();
         })));
     }

diff --git a/client/src/main/java/org/eclipse/hono/client/DeviceConnectionClient.java b/client/src/main/java/org/eclipse/hono/client/DeviceConnectionClient.java
@@ -95,13 +95,14 @@ Future<Void> setCommandHandlingAdapterInstance(String deviceId, String adapterIn
      * @param context The currently active OpenTracing span context or {@code null} if no span is currently active.
      *            An implementation should use this as the parent for any span it creates for tracing
      *            the execution of this operation.
-     * @return A future indicating the outcome of the operation.
+     * @return A future indicating the outcome of the operation, with its value indicating whether the protocol
+     *         adapter instance value was removed or not.
      *         <p>
-     *         The future will be succeeded if the entry was successfully removed.
-     *         Otherwise the future will be failed with a {@link org.eclipse.hono.client.ServiceInvocationException}.
+     *         The future will be failed with a {@link org.eclipse.hono.client.ServiceInvocationException} if there
+     *         was an error removing the value.
      * @throws NullPointerException if device id or adapter instance id is {@code null}.
      */
-    Future<Void> removeCommandHandlingAdapterInstance(String deviceId, String adapterInstanceId, SpanContext context);
+    Future<Boolean> removeCommandHandlingAdapterInstance(String deviceId, String adapterInstanceId, SpanContext context);
 
     /**
      * Gets information about the adapter instances that can handle a command for the given device.

diff --git a/client/src/main/java/org/eclipse/hono/client/impl/DeviceConnectionClientImpl.java b/client/src/main/java/org/eclipse/hono/client/impl/DeviceConnectionClientImpl.java
@@ -232,7 +232,7 @@ public Future<JsonObject> getLastKnownGatewayForDevice(final String deviceId, fi
     }
 
     @Override
-    public Future<Void> removeCommandHandlingAdapterInstance(final String deviceId, final String adapterInstanceId, final SpanContext context) {
+    public Future<Boolean> removeCommandHandlingAdapterInstance(final String deviceId, final String adapterInstanceId, final SpanContext context) {
         Objects.requireNonNull(deviceId);
         Objects.requireNonNull(adapterInstanceId);
 
@@ -252,7 +252,9 @@ public Future<Void> removeCommandHandlingAdapterInstance(final String deviceId,
         return mapResultAndFinishSpan(resultTracker.future(), result -> {
             switch (result.getStatus()) {
                 case HttpURLConnection.HTTP_NO_CONTENT:
-                    return null;
+                    return Boolean.TRUE;
+                case HttpURLConnection.HTTP_NOT_FOUND:
+                    return Boolean.FALSE;
                 default:
                     throw StatusCodeMapper.from(result);
             }

diff --git a/...src/main/java/org/eclipse/hono/client/impl/ProtocolAdapterCommandConsumerFactoryImpl.java b/...src/main/java/org/eclipse/hono/client/impl/ProtocolAdapterCommandConsumerFactoryImpl.java
@@ -15,7 +15,6 @@
 
 import java.net.HttpURLConnection;
 import java.time.Duration;
-import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
@@ -30,7 +29,6 @@
 import org.eclipse.hono.client.MessageConsumer;
 import org.eclipse.hono.client.ProtocolAdapterCommandConsumerFactory;
 import org.eclipse.hono.client.ServerErrorException;
-import org.eclipse.hono.client.ServiceInvocationException;
 import org.eclipse.hono.util.CommandConstants;
 import org.eclipse.hono.util.Constants;
 import org.eclipse.hono.util.ResourceIdentifier;
@@ -173,9 +171,8 @@ private Future<MessageConsumer> doCreateCommandConsumer(final String tenantId, f
                         return setCommandHandlingAdapterInstance(tenantId, deviceId, sanitizedLifespan, context);
                     })
                     .map(res -> {
-                        final Instant lifespanStart = Instant.now();
                         final Supplier<Future<Void>> onCloseAction = () -> removeCommandConsumer(tenantId, deviceId,
-                                sanitizedLifespan, lifespanStart, context);
+                                sanitizedLifespan, context);
                         return (MessageConsumer) new DeviceSpecificCommandConsumer(onCloseAction);
                     })
                     .setHandler(result);
@@ -197,34 +194,23 @@ private Future<Void> setCommandHandlingAdapterInstance(final String tenantId, fi
     }
 
     private Future<Void> removeCommandConsumer(final String tenantId, final String deviceId, final Duration lifespan,
-            final Instant lifespanStart, final SpanContext createCommandConsumerSpanContext) {
+            final SpanContext createCommandConsumerSpanContext) {
         log.trace("remove command consumer [tenant-id: {}, device-id: {}]", tenantId, deviceId);
         adapterInstanceCommandHandler.removeDeviceSpecificCommandHandler(tenantId, deviceId);
 
-        final boolean lifespanReached = !lifespan.isNegative() && Instant.now().isAfter(lifespanStart.plus(lifespan));
-        // TODO when making handling of the lifespan property mandatory for implementors of the Device Connection API,
-        //  removing the adapter instance can be skipped here if 'lifespanReached' is true
         return deviceConnectionClientFactory.getOrCreateDeviceConnectionClient(tenantId)
                 .compose(client -> {
                     // The given span context from the createCommandConsumer invocation is only used here if a non-unlimited lifespan is set
                     // meaning creating and removing the consumer will probably happen in a timespan short enough for one overall trace.
-                    // The span context isn't used however if lifespanReached is true since the below 'remove' operation will usually (but not necessarily)
-                    // result in a "not found" error in that case, which is expected and should not cause the overall trace to be marked with an error.
-                    final SpanContext context = !lifespan.isNegative() && !lifespanReached ? createCommandConsumerSpanContext : null;
+                    final SpanContext context = !lifespan.isNegative() ? createCommandConsumerSpanContext : null;
                     return client.removeCommandHandlingAdapterInstance(deviceId, adapterInstanceId, context);
-                }).recover(thr -> {
-                    if (lifespanReached && thr instanceof ServiceInvocationException
-                            && ((ServiceInvocationException) thr).getErrorCode() == HttpURLConnection.HTTP_NOT_FOUND) {
-                        // entry was not found, meaning it has expired
-                        log.trace("ignoring 404 error when removing command handling adapter instance; entry has already expired [tenant: {}, device: {}]",
-                                tenantId, deviceId);
-                        return Future.succeededFuture();
-                    } else {
-                        log.info("error removing command handling adapter instance [tenant: {}, device: {}]", tenantId,
-                                deviceId, thr);
-                        return Future.failedFuture(thr);
-                    }
-                });
+                })
+                .recover(thr -> {
+                    log.warn("error removing command handling adapter instance [tenant: {}, device: {}]", tenantId,
+                            deviceId, thr);
+                    return Future.failedFuture(thr);
+                })
+                .mapEmpty();
     }
 
     private Future<MessageConsumer> getOrCreateMappingAndDelegatingCommandConsumer(final String tenantId) {

diff --git a/client/src/test/java/org/eclipse/hono/client/impl/DeviceConnectionClientImplTest.java b/client/src/test/java/org/eclipse/hono/client/impl/DeviceConnectionClientImplTest.java
@@ -183,9 +183,10 @@ public void testRemoveCommandHandlingAdapterInstance(final VertxTestContext ctx)
 
         // WHEN removing the command handling adapter instance
         client.removeCommandHandlingAdapterInstance("deviceId", "gatewayId", span.context())
-                .setHandler(ctx.succeeding(r -> {
+                .setHandler(ctx.succeeding(result -> {
                     ctx.verify(() -> {
                         // THEN the response has been handled and the span is finished
+                        assertThat(result).isTrue();
                         verify(span).finish();
                     });
                     ctx.completeNow();
@@ -334,6 +335,34 @@ public void testRemoveCommandHandlingAdapterInstanceFailsWithSendError(final Ver
                 }));
     }
 
+    /**
+     * Verifies that a client invocation of the <em>remove-cmd-handling-adapter-instance</em> operation
+     * returns a <em>Boolean.FALSE</em> value if a <em>NOT_FOUND</em> response was returned.
+     *
+     * @param ctx The vert.x test context.
+     */
+    @Test
+    public void testRemoveCommandHandlingAdapterInstanceForNotFoundEntry(final VertxTestContext ctx) {
+
+        // WHEN removing the command handling adapter instance
+        client.removeCommandHandlingAdapterInstance("deviceId", "gatewayId", span.context())
+                .setHandler(ctx.succeeding(result -> {
+                    ctx.verify(() -> {
+                        // THEN the response has been handled and the span is finished
+                        assertThat(result).isFalse();
+                        verify(span).finish();
+                    });
+                    ctx.completeNow();
+                }));
+
+        final Message sentMessage = verifySenderSend();
+        final Message response = ProtonHelper.message();
+        MessageHelper.addProperty(response, MessageHelper.APP_PROPERTY_STATUS, HttpURLConnection.HTTP_NOT_FOUND);
+        MessageHelper.addCacheDirective(response, CacheDirective.maxAgeDirective(60));
+        response.setCorrelationId(sentMessage.getMessageId());
+        client.handleResponse(mock(ProtonDelivery.class), response);
+    }
+
     /**
      * Verifies that a client invocation of the <em>get-cmd-handling-adapter-instances</em> operation fails
      * if the device connection service cannot be reached.

diff --git a/...test/java/org/eclipse/hono/client/impl/ProtocolAdapterCommandConsumerFactoryImplTest.java b/...test/java/org/eclipse/hono/client/impl/ProtocolAdapterCommandConsumerFactoryImplTest.java
@@ -135,8 +135,10 @@ public void setUp() {
         when(deviceConnectionClientFactory.connect()).thenReturn(Future.succeededFuture(mock(HonoConnection.class)));
         when(deviceConnectionClientFactory.getOrCreateDeviceConnectionClient(anyString()))
                 .thenReturn(Future.succeededFuture(devConClient));
-        when(devConClient.setCommandHandlingAdapterInstance(anyString(), anyString(), any(), any())).thenReturn(Future.succeededFuture());
-        when(devConClient.removeCommandHandlingAdapterInstance(anyString(), anyString(), any())).thenReturn(Future.succeededFuture());
+        when(devConClient.setCommandHandlingAdapterInstance(anyString(), anyString(), any(), any()))
+                .thenReturn(Future.succeededFuture());
+        when(devConClient.removeCommandHandlingAdapterInstance(anyString(), anyString(), any()))
+                .thenReturn(Future.succeededFuture(Boolean.TRUE));
 
         commandConsumerFactory = new ProtocolAdapterCommandConsumerFactoryImpl(connection);
         commandConsumerFactory.initialize(commandTargetMapper, deviceConnectionClientFactory);