From ee2ed0e951e873d16285939788f4e0c16e6f75b9 Mon Sep 17 00:00:00 2001 From: subratadeypappu Date: Wed, 26 Apr 2023 22:52:08 +0600 Subject: [PATCH] Fix stringified JSON response (#22450) (#22498) ## Description TL;DR This commit fixes unexpected stringified JSON in the following plugins - REST API Plugin - GraphQL plugin The detailed analysis can be found in the comment section of the issue itself (#22450) Slack Conversations - [1](https://theappsmith.slack.com/archives/C0341RERY4R/p1681123872154409) - [2](https://theappsmith.slack.com/archives/CGBPVEJ5C/p1681726446635249) > Add a TL;DR when description is extra long (helps content team) Fixes #22450 Media ## Type of change - Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? - Manual ### Test Plan > Add Testsmith test cases links that relate to this PR ### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) ## Checklist: ### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag ### QA activity: - [ ] Test plan has been approved by relevant developers - [ ] Test plan has been peer reviewed by QA - [ ] Cypress test cases have been added and approved by either SDET or manual QA - [ ] Organized project review call with relevant stakeholders after Round 1/2 of QA - [ ] Added Test Plan Approved label after reveiwing all Cypress test --- .../ServerSideTests/ApiTests/API_Bugs_Spec.js | 11 ++------ .../com/external/plugins/GraphQLPlugin.java | 9 ------ .../com/external/plugins/RestApiPlugin.java | 11 -------- .../external/plugins/RestApiPluginTest.java | 28 +++++++++++++++++++ 4 files changed, 31 insertions(+), 28 deletions(-) diff --git a/app/client/cypress/integration/Regression_TestSuite/ServerSideTests/ApiTests/API_Bugs_Spec.js b/app/client/cypress/integration/Regression_TestSuite/ServerSideTests/ApiTests/API_Bugs_Spec.js index 165fa40b00..61e20ab206 100644 --- a/app/client/cypress/integration/Regression_TestSuite/ServerSideTests/ApiTests/API_Bugs_Spec.js +++ b/app/client/cypress/integration/Regression_TestSuite/ServerSideTests/ApiTests/API_Bugs_Spec.js @@ -176,17 +176,12 @@ describe("Rest Bugs tests", function () { ); cy.ResponseStatusCheck("404 NOT_FOUND"); cy.get(commonlocators.errorTab).should("be.visible").click({ force: true }); + cy.get(commonlocators.debuggerToggle).click(); + cy.wait(1000); cy.get(commonlocators.debuggerLabel) .invoke("text") .then(($text) => { - expect($text).to.eq("API execution error"); - }); - cy.get(commonlocators.debuggerToggle).click(); - cy.wait(1000); - cy.get(commonlocators.debuggerDownStreamErrCode) - .invoke("text") - .then(($text) => { - expect($text).to.eq("[404 NOT_FOUND]"); + expect($text).contains("Not found"); }); }); diff --git a/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java b/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java index eb60e48f95..d63a23470c 100644 --- a/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java +++ b/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java @@ -281,15 +281,6 @@ public class GraphQLPlugin extends BasePlugin { client, httpMethod, uri, requestBodyObj, actionExecutionRequest, objectMapper, hintMessages, errorResult, requestCaptureFilter ) - .map(actionExecutionResult -> { - if (! actionExecutionResult.getIsExecutionSuccess()) { - actionExecutionResult.setErrorInfo(new AppsmithPluginException(GraphQLPluginError.QUERY_EXECUTION_FAILED, - GraphQLErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG, - actionExecutionResult.getBody(), - actionExecutionResult.getStatusCode() )); - } - return actionExecutionResult; - }) .onErrorResume(error -> { errorResult.setRequest(requestCaptureFilter.populateRequestFields(actionExecutionRequest)); errorResult.setIsExecutionSuccess(false); diff --git a/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java b/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java index d4da509604..15088168cf 100644 --- a/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java +++ b/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java @@ -182,17 +182,6 @@ public class RestApiPlugin extends BasePlugin { client, httpMethod, uri, requestBodyObj, actionExecutionRequest, objectMapper, hintMessages, errorResult, requestCaptureFilter ) - .map(actionExecutionResult -> { - if (! actionExecutionResult.getIsExecutionSuccess()) { - actionExecutionResult.setErrorInfo( - new AppsmithPluginException(RestApiPluginError.API_EXECUTION_FAILED, - RestApiErrorMessages.API_EXECUTION_FAILED_ERROR_MSG, - actionExecutionResult.getBody(), - actionExecutionResult.getStatusCode() - )); - } - return actionExecutionResult; - }) .onErrorResume(error -> { errorResult.setRequest(requestCaptureFilter.populateRequestFields(actionExecutionRequest)); errorResult.setIsExecutionSuccess(false); diff --git a/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java b/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java index 6879928d7a..90bd6b5e05 100644 --- a/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java +++ b/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java @@ -1872,5 +1872,33 @@ public class RestApiPluginTest { .filter(appErrorCode -> appErrorCode.length() != 11 || !appErrorCode.startsWith("PE-RST")).count()); } + + @Test + public void whenAPIReturnsNon200_doNotStringifyResponseBody() throws IOException { + // Generate a mock response which generates a non 200 HTTP status code e.g. HTTP 500 + MockWebServer mockWebServer = new MockWebServer(); + MockResponse mockRedirectResponse = new MockResponse() + .setResponseCode(500) + .addHeader("Content-Type", "application/json") + .setBody("{\"statusCode\":500,\"description\":\"Internal Server Error\"}"); + mockWebServer.enqueue(mockRedirectResponse); + + mockWebServer.start(); + + HttpUrl mockHttpUrl = mockWebServer.url("/mock.codes/500"); + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + dsConfig.setUrl(mockHttpUrl.toString()); + + ActionConfiguration actionConfig = new ActionConfiguration(); + actionConfig.setHttpMethod(HttpMethod.GET); + + Mono resultMono = pluginExecutor.executeParameterized(null, new ExecuteActionDTO(), dsConfig, actionConfig); + StepVerifier.create(resultMono) + .assertNext(result -> { + assertFalse(result.getIsExecutionSuccess()); + assertTrue(result.getBody() instanceof JsonNode); + }) + .verifyComplete(); + } }