10000 Changed InputObjectType's default builder-from-dict argument to be `Undefined` instead of `None` (or else we can't catch "null" inputs in InputObjectTypes, as both would coerce to `None`) by flipbit03 · Pull Request #1471 · graphql-python/graphene · GitHub
[go: up one dir, main page]
Skip to content

Changed InputObjectType's default builder-from-dict argument to be Undefined instead of None (or else we can't catch "null" inputs in InputObjectTypes, as both would coerce to None) #1471

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Changed InputObjectType's default builder-from-dict argument to be Undefined instead of None (or else we can't catch "null" inputs in InputObjectTypes, as both would coerce to None) #1471

wants to merge 2 commits into from

Conversation

flipbit03
Copy link
Contributor
@flipbit03 flipbit03 commented Nov 10, 2022 •
edited
Loading

This pull request aims to fix a shortcoming in the InputObjectTypes' implementation that makes it unable to differentiate an incoming undefined value from an intentional null.

The problem lies in the way the InputObjectType is constructed from the incoming parameter dictionary:

class InputObjectTypeContainer(dict, BaseType):
    class Meta:
        abstract = True

    def __init__(self, *args, **kwargs):
        dict.__init__(self, *args, **kwargs)
        for key in self._meta.fields:
            setattr(self, key, self.get(key, None)) # <-- Ambiguous default value

    def __init_subclass__(cls, *args, **kwargs):
        pass

With None as that self.get's default value, we are unable to differentiate between someone passing an intentional null (None) in an optional field. This PR refactors that default to point to Undefined, aligning closely with the GraphQL Spec.

This change is a potential breaking change for resolvers, so I've implemented a function called set_input_object_type_default_value that allows one to set the desired default value for InputObjectTypes. Calling set_input_object_type_default_value(graphql.Undefined) activates the new behavior in your codebase. This function should be called before your InputObjectTypes are defined.

The final container type then became:

_INPUT_OBJECT_TYPE_DEFAULT_VALUE = None

def set_input_object_type_default_value(default_value):
    global _INPUT_OBJECT_TYPE_DEFAULT_VALUE
    _INPUT_OBJECT_TYPE_DEFAULT_VALUE = default_value

class InputObjectTypeContainer(dict, BaseType<
8000
/span>):  # type: ignore
    ...
    def __init__(self, *args, **kwargs):
        dict.__init__(self, *args, **kwargs)
        for key in self._meta.fields:
            setattr(self, key, self.get(key, _INPUT_OBJECT_TYPE_DEFAULT_VALUE))
    ...

@codecov
Copy link
codecov bot commented Nov 10, 2022 •
edited
Loading

Codecov Report

Base: 95.98% // Head: 95.98% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (a8cb8ca) compared to base (340d5ed).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1471   +/-   ##
=======================================
  Coverage   95.98%   95.98%           
=======================================
  Files          51       51           
  Lines        1742     1745    +3     
=======================================
+ Hits         1672     1675    +3     
  Misses         70       70
Impacted Files Coverage Δ
graphene/types/inputobjecttype.py 97.50% <100.00%> (+0.20%) ⬆️
graphene/validation/depth_limit.py 96.96% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@flipbit03
Copy link
Contributor Author

@erikwrede as requested, I have provided a specific test for the new functionality around InputObjectTypes, so that we can have its default type be changed dynamically to whatever the user might want (a good suggestion is Undefined so that we can easily keep track of inputs that were not provided in the Input). This behavior is opt-in so as not to break existing codebases.

IslamMesha
IslamMesha previously approved these changes Nov 17, 2022
View reviewed changes
@flipbit03 @erikwrede
Changed InputObjectType's default builder-from-dict argument to be `U…
9dcbfc8 
…ndefined` instead of `None` (or else we can't catch "null" inputs in InputObjectTypes, as both would coerce to `None`)
@erikwrede
Copy link
Member

Since no one had any adverse opinions here or on slack, I'd say it's time to get this change out soon!

For consistency, we should also override the Argument default values with the global settings so that missing arguments are also marked Undefined instead of None.

@flipbit03
Copy link
Contributor Author
flipbit03 commented Dec 19, 2022 •
edited
Loading

@erikwrede I believe this was already fixed in here

If we were to use set_input_object_type_default_value()'s set value to Argument's as well, we'd regress them back into None's again, since they are already flipped to use Undefined in 3.1.0, and in case of InputObjectType, the behavior we implemented is opt-in, so as not to constitute a breaking change.

We could maybe do the opposite, do away with the opt-in mechanism and make InputObjectTypes conform to the behavior that is being already applied to graphene.Argument's without any kind of 'activation' mechanism, that is, make it default to Undefined. But that's a breaking change.

I don't see a breaking change as a negative in this case, since it is fixing a bug that was blocking us from use null's correctly in some contexts. It's a very important fix.

If we are to go through the breaking change route, we only need to document the rationale behind the change, so that everyone is aware of why this was a problem and why it needs to be changed to a better default.

Please let me know your thoughts on this. Thanks in advance.

@flipbit03 flipbit03 closed this by deleting the head repository May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IslamMesha IslamMesha IslamMesha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flipbit03 @erikwrede @IslamMesha
0