A possible minor bug in ExecutionContext.complete_value

[ipeterov]

I'm trying to figure out a way to return None for non-nullable fields if the requesting user doesn't have permission to view them.

I was trying to pass Undefined from graphql.pyutils.undefined, but it doesn't work. It looks like the second check in this line

# If result value is null or undefined then return null.
if result is None or result is Undefined:
    return None

can never run because Undefined is actually a descendant of ValueError and therefore triggers this check.

if isinstance(result, Exception):
    raise result

I don't know if this was meant as an escape hatch for returning null values if you really wanted to (which is what I really need right now), but the code itself seems wrong either way.

[Cito]

Hi @ipeterov, thank you for reporting this problem.

For background, the code that you quoted actually replicates the corresponding code in GraphQL.js here and here.

As a fix, we could change the check for exceptions like this:

if isinstance(result, Exception) and result is not Undefined:
    raise result

But actually this does not feel right and reveals that the way how the type of Undefined is currently implemented as a subclass of ValueError is wrong. The intention was probably to signal that something has "no value", hence ValueError, but good intentions can sometimes have unintended consequences.

I think we should change this in the upcoming 3.3 release add instead make it a subclass of NoneType = type(None) or a direct subclass of object. Maybe we should also change the __eq__ method so that it is equal to None, like in JavaScript.

The above fix could be applied in the 3.2.x maintenance branch because it is more backward compatible.

What do you think about this?

We also need a test for this. It's strange that this was not caught by one of the existing tests.

[erikwrede]

If I interpreted your issue correctly, you are trying to set a required field to undefined in a response.

This should not be possible when you are trying to stay in line with the GQL spec, check this comment here:

graphql/graphql-js#1298 (comment)

If you have incorrect permission to read a required field your only option would be to throw an error which will traverse up the AST to the next nullable node and set it to zero. If you set a field to undefined that means it is not present in the response.

All requested fields of the query must be present in the response unless there is an error. (Or an error moved to the next nullable parent node in the AST for required fields).

I strongly suggest throwing errors instead of trying to set fields to None. Defensive contract first API design can save you many headaches later (e.g. an API consumer using gql codegen expecting that field to always be present).

[Cito]

I agree with @erikwrede but @ipeterov is also right in that this check does not work as it should. Will look a bit deeper into this when I have more time.

[erikwrede]

@Cito The other part of my comment got lost because I had to change trains, so I'll add it here 🙂
Regarding the functional changes for 3.3, I believe that Undefined being a subtype of None is probably the best solution going forward. This also makes life easier when consuming field resolver arguments, because the if not argument syntax will work for both Undefined and None if there is no need to know about the difference in this case.

[Cito]

I have changed this now so that Undefined is not an Exception any more. Unfortunately, it is not possible to subclass NoneType in Python, so I made it is standalone class. However, I added magic methods so that Undefined == None.

Keeping this open for feedback until the next version with this change is released.

[erikwrede]

I like the change!
Important to highlight that clearly though as
if value == None ... elif value == Undefined needs to be changed to if value is None ... elif value is Undefined.

[Cito]

This change is now implemented in 3.3.0a3.