Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: JSON response gets stringified by server #22450

Closed
1 task done
subrata71 opened this issue Apr 16, 2023 · 15 comments · Fixed by #22498
Closed
1 task done

[Bug]: JSON response gets stringified by server #22450

subrata71 opened this issue Apr 16, 2023 · 15 comments · Fixed by #22498
Assignees
Labels
Backend This marks the issue or pull request to reference server code BE Coders Pod Issues related to users writing code to fetch and update data Bug Something isn't working Critical This issue needs immediate attention. Drop everything else Data Platform Pod Issues related to the underlying data platform Needs Triaging Needs attention from maintainers to triage Verified When issue is retested post its fixed

Comments

@subrata71
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Description

Slack Conversation

Error in JSON format returned by users' API server gets stringified by Appsmith server.

Steps To Reproduce

  1. Create a REST API e.g. Api1
  2. Enter https://mock.codes/500 in the URI
  3. Drag and drop a text widget and use {{Api1.data.description}} in the Text field

You will notice that the evaluated value doesn't show anything because the response is coming as stringified.

Public Sample App

No response

Environment

Production

Issue video log

No response

Version

Cloud / Self Hosted

@subrata71 subrata71 added Bug Something isn't working Backend This marks the issue or pull request to reference server code Needs Triaging Needs attention from maintainers to triage REST API plugin REST API plugin related issues Error Handling labels Apr 16, 2023
@subrata71 subrata71 self-assigned this Apr 16, 2023
@github-actions github-actions bot added Integrations Pod Issues related to a specific integration Data Platform Pod Issues related to the underlying data platform BE Coders Pod Issues related to users writing code to fetch and update data labels Apr 16, 2023
@subrata71 subrata71 added the High This issue blocks a user from building or impacts a lot of users label Apr 16, 2023
subrata71 added a commit that referenced this issue Apr 18, 2023
This commit fixes unexpected stringified JSON in the following plugins
- REST API Plugin
- GraphQL plugin
- SaaS plugins (Airtable)
@subrata71 subrata71 added GraphQL Plugin Issues related to GraphQL plugin Airtable Issues for Airtable SAAS Plugins Issues related to SAAS Plugins labels Apr 18, 2023
@subrata71
Copy link
Contributor Author

Issue with Airtable

The data returned by Airtable is in valid JSON format but after deserialization happens in Appsmith the body in ActionExecutionResult contains stringified JSON.

cURL request shows that the response is in valid JSON format

curl --location 'https://api.airtable.com/v0/{baseID}/{tableName}' \
--header 'Authorization: Bearer API_KEY'

{
  "records": [
    {
      "id": "rec3dVlLB33uR0PEE",
      "createdTime": "2022-08-08T10:54:29.000Z",
      "fields": {
        "Name": "test patch",
        "Status": "Todo",
        "Notes": "nice"
      }
    },
    {
      "id": "rec7ZO0YX4wdQRt8Z",
      "createdTime": "2022-08-16T09:45:20.000Z",
      "fields": {
        "Name": "latest id ",
        "Status": "Todo",
        "Notes": "PRAPULLA12@#$%"
      }
    },
    {
      "id": "recB4rdvSYFrIWFsp",
      "createdTime": "2021-11-09T09:07:18.000Z",
      "fields": {
        "Name": "test patch",
        "Status": "Todo",
        "Notes": "nice"
      }
    },
    {
      "id": "recGuJsxd69IeUQ8g",
      "createdTime": "2022-08-08T10:53:09.000Z",
      "fields": {
        "Name": "test patch",
        "Status": "Todo",
        "Notes": "nice"
      }
    },
    {
      "id": "recMSLsSWhz6XLeYa",
      "createdTime": "2021-11-09T09:22:52.000Z",
      "fields": {
        "Name": "Closed",
        "Status": "In progress",
        "Notes": "hi"
      }
    },
    {
      "id": "recZIfsrHIAIEvHkD",
      "createdTime": "2021-11-09T09:07:18.000Z",
      "fields": {}
    },
    {
      "id": "recdfdrpukvoALby9",
      "createdTime": "2021-11-26T06:11:35.000Z",
      "fields": {
        "Name": "test",
        "Notes": "hi"
      }
    },
    {
      "id": "recjBOC8Scg4vTLgo",
      "createdTime": "2021-11-26T06:11:35.000Z",
      "fields": {
        "Name": "test",
        "Notes": "hi"
      }
    },
    {
      "id": "reckB49idYGo3hDdP",
      "createdTime": "2021-11-09T10:29:35.000Z",
      "fields": {
        "Name": "test",
        "Status": "Todo",
        "Notes": "hi"
      }
    },
    {
      "id": "recrPq8EST7K0Soo5",
      "createdTime": "2021-11-09T10:29:35.000Z",
      "fields": {
        "Name": "test",
        "Status": "Done",
        "Notes": "hi"
      }
    },
    {
      "id": "recraPx0luOphnfgM",
      "createdTime": "2021-11-09T09:22:52.000Z",
      "fields": {
        "Name": "Closed",
        "Status": "Done",
        "Notes": "hi"
      }
    }
  ]
}

Solution

The first deserialization is done by this piece of code

ActionExecutionResult result = saasObjectMapper.readValue(body, ActionExecutionResult.class);

Airtable query results can be found as stringified JSON in the body (type object) field of ActionExecutionResult object. The data type of the body field is checked with String and it's a string type that might contain stringified JSON. Later, the body is attempted to convert to a JSON node. In case it failed with a JSON processing error, string type is retained as the fallback type.

@subrata71
Copy link
Contributor Author

Issue with RestApi and GraphQL plugins

If the HTTP status code returned by the user's API server doesn't belong to 2xx then an error response is generated with plugin-specific code, Appsmith error message, and downstream error message. In this conversion, the actual response body gets stored in the downstreamErrorMessage field which is a string type. Thus a valid JSON response will get converted to stringified JSON if the HTTP status code doesn't belong to 2xx.

Solution

This extra piece of code is removed. Thus it ensures that the response returned by the user's API server will propagate to the front-end as is.

@subrata71 subrata71 added the Critical This issue needs immediate attention. Drop everything else label Apr 18, 2023
@subrata71
Copy link
Contributor Author

So many users have already reported their apps aren't working as expected. Either they need to use JSON.parse in all the occurrences or they have to wait for a fix.

Considering the above scenario I have added the critical label.

@subrata71 subrata71 removed the High This issue blocks a user from building or impacts a lot of users label Apr 18, 2023
@subrata71
Copy link
Contributor Author

Tested with Twilio

Found that the response body in Twilio is basically a Hashmap in Java after the deserialization happens. So in this case the following deserialization suffices.

ActionExecutionResult result = saasObjectMapper.readValue(body, ActionExecutionResult.class);

@subrata71
Copy link
Contributor Author

Regarding Airtable Issue

Looking at the source code it seems we haven't made any changes recently. The last change was made almost 2 years ago. The Cypress test cases are also written in a way so that it expects stringified JSON and therefore, it uses JSON.parse. Now if we make it a valid JSON from the backend before rendering it to the client, there's a chance it might break existing apps of the users that are built on the Airtable plugin. Because in order to access the data returned by the Airtable queries they must have used JSON.parse.

And JSON.parse expects stringified JSON as its input. If we provide a valid JSON to JSON.parse it will error out.

Risk

If make ship the changes regarding making the Airtable query result as valid JSON, that is going to be a breaking change for the aforementioned user group.

@lelesrc
Copy link

lelesrc commented Apr 19, 2023

Airtable user here (cloud).
The airtable integration stopped working with Appsmith v1.9.15-SNAPSHOT.

.data is now a string and not an object as before. But I'm pretty sure it was an object, since all the bindings in my app were working as query.data.fields[field_name] - and in my custom JS objects too.

It's true that if I JSON.parse .data I have the correct object, but it means replacing query.data with JSON.parse( query.data ) everywhere in my app (tens of instances)

Let's take a text field as an example. In my app I have something like {{ query.data.fields[field_name]}} and it used to work until v1.9.15-SNAPSHOT. I believe all airtable users did the same, since no JSON.parse was needed to get the .data content.

I noticed the JSON.parse in the tests you are mentioning, and I found it weird too (and probably this is the reason this behaviour found its way to production: it passed the tests).

Hope this helps!

@subrata71
Copy link
Contributor Author

@lelesrc
Thank you very much for sharing your thoughts on this.
I tried Airtable queries with the following versions of Appsmith. And I found the response as stringified JSON in each of them.

Version Release Date
v1.9.15 (Latest) April 6, 2023
v1.9.8 Feb 21, 2023
v1.9.2 Jan 6, 2023

Am I missing something?

@rohan-arthur rohan-arthur removed GraphQL Plugin Issues related to GraphQL plugin REST API plugin REST API plugin related issues SAAS Plugins Issues related to SAAS Plugins Airtable Issues for Airtable labels Apr 19, 2023
@rohan-arthur rohan-arthur removed the Integrations Pod Issues related to a specific integration label Apr 19, 2023
@rohan-arthur
Copy link
Contributor

removed tags related to integrations pod since @subrata71 is looking into this

@lelesrc
Copy link

lelesrc commented Apr 19, 2023

Can't explain this. Really.

Basically, all of my bindings and functions were working until last update (1.9.15).
I checked the doc (https://docs.appsmith.com/reference/datasources/airtable) and seems that no JSON.parse is involved in the binding example:

// binding response data to a Table widget
{{
  AirtableQuery.data.records.map(row => {
    return {
      "Name": row.fields.name,
      "Employee ID": row.fields.employee_id
    }
  })
}}

So, no clue! I don't have any valid explanation about how my code used to work until 1.9.15 (but I'm sure it did, since I had many end users using my app).

The bottom line is that It's not a big deal to update my app code to JSON.parse .data - as long as this won't change back in the future.

As a side consideration, under a UX perspective, using JSON.parse in every binding dialogue could be sub-optimal in terms of clarity (and probably not immediate for most users). I only use airtable as a datasource, so I don't know if parsing is needed when binding using other integrations. In this case, consistency would be the best design choice.

Thank you for tackling this issue, I'm available for testing if you need!

@subrata71
Copy link
Contributor Author

@lelesrc
Thank you very much for your valuable input. We don't have any clue either 😕 Seems really weird.

However, while we are trying to come up with the best solution can you please check whether the Airtable query result is giving the expected output in the following deploy-preview?

https://ce-22498.dp.appsmith.com (Note: You need to sign up)

Would really appreciate your feedback!

@lelesrc
Copy link

lelesrc commented Apr 19, 2023

I put up the simplest test case I came up with on ce-22498.dp.appsmith.com:

  1. created a datasource
  2. created a testReturnObject query retrieving a single record by id
  3. created a text field and binded it to {{testReturnObject.data.id}}

It works as expected, the record id is printed. The data object is an object.
I also tested the output of testReturnObject.run() in a JSObject:

export default {
	getId: async () => {
		let data = await testReturnObject.run()
		return data 
	}
}

data returned by the getId function is an object.

As a counter-example, I did the same on 1.9.15 and testReturnObject.data.id is undefined. The data object is a stringified JSON. The same with the getId function using testReturnObject.run(): the result is a string.

Happy to help!

@subrata71
Copy link
Contributor Author

@lelesrc
Thanks a million for doing end-to-end testing on this. I really appreciate your effort. 🙇‍♂️

@subrata71
Copy link
Contributor Author

Update

Although we have tried with different versions of Appsmith, the cloud-services remains to the latest version. And that’s where the problem was hiding.
Later, we have run the cloud-services locally and figured out that in cloud-services the response body wasn’t coming out as JSON due to the content-type mismatch.
The way we match the content type in cloud-services is through equality check
MediaType.APPLICATION_JSON.equals(contentType)
In case the contentType holds an additional charset information like application/json; charset=utf-8 then the above equality check fails and the response is coming out as String from cloud-services to Appsmith server.
Solution
contentType.includes(MediaType.APPLICATION_JSON)

Next course of action

We will be creating 3 PRs.

  • The current PR should exclude the changes made for Airtable plugin and include the fix for RestAPI and GraphQL plugins only
  • A PR to fix the content-type matching in cloud-services repository
  • A PR to fix Airtable Cypress tests

nidhi-nair pushed a commit that referenced this issue Apr 26, 2023
## 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
@appsmith-bot appsmith-bot added the QA Needs QA attention label Apr 26, 2023
ayushpahwa pushed a commit that referenced this issue Apr 27, 2023
## 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
ayushpahwa pushed a commit that referenced this issue Apr 27, 2023
## 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
@subrata71
Copy link
Contributor Author

Update

The fix is now available on Appsmith cloud i.e. app.appsmith.com with Appsmith Version: v1.9.17.

cc: @lelesrc

@btsgh btsgh added Verified When issue is retested post its fixed and removed QA Needs QA attention labels May 27, 2023
@btsgh
Copy link

btsgh commented May 27, 2023

Closing this out as per discussions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend This marks the issue or pull request to reference server code BE Coders Pod Issues related to users writing code to fetch and update data Bug Something isn't working Critical This issue needs immediate attention. Drop everything else Data Platform Pod Issues related to the underlying data platform Needs Triaging Needs attention from maintainers to triage Verified When issue is retested post its fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants