Consume REST params and consistently handle error messages (opensearc…

…h-project#295) * Always consume the workflow_id param Signed-off-by: Daniel Widdis <widdis@gmail.com> * Delegate no-content error message to BaseRestHandler Signed-off-by: Daniel Widdis <widdis@gmail.com> * Don't lose FlowFrameworkException status Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com>
dbwiddis · Dec 15, 2023 · bb29302 · bb29302
1 parent ce72cf1
commit bb29302
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 20 deletions.
diff --git 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<Route> 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
@@ -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
@@ -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
@@ -62,21 +62,20 @@ public List<Route> 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(
                     "This API is disabled. To enable it, update the setting [" + FLOW_FRAMEWORK_ENABLED.getKey() + "] to true.",
                     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
@@ -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
@@ -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
@@ -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);