From fae80a583082d3a9d7fdd9d1c8d47b5332f77209 Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Fri, 15 Dec 2023 09:49:02 -0800 Subject: [PATCH] Consume REST params and consistently handle error messages (#295) * Always consume the workflow_id param Signed-off-by: Daniel Widdis * Delegate no-content error message to BaseRestHandler Signed-off-by: Daniel Widdis * Don't lose FlowFrameworkException status Signed-off-by: Daniel Widdis --------- Signed-off-by: Daniel Widdis --- .../flowframework/rest/RestCreateWorkflowAction.java | 3 +-- .../flowframework/rest/RestDeleteWorkflowAction.java | 3 ++- .../rest/RestDeprovisionWorkflowAction.java | 6 +++--- .../flowframework/rest/RestGetWorkflowAction.java | 11 ++++++----- .../rest/RestGetWorkflowStateAction.java | 10 ++++++---- .../rest/RestProvisionWorkflowAction.java | 3 ++- .../rest/RestGetWorkflowStateActionTests.java | 8 ++++---- 7 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java index deeabdd76..5d8aed031 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java @@ -78,6 +78,7 @@ public List routes() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + String workflowId = request.param(WORKFLOW_ID); if (!flowFrameworkFeatureEnabledSetting.isFlowFrameworkEnabled()) { FlowFrameworkException ffe = new FlowFrameworkException( "This API is disabled. To enable it, set [" + FLOW_FRAMEWORK_ENABLED.getKey() + "] to true.", @@ -88,8 +89,6 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli ); } try { - - String workflowId = request.param(WORKFLOW_ID); Template template = Template.parse(request.content().utf8ToString()); boolean dryRun = request.paramAsBoolean(DRY_RUN, false); boolean provision = request.paramAsBoolean(PROVISION_WORKFLOW, false); diff --git a/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java index e017ee581..cd0672e62 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java @@ -72,7 +72,8 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request } // Validate content if (request.hasContent()) { - throw new FlowFrameworkException("Invalid request format", RestStatus.BAD_REQUEST); + // BaseRestHandler will give appropriate error message + return channel -> channel.sendResponse(null); } // Validate params if (workflowId == null) { diff --git a/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java index 467a683ce..bfd5c70d4 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java @@ -57,7 +57,7 @@ public String getName() { @Override protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - + String workflowId = request.param(WORKFLOW_ID); try { if (!flowFrameworkFeatureEnabledSetting.isFlowFrameworkEnabled()) { throw new FlowFrameworkException( @@ -67,10 +67,10 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request } // Validate content if (request.hasContent()) { - throw new FlowFrameworkException("No request body is required", RestStatus.BAD_REQUEST); + // BaseRestHandler will give appropriate error message + return channel -> channel.sendResponse(null); } // Validate params - String workflowId = request.param(WORKFLOW_ID); if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); } diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java index 5a92e9c0e..93ea6d134 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java @@ -62,7 +62,7 @@ public List routes() { @Override protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - + String workflowId = request.param(WORKFLOW_ID); try { if (!flowFrameworkFeatureEnabledSetting.isFlowFrameworkEnabled()) { throw new FlowFrameworkException( @@ -70,13 +70,12 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request RestStatus.FORBIDDEN ); } - // Validate content if (request.hasContent()) { - throw new FlowFrameworkException("Invalid request format", RestStatus.BAD_REQUEST); + // BaseRestHandler will give appropriate error message + return channel -> channel.sendResponse(null); } // Validate params - String workflowId = request.param(WORKFLOW_ID); if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); } @@ -87,7 +86,9 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); }, exception -> { try { - FlowFrameworkException ex = new FlowFrameworkException(exception.getMessage(), ExceptionsHelper.status(exception)); + FlowFrameworkException ex = exception instanceof FlowFrameworkException + ? (FlowFrameworkException) exception + : new FlowFrameworkException(exception.getMessage(), ExceptionsHelper.status(exception)); XContentBuilder exceptionBuilder = ex.toXContent(channel.newErrorBuilder(), ToXContent.EMPTY_PARAMS); channel.sendResponse(new BytesRestResponse(ex.getRestStatus(), exceptionBuilder)); diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java index ab7335b2d..20f8d69b7 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java @@ -57,7 +57,7 @@ public String getName() { @Override protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - + String workflowId = request.param(WORKFLOW_ID); try { if (!flowFrameworkFeatureEnabledSetting.isFlowFrameworkEnabled()) { throw new FlowFrameworkException( @@ -68,10 +68,10 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request // Validate content if (request.hasContent()) { - throw new FlowFrameworkException("No request body present", RestStatus.BAD_REQUEST); + // BaseRestHandler will give appropriate error message + return channel -> channel.sendResponse(null); } // Validate params - String workflowId = request.param(WORKFLOW_ID); if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); } @@ -83,7 +83,9 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); }, exception -> { try { - FlowFrameworkException ex = new FlowFrameworkException(exception.getMessage(), ExceptionsHelper.status(exception)); + FlowFrameworkException ex = exception instanceof FlowFrameworkException + ? (FlowFrameworkException) exception + : new FlowFrameworkException(exception.getMessage(), ExceptionsHelper.status(exception)); XContentBuilder exceptionBuilder = ex.toXContent(channel.newErrorBuilder(), ToXContent.EMPTY_PARAMS); channel.sendResponse(new BytesRestResponse(ex.getRestStatus(), exceptionBuilder)); diff --git a/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java index 9e6eb4d01..7e4d68183 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java @@ -77,7 +77,8 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli } // Validate content if (request.hasContent()) { - throw new FlowFrameworkException("Invalid request format", RestStatus.BAD_REQUEST); + // BaseRestHandler will give appropriate error message + return channel -> channel.sendResponse(null); } // Validate params if (workflowId == null) { diff --git a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java index dc605a5cd..06c4a7053 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java @@ -63,7 +63,7 @@ public void testRestGetWorkflowStateActionRoutes() { public void testNullWorkflowId() throws Exception { // Request with no params - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.POST) + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) .withPath(this.getPath) .build(); @@ -76,7 +76,7 @@ public void testNullWorkflowId() throws Exception { } public void testInvalidRequestWithContent() { - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.POST) + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) .withPath(this.getPath) .withContent(new BytesArray("request body"), MediaTypeRegistry.JSON) .build(); @@ -86,14 +86,14 @@ public void testInvalidRequestWithContent() { restGetWorkflowStateAction.handleRequest(request, channel, nodeClient); }); assertEquals( - "request [POST /_plugins/_flow_framework/workflow/{workflow_id}/_status] does not support having a body", + "request [GET /_plugins/_flow_framework/workflow/{workflow_id}/_status] does not support having a body", ex.getMessage() ); } public void testFeatureFlagNotEnabled() throws Exception { when(flowFrameworkFeatureEnabledSetting.isFlowFrameworkEnabled()).thenReturn(false); - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.POST) + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) .withPath(this.getPath) .build(); FakeRestChannel channel = new FakeRestChannel(request, false, 1);