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

document behaviour when id _eq: undefined or null #2648

Closed
pronevich opened this issue Jul 31, 2019 · 8 comments
Closed

document behaviour when id _eq: undefined or null #2648

pronevich opened this issue Jul 31, 2019 · 8 comments
Labels
c/docs Related to docs e/easy can be wrapped up in a couple of days p/urgent Immediate action required

Comments

@pronevich
Copy link

Everything worked well, but then on the client-side happened issue when selected for deletion item id has become undefined and as result, all items in the table was deleted:

{
  variables: { where: { id: { _eq: undefined } } },
}
@0x777
Copy link
Member

0x777 commented Aug 1, 2019

The reason why this happens is because:

where: { id: { _eq: null } }

reduces to

where: { id: {} }

which further reduces to

where: {}

and {} translates to true. {} translating to true is something that we cannot change now as it breaks a lot of already existing queries. @shahidhk opened this a while ago. I've added my thoughts there.

For now I suggest you to write the delete mutation as follows:

mutation delete_article($article_id: Int!) {
  delete_article(where: {id: {_eq: $article_id}}) {
    affected_rows
  }
}

and the variables will then be:

{
  "article_id": 1
}

Since $article_id is defined to be of type Int!, a null value will never be accepted.

@m0ngr31
Copy link

m0ngr31 commented Aug 6, 2019

Happened to me twice recently. Wasn't a good situation. Would be good to have this explicitly documented.

@shahidhk shahidhk changed the title Critical bug. Delete mutation works with id _eq: undefined and removes all rows delete mutation works with id _eq: undefined and removes all rows Aug 6, 2019
@0x777 0x777 added the c/docs Related to docs label Aug 6, 2019
@marionschleifer marionschleifer added p/medium non-urgent issues/features that are candidates for being included in one of the upcoming sprints e/easy can be wrapped up in a couple of days and removed k/question labels Aug 13, 2019
@leimantas
Copy link

leimantas commented Aug 21, 2019

I don't use:
where: { id: { _eq: null } }

Safer:
where: {id: {_in: [ ]}}

So if somehow ID becomes undefined - mutation does not affect any rows.

@pronevich
Copy link
Author

@leimantas but this is just a temporary escape hatch, in any time new team member that doesn't know all hasura quirks could catch this issue.

@marionschleifer marionschleifer added p/urgent Immediate action required and removed p/medium non-urgent issues/features that are candidates for being included in one of the upcoming sprints labels Sep 8, 2019
@pnappa
Copy link

pnappa commented Dec 4, 2019

The reason why this happens is because:

where: { id: { _eq: null } }

reduces to

where: { id: {} }

I think this could be fixed by preventing nullable types from being compared with some operators (e.g. {_eq: X } requires X to be some type Y!). The reduction from { _eq: null } to {} is unintuitive to those unfamiliar with ternary-valued logic in SQL, especially when the column being compared is non-nullable. My 2 cents is that exposing this as a type error is a far more elegant solution to now.

@tirumaraiselvan tirumaraiselvan changed the title delete mutation works with id _eq: undefined and removes all rows document behaviour when id _eq: undefined or null Mar 19, 2020
@marionschleifer
Copy link
Contributor

@pronevich
Copy link
Author

@marionschleifer but the issue still here - one day some broken code could return null / undefined to graphql expression and we will be in troubles

@marionschleifer
Copy link
Contributor

@pronevich we are tracking this issue here .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/docs Related to docs e/easy can be wrapped up in a couple of days p/urgent Immediate action required
Projects
None yet
Development

No branches or pull requests

6 participants