Graphql-engine: `{}` evaluating to true is confusing

Created on 11 Oct 2018 · 10Comments · Source: hasura/graphql-engine

{} evaluating to true especially with where is confusing. We had cases which a frontend bug caused one variable to be undefined and the query was sent with where: {} instead of where: {key:value} and the entire table got cleared (acepsc).

701 is also due to `where: {}` matching all the rows.

We need a special construct to match all the cases (that evaluates to true). An empty {} should mean nothing so that we can avoid accidental wiping out of the entire table using where: {}. This is bound to happen with frontend languages like JS and we should be a little forgiving there as it is our primary target.

server triag2-needs-rfc

Source

shahidhk

👍6

Most helpful comment

I propose that we fix this as part of #4111. This would be a breaking change, though as has already been discussed, it seems just as likely that any situations where it comes up are bugs, not intended uses. I believe the potential for harm is significant enough that we should make the change, but if we receive reports of breakage during beta, we could add a temporary flag to re-enable the old behavior.

Our current solution in #4111 is more sophisticated than what has been described in this thread so far, and I believe it addresses the mentioned concerns in a neat way. Here’s the overview:

We do not change the behavior of where: {}—if you write that in a query, it is equivalent to no where argument at all.
Likewise, we do not change the behavior of where: {foo: {}}, which behaves the same as where: {}.
We do change the behavior of where: {foo: {_eq: null}}. This is now an error; it is not equivalent to omitting the _eq completely.

This is intensely strange from the perspective of the GraphQL type system, since the type of the _eq field remains nullable. As discussed above, it has to be, or it could not be omitted. But we reject any situations where a null value is actually passed with an error that says we expect a non-null value.

This behavior is explicitly permitted by the GraphQL specification. From the section on Input Objects:

If the value null was provided for an input object field, and the field’s type is not a non‐null type, an entry in the coerced unordered map is given the value null. In other words, there is a semantic difference between the explicitly provided value null versus having not provided a value.

Emphasis mine. Again, this is completely bizarre from the point of view of the GraphQL type system, but it is completely logical from the point of view of the user! The user does not think of _eq as accepting a nullable column, they think of it as an optional argument that accepts a non-nullable column. The GraphQL type system treats these identically, but the specification gives us an out: we can’t give them different types, but we can give them different semantics.

This neatly solves the common problem where a variable accidentally ends up undefined, since we would now fail with an error, not delete the user’s database. At the same time, if someone explicitly writes code to omit the condition completely, we accept it. This seems like it’s clearly the right behavior to me.

lexi-lambda on 28 May 2020

👍6

All 10 comments

Changing the behaviour of {} will be a huge breaking change. Instead we can try to solve the mutations case where this becomes an issue.

By having a (delete|update)_table_by_pk top level field. These will automatically restrict the mutation's surface to one row.
By introducing allow_mutating_all_rows in update and delete permissions. This will not allow where to be {} in mutations.

0x777 on 1 Aug 2019

👍3

How about adding a parameter to delete mutations that specifies the behaviour of where: {}? eg, allow_delete_all: boolean? if it defaulted to true it wouldn't change the existing behaviour.

dariocravero on 16 Aug 2019

There seems to be a few things going on together here. Calling a bulk delete with "where": {} is definitely asking to delete the whole dataset, but the real issue seems to be a) it’s too easy to do that by mistake generally, and b) specifically because JS sends {} when you meant { "foo": null } but foo was undefined instead of null.

If we’re using objects to encode sets of filters, I don’t see a way around this, keeping the API as it is. The onus seems to be on the user to sanitize the inputs.

If the alternative is changing the API, I definitely don’t think the solution is to change the meaning of {}. Some other possibilities:

Use a flag or a header, as @dariocravero describes. If we do add a flag, I’d hope the two behaviors would be a) what we have now, vs. b) producing an error on "where": {}, and not turning {} into false. But even the flag is an incomplete solution. It’s better certainly, but I can imagine a case with something like "where": {"foo": true, bar: $bar}, where foo is never or very rarely false (say 1% of the rows) in the table, and then if bar is undefined, you still wipe out 99% of the rows.
Don't use an object to encode the filters. Obviously, this is a much more drastic change and not incremental like the flag option.

paf31 on 10 Dec 2019

@paf31 That is a perfect summary! The best approach would be to make it harder for {} to happen in unexpected scenarios, without changing the semantics of {}.

In a brief discussion with @0x777 and @rikinsk the proposed next step is to work on preventing the automatic collapsing of where: {id: {_eq: null} } to where: {id: null} to where: {}.

The case where users don't sanitize their input and inadvertently create a where: {} in their query itself, is not something we can solve very well without changing the semantics of {}. For example, this might happen when the value is undefined: JSON.stringify({where: {id: {_eq: undefined}}}) becomes {where: {id: {}}} which evaluates to where id is true. At this point, Hasura doesn't know if this behaviour was intended or not.

👉 To fix: Disable collapsing of null into {} inside the Hasura BoolExp.

@0x777 If you can put together the RFC for this, then we can get started!

coco98 on 10 Jan 2020

In a brief discussion with @0x777 and @rikinsk the proposed next step is to work on preventing the automatic collapsing of where: {id: {_eq: null} } to where: {id: null} to where: {}.

That case sounds like a real bug to me though. Passing undefined is not so much collapsing as the value never getting encoded in the first place. If you encode null, I don't think it should be equivalent to passing nothing at all. If the query isn't meaningful, we could throw the equivalent of a type error.

paf31 on 10 Jan 2020

Our current solution in #4111 is more sophisticated than what has been described in this thread so far, and I believe it addresses the mentioned concerns in a neat way. Here’s the overview:

We do not change the behavior of where: {}—if you write that in a query, it is equivalent to no where argument at all.
Likewise, we do not change the behavior of where: {foo: {}}, which behaves the same as where: {}.
We do change the behavior of where: {foo: {_eq: null}}. This is now an error; it is not equivalent to omitting the _eq completely.

This behavior is explicitly permitted by the GraphQL specification. From the section on Input Objects:

If the value null was provided for an input object field, and the field’s type is not a non‐null type, an entry in the coerced unordered map is given the value null. In other words, there is a semantic difference between the explicitly provided value null versus having not provided a value.

lexi-lambda on 28 May 2020

👍6

A breaking change from "probably behavior you didn't intend" to "refuse to do sketchy thing; explain situation to user" seems safe to me!

jberryman on 1 Jun 2020

This sounds great and certainly fits with how I'd be making the API calls in the .Net world. It would allow me to avoid writing code for generic checks to make sure no parameters are null or checking every one individually.