diff options
author | Jon Bratseth <bratseth@oath.com> | 2019-04-29 11:41:50 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-04-29 11:41:50 +0200 |
commit | d26962c2102e9b38004035bb9876326d1c92722f (patch) | |
tree | 420b0c331590f6de40d0a2bdca8749f871236dd7 /vespaclient-container-plugin | |
parent | c9f9f467df69a65fe1a403f58411259e3c0fa823 (diff) | |
parent | 8cbc4e04c8caf10222e00e203cc8e0b1ea569087 (diff) |
Merge pull request #9220 from vespa-engine/bratseth/document-api-dont-blame-users
Bratseth/document api dont blame users
Diffstat (limited to 'vespaclient-container-plugin')
2 files changed, 57 insertions, 50 deletions
diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java index 873b0569553..b4992fb519b 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java @@ -36,6 +36,7 @@ import com.yahoo.vespaclient.ClusterList; import com.yahoo.vespaxmlparser.DocumentFeedOperation; import com.yahoo.vespaxmlparser.FeedOperation; import com.yahoo.vespaxmlparser.VespaXMLFeedReader; +import com.yahoo.yolean.Exceptions; import java.io.IOException; import java.io.OutputStream; @@ -79,16 +80,14 @@ public class RestApi extends LoggingRequestHandler { public RestApi(LoggingRequestHandler.Context parentCtx, DocumentmanagerConfig documentManagerConfig, LoadTypeConfig loadTypeConfig, ThreadpoolConfig threadpoolConfig, AllClustersBucketSpacesConfig bucketSpacesConfig, - ClusterListConfig clusterListConfig, MetricReceiver metricReceiver) - { + ClusterListConfig clusterListConfig, MetricReceiver metricReceiver) { super(parentCtx); MessageBusParams params = new MessageBusParams(new LoadTypeSet(loadTypeConfig)); params.setDocumentmanagerConfig(documentManagerConfig); - this.operationHandler = new OperationHandlerImpl( - new MessageBusDocumentAccess(params), - fixedClusterEnumeratorFromConfig(clusterListConfig), - fixedBucketSpaceResolverFromConfig(bucketSpacesConfig), - metricReceiver); + this.operationHandler = new OperationHandlerImpl(new MessageBusDocumentAccess(params), + fixedClusterEnumeratorFromConfig(clusterListConfig), + fixedBucketSpaceResolverFromConfig(bucketSpacesConfig), + metricReceiver); this.singleDocumentParser = new SingleDocumentParser(new DocumentTypeManager(documentManagerConfig)); // 40% of the threads can be blocked before we deny requests. if (threadpoolConfig != null) { @@ -100,11 +99,10 @@ public class RestApi extends LoggingRequestHandler { } // For testing and development - public RestApi( - Executor executor, - AccessLog accessLog, - OperationHandler operationHandler, - int threadsAvailable) { + public RestApi(Executor executor, + AccessLog accessLog, + OperationHandler operationHandler, + int threadsAvailable) { super(executor, accessLog, null); this.operationHandler = operationHandler; this.threadsAvailableForApi = new AtomicInteger(threadsAvailable); @@ -121,12 +119,11 @@ public class RestApi extends LoggingRequestHandler { } private static OperationHandlerImpl.ClusterEnumerator fixedClusterEnumeratorFromConfig(ClusterListConfig config) { - final List<ClusterDef> clusters = Collections.unmodifiableList(new ClusterList(config).getStorageClusters()); + List<ClusterDef> clusters = Collections.unmodifiableList(new ClusterList(config).getStorageClusters()); return () -> clusters; } - private static OperationHandlerImpl.BucketSpaceResolver fixedBucketSpaceResolverFromConfig( - AllClustersBucketSpacesConfig bucketSpacesConfig) { + private static OperationHandlerImpl.BucketSpaceResolver fixedBucketSpaceResolverFromConfig(AllClustersBucketSpacesConfig bucketSpacesConfig) { return (clusterId, docType) -> Optional.ofNullable(bucketSpacesConfig.cluster(clusterId)) .map(cluster -> cluster.documentType(docType)) @@ -134,7 +131,7 @@ public class RestApi extends LoggingRequestHandler { } private static Optional<String> requestProperty(String parameter, HttpRequest request) { - final String property = request.getProperty(parameter); + String property = request.getProperty(parameter); if (property != null && ! property.isEmpty()) { return Optional.of(property); } @@ -151,8 +148,15 @@ public class RestApi extends LoggingRequestHandler { } private static Optional<Boolean> parseBoolean(String parameter, HttpRequest request) { - Optional<String> property = requestProperty(parameter, request); - return property.map(RestApi::parseBooleanStrict); + try { + Optional<String> property = requestProperty(parameter, request); + return property.map(RestApi::parseBooleanStrict); + } + catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Invalid value for '" + parameter + "' parameter: " + + "Must be empty, true, or false but was '" + + request.getProperty(parameter) + "'"); + } } private static int parsePositiveInt(String str) throws NumberFormatException { @@ -167,9 +171,9 @@ public class RestApi extends LoggingRequestHandler { public HttpResponse handle(HttpRequest request) { try { if (threadsAvailableForApi.decrementAndGet() < 1) { - return Response.createErrorResponse( - 429 /* Too Many Requests */, - "Too many parallel requests, consider using http-vespa-java-client. Please try again later.", RestUri.apiErrorCodes.TOO_MANY_PARALLEL_REQUESTS); + return Response.createErrorResponse(429 /* Too Many Requests */, + "Too many parallel requests, consider using http-vespa-java-client. Please try again later.", + RestUri.apiErrorCodes.TOO_MANY_PARALLEL_REQUESTS); } return handleInternal(request); } finally { @@ -177,11 +181,12 @@ public class RestApi extends LoggingRequestHandler { } } - private static void validateUriStructureForRequestMethod(RestUri uri, com.yahoo.jdisc.http.HttpRequest.Method method) throws RestApiException { + private static void validateUriStructureForRequestMethod(RestUri uri, com.yahoo.jdisc.http.HttpRequest.Method method) + throws RestApiException { if ((method != com.yahoo.jdisc.http.HttpRequest.Method.GET) && uri.isRootOnly()) { throw new RestApiException(Response.createErrorResponse(BAD_REQUEST, - "Root /document/v1/ requests only supported for HTTP GET", - RestUri.apiErrorCodes.ERROR_ID_BASIC_USAGE)); + "Root /document/v1/ requests only supported for HTTP GET", + RestUri.apiErrorCodes.ERROR_ID_BASIC_USAGE)); } } @@ -191,28 +196,23 @@ public class RestApi extends LoggingRequestHandler { // protected for testing protected HttpResponse handleInternal(HttpRequest request) { - final RestUri restUri; + RestUri restUri = null; try { restUri = new RestUri(request.getUri()); validateUriStructureForRequestMethod(restUri, request.getMethod()); - } catch (RestApiException e) { - return e.getResponse(); - } catch (Exception e2) { - return Response.createErrorResponse(500, "Exception while parsing URI: " + e2.getMessage(), RestUri.apiErrorCodes.URL_PARSING); - } - final Optional<Boolean> create; - try { - create = parseBoolean(CREATE_PARAMETER_NAME, request); - } catch (IllegalArgumentException e) { - return Response.createErrorResponse(400, "Non valid value for 'create' parameter, must be empty, true, or " + - "false: " + request.getProperty(CREATE_PARAMETER_NAME), RestUri.apiErrorCodes.INVALID_CREATE_VALUE); - } - String condition = request.getProperty(CONDITION_PARAMETER_NAME); - Optional<String> route = Optional.ofNullable(request.getProperty(ROUTE_PARAMETER_NAME)); + Optional<Boolean> create; + try { + create = parseBoolean(CREATE_PARAMETER_NAME, request); + } + catch (IllegalArgumentException e) { + return Response.createErrorResponse(400, e.getMessage(), RestUri.apiErrorCodes.INVALID_CREATE_VALUE); + } - Optional<ObjectNode> resultJson = Optional.empty(); - try { + String condition = request.getProperty(CONDITION_PARAMETER_NAME); + Optional<String> route = Optional.ofNullable(request.getProperty(ROUTE_PARAMETER_NAME)); + + Optional<ObjectNode> resultJson = Optional.empty(); switch (request.getMethod()) { case GET: // Vespa Visit/Get return isVisitRequestUri(restUri) ? handleVisit(restUri, request) : handleGet(restUri, request); @@ -228,14 +228,21 @@ public class RestApi extends LoggingRequestHandler { default: return new Response(405, Optional.empty(), Optional.of(restUri)); } - } catch (RestApiException e) { + return new Response(200, resultJson, Optional.of(restUri)); + } + catch (RestApiException e) { return e.getResponse(); - } catch (Exception e2) { - // We always blame the user. This might be a bit nasty, but the parser throws various kind of exception - // types, but with nice descriptions. - return Response.createErrorResponse(400, e2.getMessage(), restUri, RestUri.apiErrorCodes.PARSER_ERROR); } - return new Response(200, resultJson, Optional.of(restUri)); + catch (IllegalArgumentException userException) { + return Response.createErrorResponse(400, Exceptions.toMessageString(userException), + restUri, + RestUri.apiErrorCodes.PARSER_ERROR); + } + catch (RuntimeException systemException) { + return Response.createErrorResponse(500, Exceptions.toMessageString(systemException), + restUri, + RestUri.apiErrorCodes.UNSPECIFIED); + } } private FeedOperation createPutOperation(HttpRequest request, String id, String condition) { @@ -401,8 +408,8 @@ public class RestApi extends LoggingRequestHandler { } catch (BadRequestParameterException e) { return createInvalidParameterResponse(e.getParameter(), e.getMessage()); } - final OperationHandler.VisitResult visit = operationHandler.visit(restUri, documentSelection, options); - final ObjectNode resultNode = mapper.createObjectNode(); + OperationHandler.VisitResult visit = operationHandler.visit(restUri, documentSelection, options); + ObjectNode resultNode = mapper.createObjectNode(); visit.token.ifPresent(t -> resultNode.put(CONTINUATION, t)); resultNode.putArray(DOCUMENTS).addPOJO(visit.documentsAsJsonList); resultNode.put(PATH_NAME, restUri.getRawPath()); diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java index 94bc8deb47a..773d45b6b18 100644 --- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java +++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java @@ -197,7 +197,7 @@ public class RestApiTest { HttpPut httpPut = new HttpPut(request.getUri()); StringEntity entity = new StringEntity(update_test_doc, ContentType.create("application/json")); httpPut.setEntity(entity); - assertHttp400ResponseContains(doRest(httpPut), "Non valid value for 'create' parameter, must be empty, true, or false: batman"); + assertHttp400ResponseContains(doRest(httpPut), "Invalid value for 'create' parameter: Must be empty, true, or false but was 'batman'"); } // Get logs through some hackish fetch method. Logs is something the mocked backend write. |