fix json/jsonb columns as String values in nested returning of a mutation (fix #3365) #3375

rakeshkky · 2019-11-18T15:56:07Z

Description

PR #3198 caused the JSON/JSONB values returning as Strings when a mutation is performed with returning nested fields.
query:-

mutation {
  delete_author(where: {id: {_eq: 3}}){
    affected_rows
    returning{
      id
      name
      info
      articles{
        id
        title
        content
      }
    }
  }
}

Response:-

{
  "data": {
    "delete_author": {
      "affected_rows": 1,
      "returning": [
        {
          "id": 3,
          "name": "Author 3",
          "info": "{\"age\": 23}",
          "articles": []
        }
      ]
    }
  }
}

This PR fixes the same.

Affected components

Related Issues

Fix #3365

Solution and Design

The parsePGScalarValue is used to parse the Postgres column values correctly. The change in #3198 is made to select the mutated columns as Text values except for geometry/geography types. JSON/JSONB column values are parsed as JSON Strings and not as Postgres literals, which gives those values as Strings. A special parseJSONStringAsLiteral function is introduced, which forces all String values as Postgres literals.

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

No
Yes
- Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

No
Yes
- Does run_sql auto manages the new metadata through schema diffing?
  - Yes
  - Not required
- Does run_sql auto manages the definitions of metadata on renaming?
  - Yes
  - Not required
- Does export_metadata/replace_metadata supports the new metadata added?
  - Yes
  - Not required

GraphQL

No new GraphQL schema is generated
New GraphQL schema is being generated:
- New types and typenames are correlated

Breaking changes

Steps to test and verify

Reproduce #3365, the json/jsonb columns shouldn't be returned as strings.

Limitations, known bugs & workarounds

netlify · 2019-11-18T15:57:30Z

Deploy preview for hasura-docs ready!

Built with commit cab14f0

https://deploy-preview-3375--hasura-docs.netlify.com

hasura-bot · 2019-11-18T16:12:19Z

Review app for commit e8fa447 deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-e8fa447e

hasura-bot · 2019-11-19T06:42:57Z

Review app for commit 40315d2 deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-40315d28

hasura-bot · 2019-11-19T07:01:03Z

Review app for commit ac1c5c9 deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-ac1c5c9c

…3365 TODO:- Add tests

hasura-bot · 2019-11-19T08:36:10Z

Review app for commit 3117edd deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-3117eddc

hasura-bot · 2019-11-19T09:59:36Z

Review app for commit 58b4a0a deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-58b4a0ab

lexi-lambda · 2019-11-19T13:49:38Z

server/src-lib/Hasura/RQL/DML/Mutation.hs

-mkSelCTEFromColVals qt allCols colVals =
+  => (PGColumnType -> Value -> m (WithScalarType PGScalarValue))
+  -> QualifiedTable -> [PGColumnInfo] -> [ColVals] -> m S.CTE
+mkSelCTEFromColVals parseFn qt allCols colVals =


Just for my own understanding: could you explain to me from a high-level what this function is doing? When I was first looking through this diff, I was slightly confused, because it seems like we’re parsing the values (using parseFn) but then immediately re-encoding them (using toTxtValue). It seems like mutateAndSel is doing that as part of some kind of two-pass query strategy, but it’s not quite clicking for me what the reason for doing that is.

To be honest, this diff is not so informative. I'll give you a bit of context.

toTxtValue will encode any PGScalarValue to literal values. I thought it is enough and went for #3198. In case of json/jsonb values, the json string we're getting, let's say some String t where t is json string, is parsed into PGScalarValue as PGValJSON (String t). The toTxtValue converts PGValJSON (String t) to json encoding of a string value. The {"age": 23} is converted to something like "{\"age\": 23}" which is causing the bug. In mutateAndSel function, I'm enforcing all String t to PGValText t irrespective of what type it is, hence toTxtValue encodes correctly for json/jsonb values.

I hope you understand the above explanation. I'm happy to know any improvements to the current fix.

That definitely is helpful, so thank you for that explanation, but it actually doesn’t answer my original question (which is only semi-related to this pull request). Specifically, I’m trying to understand what mkSelCTEFromColVals is doing in the first place, from a high level. 🙂 Sorry if I didn’t make that very clear!

Okay 🙂 . The mkSelCTEFromColVals will generate a SELECT query, that is used inside a WITH .... statement. The SELECT statement look like

SELECT * FROM VALUES ((col1-val, col2-val, col3-val), .....) AS "table_name" (col1, col2, col3)

The parseFn converts JSON value to col-val.

I probably should have been more clear: I can mostly make out what it does, but what I’m less sure about is why. If mutateAndFetchCols does the actual mutation query, and it returns a bunch of values, why don’t we parse them directly from that result? Why do we send them back through Postgres another time?

The (1) is possible only if we have the context of array types which #2243 is meant to solve. I think this PR is solving (2). We're fetching column values as Strings (except for geometry/geography types) and storing them as PGValText. The parseJSONStringAsLiteral is doing the job.

I think this PR is solving (2). We're fetching column values as Strings (except for geometry/geography types) and storing them as PGValText. The parseJSONStringAsLiteral is doing the job.

Okay, I think that helps me to understand better what’s going on. A followup question: why are we treating the geometry/geography types differently? They have a textual representation as well, right? So it seems as though we ought to be able to treat everything uniformly.

Yes, we can use the textual representation for geometry/geography columns. Ideally, the returned columns are HashMap PGCol Text. I'll make the necessary change in next commit.

Now geometry/geography values are stored as Strings via 0f484df. Please approve if everything is OK. 🙂

@lexi-lambda Updated the PR with the latest master. Please review. 🙂

hasura-bot · 2019-11-25T06:23:03Z

Review app for commit fdb6852 deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-fdb6852c

hasura-bot · 2019-11-26T06:37:31Z

Review app for commit 281e5df deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-281e5df3

hasura-bot · 2019-12-03T06:43:34Z

Review app for commit 340b63f deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-340b63fe

hasura-bot · 2019-12-04T09:53:28Z

Review app for commit 980ddac deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-980ddacd

hasura-bot · 2019-12-09T07:48:04Z

Review app for commit 60da288 deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-60da288c

lexi-lambda

Sorry for the long delay on getting this merged!

hasura-bot · 2019-12-10T22:53:41Z

Review app for commit cab14f0 deployed to Heroku: https://hge-ci-pull-3375.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3375-cab14f00

hasura-bot · 2019-12-10T23:20:59Z

Review app https://hge-ci-pull-3375.herokuapp.com is deleted

…tion (fix hasura#3365) (hasura#3375)

rakeshkky self-assigned this Nov 18, 2019

rakeshkky mentioned this pull request Nov 18, 2019

Append Mutation Returning JSONB as String #3365

Closed

rakeshkky force-pushed the issue-3365-json-returning-mutation branch from e8fa447 to 40315d2 Compare November 19, 2019 06:26

rakeshkky force-pushed the issue-3365-json-returning-mutation branch from 40315d2 to ac1c5c9 Compare November 19, 2019 06:44

rakeshkky force-pushed the issue-3365-json-returning-mutation branch from ac1c5c9 to 541da03 Compare November 19, 2019 08:17

rakeshkky added 2 commits November 19, 2019 13:50

fix json/jsonb column values as Text in nested returning, fix hasura#…

26e70bf

…3365 TODO:- Add tests

add tests

3117edd

rakeshkky force-pushed the issue-3365-json-returning-mutation branch from 541da03 to 3117edd Compare November 19, 2019 08:20

remove 'TextOrValue' type and generate values parser using 'Value' type

58b4a0a

rakeshkky changed the title ~~fix json/jsonb column values as Text in nested returning (fix #3365)~~ fix json/jsonb column values returned as Strings in nested returning of a mutation (fix #3365) Nov 19, 2019

rakeshkky added the c/server Related to server label Nov 19, 2019

rakeshkky marked this pull request as ready for review November 19, 2019 10:02

rakeshkky requested a review from lexi-lambda as a code owner November 19, 2019 10:02

rakeshkky changed the title ~~fix json/jsonb column values returned as Strings in nested returning of a mutation (fix #3365)~~ fix json/jsonb columns as String values in nested returning of a mutation (fix #3365) Nov 19, 2019

lexi-lambda reviewed Nov 19, 2019

View reviewed changes

Merge branch 'master' into issue-3365-json-returning-mutation

fdb6852

rakeshkky requested a review from lexi-lambda November 25, 2019 06:06

Merge branch 'master' into issue-3365-json-returning-mutation

281e5df

Merge branch 'master' into issue-3365-json-returning-mutation

340b63f

rakeshkky added 2 commits December 4, 2019 15:04

store geography/geometry values as text in mutation column values

0f484df

Merge branch 'master' into issue-3365-json-returning-mutation

980ddac

Merge branch 'master' into issue-3365-json-returning-mutation

60da288

rakeshkky and others added 3 commits December 10, 2019 13:01

use 'TxtEncodedPGVal' for parsing mutated column values

1ee003f

Merge branch 'master' into issue-3365-json-returning-mutation

7cefb1d

Merge branch 'master' into issue-3365-json-returning-mutation

cab14f0

lexi-lambda approved these changes Dec 10, 2019

View reviewed changes

lexi-lambda merged commit 60acf7c into hasura:master Dec 10, 2019

0x777 mentioned this pull request Jan 3, 2020

remove hdb_views for inserts #3598

Merged

31 tasks

fvieira mentioned this pull request Jan 11, 2020

JSONB returned as String returning field of a mutation iff it includes nested fields #3682

Closed

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020

fix json/jsonb columns as String values in nested returning of a muta…

3f23b97

…tion (fix hasura#3365) (hasura#3375)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix json/jsonb columns as String values in nested returning of a mutation (fix #3365) #3375

fix json/jsonb columns as String values in nested returning of a mutation (fix #3365) #3375

rakeshkky commented Nov 18, 2019 •

edited

Loading

netlify bot commented Nov 18, 2019 •

edited

Loading

hasura-bot commented Nov 18, 2019

hasura-bot commented Nov 19, 2019

hasura-bot commented Nov 19, 2019

hasura-bot commented Nov 19, 2019

hasura-bot commented Nov 19, 2019

lexi-lambda Nov 19, 2019

rakeshkky Nov 19, 2019 •

edited

Loading

lexi-lambda Nov 19, 2019

rakeshkky Nov 20, 2019

lexi-lambda Nov 26, 2019

rakeshkky Dec 3, 2019

lexi-lambda Dec 3, 2019

rakeshkky Dec 4, 2019

rakeshkky Dec 5, 2019

rakeshkky Dec 9, 2019

hasura-bot commented Nov 25, 2019

hasura-bot commented Nov 26, 2019

hasura-bot commented Dec 3, 2019

hasura-bot commented Dec 4, 2019

hasura-bot commented Dec 9, 2019

lexi-lambda left a comment

hasura-bot commented Dec 10, 2019

hasura-bot commented Dec 10, 2019

fix json/jsonb columns as String values in nested returning of a mutation (fix #3365) #3375

fix json/jsonb columns as String values in nested returning of a mutation (fix #3365) #3375

Conversation

rakeshkky commented Nov 18, 2019 • edited Loading

Description

Affected components

Related Issues

Solution and Design

Server checklist

Catalog upgrade

Metadata

GraphQL

Breaking changes

Steps to test and verify

Limitations, known bugs & workarounds

netlify bot commented Nov 18, 2019 • edited Loading

hasura-bot commented Nov 18, 2019

hasura-bot commented Nov 19, 2019

hasura-bot commented Nov 19, 2019

hasura-bot commented Nov 19, 2019

hasura-bot commented Nov 19, 2019

Choose a reason for hiding this comment

rakeshkky Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasura-bot commented Nov 25, 2019

hasura-bot commented Nov 26, 2019

hasura-bot commented Dec 3, 2019

hasura-bot commented Dec 4, 2019

hasura-bot commented Dec 9, 2019

lexi-lambda left a comment

Choose a reason for hiding this comment

hasura-bot commented Dec 10, 2019

hasura-bot commented Dec 10, 2019

rakeshkky commented Nov 18, 2019 •

edited

Loading

netlify bot commented Nov 18, 2019 •

edited

Loading

rakeshkky Nov 19, 2019 •

edited

Loading