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

GraphQL: null on GraphQL API layer should be converted to {__op: "Delete"} #7648

Closed
4 tasks done
Moumouls opened this issue Oct 19, 2021 · 8 comments · Fixed by #7649
Closed
4 tasks done

GraphQL: null on GraphQL API layer should be converted to {__op: "Delete"} #7648

Moumouls opened this issue Oct 19, 2021 · 8 comments · Fixed by #7649
Labels
state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version type:bug Impaired feature or lacking behavior that is likely assumed

Comments

@Moumouls
Copy link
Member

New Issue Checklist

Issue Description

Currently null values used during GraphQL mutation are not transformed by the GraphQL layer, so null is directly saved into the database. null in database could lead to unwanted behavior since query like exists: false will not resolve when the field has a null value.

Currently the GraphQL API has a lack of support to correctly unset fields. We should transform null to op Delete

Steps to reproduce

  • Create an object via graphql
  • Update a field to null
  • Query the field object with exists: false
  • Query will not resolve the object

Actual Outcome

null is saved into the DB

Expected Outcome

Null should transformed to undefined into the DB through the GraphQL API

Environment

alpha 5.0

Server

  • Parse Server version: alpha 5.0
  • Operating system: macOs
  • Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): local

Database

  • System (MongoDB or Postgres): mongo
  • Database version: 4.4
  • Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): local
@parse-github-assistant
Copy link

Thanks for opening this issue!

  • 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

@mtrezza
Copy link
Member

mtrezza commented Oct 19, 2021

null in database could lead to unwanted behavior since query like exists: false will not resolve when the field has a null value.

This is expected and well-known behavior. A null value is not the same as undefined. A developer is expected to be aware of that, and there are use cases in which this differentiation is desired. The Parse SDKs account for that differentiation. For example, in the JS SDK there is a distinction between obj.set("key", null); and obj.unset("key");. See #7518 (comment).

@Moumouls
Copy link
Member Author

Moumouls commented Oct 19, 2021

So here following the official spec of GraphQL null value in the input SHOULD delete the field in the database.

Spec ref: https://spec.graphql.org/June2018/#sec-Null-Value

a mutation representing deleting a field vs not altering a field, respectively

and there are use cases in which this differentiation is desired

Agree, but GraphQL API could not handle this diff, currently a developer with the GraphQL API can't know if the field is undefined or null, and graphql will never support this 2 values. Here the special case with the strict null check should be handled with the SDK or cloud code.

But we need to let the developer unset the field via null value as expected by the official GraphQL Doc. And avoid a custom system with some unlink unset workarounds 🙂 (also hard to maintain)

Later if a developer need to know if the field value is really "null" (non sense in a graphql use case since the response will not change), we could introduce a query {field: {isNull: false}} or something similar

@Moumouls
Copy link
Member Author

PR sent

@mtrezza
Copy link
Member

mtrezza commented Oct 19, 2021

Interesting, looking at the GraphQL spec it seems you are correct. Reclassified this as a bug since the current behavior seems to violate the GraphQL specs.

@mtrezza mtrezza added severity:medium type:bug Impaired feature or lacking behavior that is likely assumed and removed type:improvement labels Oct 20, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-alpha.2

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label Oct 27, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version type:bug Impaired feature or lacking behavior that is likely assumed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants