Merge pull request #433 from tbak/master

1.x Fix error reporting for status update/delete REST resource (#422).
Netflix · Feb 23, 2015 · 2f656f8 · 2f656f8
2 parents a7ca132 + a0b8396
commit 2f656f8
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 3 deletions.
diff --git a/eureka-core/src/main/java/com/netflix/eureka/InstanceRegistry.java b/eureka-core/src/main/java/com/netflix/eureka/InstanceRegistry.java
@@ -956,7 +956,9 @@ public InstanceInfo getInstanceByAppAndId(String appName, String id,
         } else if (includeRemoteRegions) {
             for (RemoteRegionRegistry remoteRegistry : this.regionNameVSRemoteRegistry.values()) {
                 Application application = remoteRegistry.getApplication(appName);
-                return application.getByInstanceId(id);
+                if(application != null) {
+                    return application.getByInstanceId(id);
+                }
             }
         }
         return null;

diff --git a/eureka-core/src/main/java/com/netflix/eureka/resources/InstanceResource.java b/eureka-core/src/main/java/com/netflix/eureka/resources/InstanceResource.java
@@ -173,6 +173,10 @@ public Response statusUpdate(
             @HeaderParam(PeerEurekaNode.HEADER_REPLICATION) String isReplication,
             @QueryParam("lastDirtyTimestamp") String lastDirtyTimestamp) {
         try {
+            if(registry.getInstanceByAppAndId(app.getName(), id) == null) {
+                logger.warn("Instance not found: {}/{}", app.getName(), id);
+                return Response.status(Status.NOT_FOUND).build();
+            }
             boolean isSuccess = registry.statusUpdate(app.getName(), id,
                     InstanceStatus.valueOf(newStatus), lastDirtyTimestamp,
                     "true".equals(isReplication));
@@ -184,7 +188,7 @@ public Response statusUpdate(
             } else {
                 logger.warn("Unable to update status: " + app.getName() + " - "
                         + id + " - " + newStatus);
-                return Response.status(Status.NOT_ACCEPTABLE).build();
+                return Response.serverError().build();
             }
         } catch (Throwable e) {
             logger.error("Error updating instance {} for status {}", id,
@@ -212,6 +216,11 @@ public Response deleteStatusUpdate(
             @QueryParam("value") String newStatusValue,
             @QueryParam("lastDirtyTimestamp") String lastDirtyTimestamp) {
         try {
+            if(registry.getInstanceByAppAndId(app.getName(), id) == null) {
+                logger.warn("Instance not found: {}/{}", app.getName(), id);
+                return Response.status(Status.NOT_FOUND).build();
+            }
+
             InstanceStatus newStatus = newStatusValue == null ? InstanceStatus.UNKNOWN : InstanceStatus.valueOf(newStatusValue);
             boolean isSuccess = registry.deleteStatusOverride(app.getName(), id,
                     newStatus, lastDirtyTimestamp, "true".equals(isReplication));
@@ -221,7 +230,7 @@ public Response deleteStatusUpdate(
                 return Response.ok().build();
             } else {
                 logger.warn("Unable to remove status override: " + app.getName() + " - " + id);
-                return Response.status(Status.NOT_ACCEPTABLE).build();
+                return Response.serverError().build();
             }
         } catch (Throwable e) {
             logger.error("Error removing instance's {} status override", id);

diff --git a/eureka-core/src/test/java/com/netflix/eureka/resources/InstanceResourceTest.java b/eureka-core/src/test/java/com/netflix/eureka/resources/InstanceResourceTest.java
@@ -1,6 +1,7 @@
 package com.netflix.eureka.resources;
 
 import javax.ws.rs.core.Response;
+import javax.ws.rs.core.Response.Status;
 
 import com.netflix.appinfo.InstanceInfo;
 import com.netflix.appinfo.InstanceInfo.InstanceStatus;
@@ -26,6 +27,18 @@ public void setUp() throws Exception {
         instanceResource = new InstanceResource(applicationResource, testInstanceInfo.getId(), registry);
     }
 
+    @Test
+    public void testStatusOverrideReturnsNotFoundErrorCodeIfInstanceNotRegistered() throws Exception {
+        Response response = instanceResource.statusUpdate(InstanceStatus.OUT_OF_SERVICE.name(), "false", "0");
+        assertThat(response.getStatus(), is(equalTo(Status.NOT_FOUND.getStatusCode())));
+    }
+
+    @Test
+    public void testStatusOverrideDeleteReturnsNotFoundErrorCodeIfInstanceNotRegistered() throws Exception {
+        Response response = instanceResource.deleteStatusUpdate(InstanceStatus.OUT_OF_SERVICE.name(), "false", "0");
+        assertThat(response.getStatus(), is(equalTo(Status.NOT_FOUND.getStatusCode())));
+    }
+
     @Test
     public void testStatusOverrideDeleteIsAppliedToRegistry() throws Exception {
         // Override instance status