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

Fix error raised in deserialization of resolver aliased parameters. #130

Merged
merged 3 commits into from
May 21, 2021

Conversation

callumforrester
Copy link
Contributor

Fix aliased resolver arguments being looked up by original value when checking for parameter requirement. Unsure if the routine needs to check for both the alias and original parameter name.

@wyfo
Copy link
Owner

wyfo commented May 20, 2021

Thank you for having spotted this tricky bug.

Because it's not enough documented in the code, let me explain the reason of this conditional continue. Normally, if the parameter hasn't Optional type, it should result in a non-null parameter in the generated GraphQL schema, and this condition should never be true (because graphql-core would raise an error before calling the resolver). However, there is three cases in which the parameter can become nullable in the schema, which are handled here:

  • If the parameter default is Undefined, it should be able to be omitted, and that's only possible with a nullable type, so Optional is added just for the schema generation, but not for deserialization; null/None is thus ignored for this parameter.
  • If the parameter default is None with a non-optional type, what is possible because of Python typing rule and a kind of bug in typing.get_type_hints; in this case, Optional is added for the schema generation. However i've just realized that i should have written opt_param = is_union_of(param_type, NoneType) or param.default is None, so opt_param should be true.
  • If the serialization of the parameter default fails, it can not be added in the schema, but the field has to be optional, which result in nullable in the schema.
    In all of this cases, parameter has a default value (so it's not required), but the None value is authorized because it has been added as a dummy default, so it's skipped.

Concerning your test, for this kind of issue, i would prefer an "integration test", calling graphql.graphql_sync with a generated schema, but it's up to you, I can merge now.

Unsure if the routine needs to check for both the alias and original parameter name.
Only the alias. This line of code was written at a time were aliases where handled differently, and it had not been updated accordingly when aliases have been added here.

@@ -269,7 +269,9 @@ def resolve(__self, __info, **kwargs):
errors: Dict[str, ValidationError] = {}
for alias, param_name, deserializer, opt_param, required in parameters:
if alias in kwargs:
if not opt_param and kwargs[param_name] is None:
# If this parameter was required but not supplied we can ignore
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not exact (see my comment in the PR)
The parameter is not required but its default is not serializable, so it ends up with an optional parameter, with null as an authorized value. In that case, it should be ignored to ensure using the true default value.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, because the parameter is in kwargs, that means it has been explicitely supplied by the GraphQL query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've redone the comment, is that closer to what you mean? Re: kwargs, do you mean as opposed to just having a default value?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is perfect.
About kwargs, your previous comment was not exact, because kwargs[alias] being None means that the argument *was supplied in the query; if it was not supplied, kwargs would not contains alias.

@wyfo wyfo added the bug Something isn't working label May 21, 2021
@callumforrester
Copy link
Contributor Author

Thanks for taking a look. I agree RE: the tests, I've moved them. I'm also happy to include the correction to opt_param = is_union_of(param_type, NoneType) or param.default is None or I can leave that to you to do in another PR, up to you.

@wyfo wyfo changed the title Fix resolver argument alias lookup, add relevant tests Fix error raised in deserialization of resolver aliased parameters. May 21, 2021
@wyfo wyfo merged commit c659d3c into wyfo:master May 21, 2021
@wyfo
Copy link
Owner

wyfo commented May 21, 2021

Concerning opt_param, i will do it in an other PR in order to merge yours directly. I've modified the title of the PR because I use them to generate the changelog.
Thank you a lot for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants