{} evaluating to true is confusing

[shahidhk]

{} 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.

[0x777]

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

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

[dariocravero]

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.

[paf31]

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.

[coco98]

@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!

[paf31]

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.

[lexi-lambda]

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.

[jberryman]

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

[RapidOwl]

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.

[tirumaraiselvan]

The behaviour as decribed in #704 (comment) is now released in v1.3.4-beta.1

[reinoldus]

That kind of change in a minor version update seems dangerous to me. I think quite a few people will mindlessly upgrade those

[rocketraman]

I've come across this issue in a situation where the current behavior or { something: {_eq: null} } is actually useful to me -- its a lookup table where a NULL value in the table means "this row is valid for any value".

Now, the tricky part is that the value in the query itself can be null, so today I can write my query like this if I want to get all rows where any of the following are true:

• rows where the value in the db is null
• rows where the value in the db matches the non-null parameter value of lookupValue
• all rows when the parameter lookupValue is passed explicitly as null

query Foo($lookupValue: String) {
  foo (
    where: {
      _or: [
        { col: {_eq: $lookupValue} },
        { col: {_is_null: true } }
      ]
    }
  )
  { ... }
}

As I understand the change in 1.3.4-beta.1 as described by #704 (comment), I will no longer be able to do this without writing two separate queries:

query FooNullLookup() {
  { ... }
}

query FooLookup($lookupValue: String!) {
  foo (
    where: {
      _or: [
        { col: {_eq: $lookupValue} },
        { col: {_is_null: true } }
      ]
    }
  )
  { ... }
}

So firstly, making such a change in a minor version seems to be breaking the semver rules, as it breaks my current query above. That being said, given it fails fast instead of just returning different results, I can live with this.

More importantly, am I missing some way I can write my query so that it will work with 1.3.4-beta.2, without having to write two separate queries?

[maxpain]

@rocketraman It appears that where: {foo: {_eq: undefined} effectively reduces to where: {foo: {_eq: null} which throws.

The only solution I can see now is to handle it in JS as:

query GetUsers(
  $where: user_bool_exp!
) {
  users(
    where: $where
  ){
    id
    email
    first_name
  }
}

const response = graphQL.query(GetUsers, {variables: { where: { userId: id ? { _eq: id } : {} }}});

Would love to hear about any other alternatives.

@pat154 In this case using operation allowlist quite useless. What we can do?

[maxpain]

The older (dangerous) behaviour can be restored by explicitly setting HASURA_GRAPHQL_V1_BOOLEAN_NULL_COLLAPSE: true in your hasura instance.

@tirumaraiselvan

It doesn't work for this query:

query {
  users(
    where: {
      nick_name: {
        _ilike: null
      }
    }
  ) {
    id
  }
}

{
  "errors": [
    {
      "extensions": {
        "path": "$.selectionSet.users.args.where.nick_name._ilike",
        "code": "validation-failed"
      },
      "message": "unexpected null value for type \"String\""
    }
  ]
}

Also doesn't work for this:

query {
  users(
    where: {
      nick_name: null
    }
  ) {
    id
  }
}

{
  "errors": [
    {
      "extensions": {
        "path": "$.selectionSet.users.args.where.nick_name",
        "code": "validation-failed"
      },
      "message": "expected an object for type \"String_comparison_exp\", but found null"
    }
  ]
}

[maxpain]

Every time I see breaking changes like this, which break ever more or less big project I think nobody from Hasura Team uses their product in their own projects. Maybe it is ok for a regular "Todo list" (lol)... IDK.

[tirumaraiselvan]

In cfab5fc , we have added HASURA_GRAPHQL_V1_BOOLEAN_NULL_COLLAPSE: true behaviour to all PG operators.

Note that queries which do not use operators (but use the undocumented shorthand of directly using the column) are still not supported under this flag. e.g. the following will not work even with the flag.

query {
  users(
    where: {
      nick_name: null
    }
  ) {
    id
  }
}

Again, it might be worthwhile to examine such queries because something like:

query {
  users(
    where: {
      nick_name: {
        _ilike: null
      }
    }
  ) {
    id
  }
}

will return ALL users (i.e. the whole boolean expression resolves to true) and may not be expected.

[christian-predebon]

@tirumaraiselvan I would like to know if this flag is temporary or not. Just to know whether I have to modify all my queries soon.

[tirumaraiselvan]

@kasugaicrow This flag will be supported in 2.0 but note that it is limited to PG source and its operators (for backwards compatibility). It is not supported for other sources like MSSQL, BiqQuery, etc. Hence, it is ideal if queries can be refactored to NOT rely on this flag.

[hector]

In the past I did things like:

query Users($countries: [String!]) {
  users(where: {country: {_in: $countries}}) {
    id
    name
  }
}

By setting $countries to undefined I would just skip the filter.
How are we supposed to do this now?

Writing different queries is so overkill.
Writing the where clause as a variable is a less specific and poorer solution.

May we at least differentiate between null and undefined so that one is intentional?
I know it might seem cool to protect the programmer from making mistakes but this change just erased what is actually a useful feature for the sake of bad programmers.

[TeoTN]

@tirumaraiselvan Currently I think there's no clear, documented migration path to the new behaviour. How are we supposed to write optional filters in query now?
I don't quite understand what is official recommended way of rewriting the below, so that it fetches all entities when $directory is not passed?

  subscription GetEntities(
    $directory: uuid
  ) {
    entity(
      where: {
        directory_id: { _eq: $directory }
      }
    )

My impression is that this is very opinionated change that limits the capabilities of GraphQL - I understand that it might be painful for some to run sudo rm -rf / but it doesn't mean it has no valid usages.

[rocketraman]

My impression is that this is very opinionated change that limits the capabilities of GraphQL - I understand that it might be painful for some to run sudo rm -rf / but it doesn't mean it has no valid usages.

With this change we can't even do sudo ls -lR / without workarounds like two separate queries or dynamic where clauses!

[carlosbaraza]

If I understood it right, there is basically no way to do conditional _or entries depending on the input without exposing the entire bool_exp input? This is a bit frustrating because we use the Allow List, which means that allowing any arbitrary _or queries defeat the purpose of having an Allow List.

[phishy]

How do we truncate a table now?

[ilijaNL]

What about performance implications? According to this article: https://hasura.io/blog/fast-graphql-execution-with-query-caching-prepared-statements/ , hasura maintaince an LRU cache of queries to avoid ast parsing. However it states:

Currently, only queries and subscriptions are cached—not mutations—but most queries can be cached. Simple queries that do not contain variables are trivially cacheable, but using variables allows Hasura to create a parameterized query plan that can be reused even if variable values change. This is possible as long as query variables only contain scalar values, but more complex variables may defeat the cache, as different variable values may require different SQL to be generated (since they may change which filters are used in a boolean expression, for example).

Does it mean that when using user_bool_exp! as variables does not get cached?

[intrnl]

I suppose the only way to achieve optional filters is to move ALL the where related arguments outside as a variable?

[shoaibhassan]

We can use Nullish coalescing (??) to make sure the request contains null instead of undefined to avoid running for all rows in a table. Because in case of where: {id: {_eq: null} the request fails and in case of where: {id: {_eq: undefined} } the object collapse to where: {} }

where: {id: {_eq: undefined ?? null} }