From bd02375be76398a1eb036f63a85790ac67736395 Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Thu, 14 Dec 2023 18:56:15 -0800 Subject: [PATCH 1/3] Always consume the workflow_id param Signed-off-by: Daniel Widdis --- .../flowframework/rest/RestCreateWorkflowAction.java | 3 +-- .../flowframework/rest/RestDeprovisionWorkflowAction.java | 3 +-- .../opensearch/flowframework/rest/RestGetWorkflowAction.java | 3 +-- .../flowframework/rest/RestGetWorkflowStateAction.java | 3 +-- 4 files changed, 4 insertions(+), 8 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/RestDeprovisionWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java index 467a683ce..ad9587cac 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( @@ -70,7 +70,6 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request throw new FlowFrameworkException("No request body is required", RestStatus.BAD_REQUEST); } // 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..6dc271dbc 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( @@ -76,7 +76,6 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request throw new FlowFrameworkException("Invalid request format", RestStatus.BAD_REQUEST); } // 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/RestGetWorkflowStateAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java index ab7335b2d..1118c185d 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( @@ -71,7 +71,6 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request throw new FlowFrameworkException("No request body present", RestStatus.BAD_REQUEST); } // Validate params - String workflowId = request.param(WORKFLOW_ID); if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); } From 616739e3639143231020db09b76fa5765072990c Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Thu, 14 Dec 2023 19:08:39 -0800 Subject: [PATCH 2/3] Delegate no-content error message to BaseRestHandler Signed-off-by: Daniel Widdis --- .../flowframework/rest/RestDeleteWorkflowAction.java | 3 ++- .../flowframework/rest/RestDeprovisionWorkflowAction.java | 3 ++- .../flowframework/rest/RestGetWorkflowAction.java | 4 ++-- .../flowframework/rest/RestGetWorkflowStateAction.java | 3 ++- .../flowframework/rest/RestProvisionWorkflowAction.java | 3 ++- .../rest/RestGetWorkflowStateActionTests.java | 8 ++++---- 6 files changed, 14 insertions(+), 10 deletions(-) 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 ad9587cac..bfd5c70d4 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java @@ -67,7 +67,8 @@ 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 if (workflowId == null) { diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java index 6dc271dbc..edcc2d612 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java @@ -70,10 +70,10 @@ 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 if (workflowId == null) { diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java index 1118c185d..e49b8eb8e 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java @@ -68,7 +68,8 @@ 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 if (workflowId == null) { 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); From e8fba4262872afa81bb59856db515228f1b264ed Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Thu, 14 Dec 2023 19:17:45 -0800 Subject: [PATCH 3/3] Don't lose FlowFrameworkException status Signed-off-by: Daniel Widdis --- .../opensearch/flowframework/rest/RestGetWorkflowAction.java | 4 +++- .../flowframework/rest/RestGetWorkflowStateAction.java | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java index edcc2d612..93ea6d134 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java @@ -86,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 e49b8eb8e..20f8d69b7 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java @@ -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));