Merge pull request #18168 from vespa-engine/jonmv/vespa-feed-client

Document API errors are BAD_GATEWAY

2 files changed, 23 insertions, 8 deletions

diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
index 58857d1d8e6..789a2ab537f 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
@@ -743,11 +743,18 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
 
     private static void serverError(HttpRequest request, Throwable t, ResponseHandler handler) {
         loggingException(() -> {
-            log.log(WARNING, "Uncaught exception handling request " + request.getMethod() + " " + request.getUri().getRawPath() + ":", t);
+            log.log(WARNING, "Uncaught exception handling request " + request.getMethod() + " " + request.getUri().getRawPath(), t);
             JsonResponse.create(request, Exceptions.toMessageString(t), handler).respond(Response.Status.INTERNAL_SERVER_ERROR);
         });
     }
 
+    private static void badGateway(HttpRequest request, Throwable t, ResponseHandler handler) {
+        loggingException(() -> {
+            log.log(FINE, t, () -> "Document access error handling request " + request.getMethod() + " " + request.getUri().getRawPath());
+            JsonResponse.create(request, Exceptions.toMessageString(t), handler).respond(Response.Status.BAD_GATEWAY);
+        });
+    }
+
     private static void timeout(HttpRequest request, String message, ResponseHandler handler) {
         loggingException(() -> {
             log.log(FINE, () -> "Timeout handling request " + request.getMethod() + " " + request.getUri().getRawPath() + ": " + message);
@@ -803,6 +810,9 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
             catch (IllegalArgumentException e) {
                 badRequest(request, e, handler);
             }
+            catch (DispatchException e) {
+                badGateway(request, e, handler);
+            }
             catch (RuntimeException e) {
                 serverError(request, e, handler);
             }
@@ -821,12 +831,16 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
             return false;
 
         if (result.type() == Result.ResultType.FATAL_ERROR)
-            throw new RuntimeException(result.getError());
+            throw new DispatchException(result.getError());
 
         outstanding.incrementAndGet();
         return true;
     }
 
+    private static class DispatchException extends RuntimeException {
+        private DispatchException(Throwable cause) { super(cause); }
+    }
+
     /** Readable content channel which forwards data to a reader when closed. */
     static class ForwardingContentChannel implements ContentChannel {
 
@@ -923,7 +937,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                         log.log(WARNING, "Unexpected document API operation outcome '" + response.outcome() + "'");
                     case ERROR:
                         log.log(FINE, () -> "Exception performing document operation: " + response.getTextMessage());
-                        jsonResponse.commit(Response.Status.INTERNAL_SERVER_ERROR);
+                        jsonResponse.commit(Response.Status.BAD_GATEWAY);
                 }
             }
         }
@@ -1118,7 +1132,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                                 if (getVisitorStatistics() != null)
                                     response.writeDocumentCount(getVisitorStatistics().getDocumentsReturned());
 
-                                response.respond(Response.Status.INTERNAL_SERVER_ERROR);
+                                response.respond(Response.Status.BAD_GATEWAY);
                         }
                     });
                     visitDispatcher.execute(() -> {
diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
index 6f1b0466350..a0f6fd45dd8 100644
--- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
+++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
@@ -330,6 +330,7 @@ public class DocumentV1ApiTest {
         assertEquals(400, response.getStatus());
 
         // DELETE with namespace and document type is a restricted visit which deletes visited documents.
+        // When visiting fails fatally, a 502 BAD GATEWAY is returned.
         access.expect(tokens.subList(0, 1));
         access.expect(parameters -> {
             assertEquals("(false) and (music) and (id.namespace=='space')", parameters.getDocumentSelection());
@@ -351,7 +352,7 @@ public class DocumentV1ApiTest {
                        "  \"pathId\": \"/document/v1/space/music/docid\"," +
                        "  \"message\": \"boom\"" +
                        "}", response.readAll());
-        assertEquals(500, response.getStatus());
+        assertEquals(502, response.getStatus());
 
         // DELETE at the root is also a deletion visit. These also require a selection.
         access.expect(parameters -> {
@@ -386,7 +387,7 @@ public class DocumentV1ApiTest {
                        "  \"documents\": []," +
                        "  \"message\": \"error\"" +
                        "}", response.readAll());
-        assertEquals(500, response.getStatus());
+        assertEquals(502, response.getStatus());
 
         // GET with namespace, document type and number is a restricted visit.
         access.expect(parameters -> {
@@ -649,12 +650,12 @@ public class DocumentV1ApiTest {
                        "  \"pathId\": \"/document/v1/space/music/number/1/two\"," +
                        "  \"message\": \"error\"" +
                        "}", response1.readAll());
-        assertEquals(500, response1.getStatus());
+        assertEquals(502, response1.getStatus());
         assertSameJson("{" +
                        "  \"pathId\": \"/document/v1/space/music/number/1/two\"," +
                        "  \"message\": \"error\"" +
                        "}", response2.readAll());
-        assertEquals(500, response2.getStatus());
+        assertEquals(502, response2.getStatus());
 
         // Request response does not arrive before timeout has passed.
         AtomicReference<ResponseHandler> handler = new AtomicReference<>();