Discussion - Pass context to resolve methods

[mjtamlyn]

The context story seems very confused in the newest version. My understanding from the graphql-js 0.5.0 release notes is that all resolving functions should now have the signature ([self,] args, context, info), as opposed to ([self,] args, info).

At the moment, if I update a resolve_NAME method to take context, then it throws argument errors.

Also, mutations don't take context and therefore they cannot access context data. It think a patch similar to this one on graphql-relay-js is necessary for the relay mutations at least, but probably for all of them.

[danielfarrell]

That PR seems to address this issue for me, and I've got it working locally. If there should be anything else on there, let me know.

[syrusakbary]

@mjtamlyn @danielfarrell thanks for opening the issue and the PR.

For achieve more simplicity, context and info args will be not mandatory in the resolver in the next stable version of graphene (see Roadmap).
So resolvers could access the context and info only if they requested explicitly by using the with_context and with_info decorators.

The current implementation only uses with_context, as step in the middle before the next stable release.

PS: In JS this is a little bit easier as you could call a function with less arguments than required without triggering an error as it will complete the argument values with undefined, so they didn't have this problem

Makes sense?

[mjtamlyn]

Right, I think that makes sense. Python actually wanting a proper set of arguments to a function does complicate the issue slightly.

By the way, there's no tagged release or changelog I can find for 0.9 which did make this slightly more complicated to work out! I'll give it a go using with_context.

[mjtamlyn]

Ok, I've got it working fine apart from relay mutations which need a bit of extra work I've done in the PR. It would also be very helpful to add some docs about with_context and how to use it.

[danielfarrell]

Dangit, how was I all over the codebase yesterday and didn't see with_context. Ha, I'll close the PR.

[Globegitter]

@syrusakbary is it possible to maybe have this as an option? We use the context all over the place and it would be quite a pain to have to add the with_context decorator to 90% of our resolve function. Also why then not just write resolve_bla(self, _, __) if you don't need the arguments. Or resolve_bla(self, *_) or resolve_bla(self, **_) depending on if they are being passed in as positional or keyword args.

Especially the * versions seems to me simpler than having to remember and import different decorators depending on what you need. I hope that decision is not fully set in stone yet.

[mjtamlyn]

I would agree that resolve_blah(self, args, **options) where options is now info=, context= but can be extended later would be more pythonic.

[syrusakbary]

I would like to discuss about that.
Somehow I like having the args exposed directly as keyword arguments in the resolver.

Like:

class Query(graphene.ObjectType):
    say = graphene.String(what=graphene.String())

    def resolve_say(self, what):
        return "Hey {}".format(say)

I think it's too verbose having info and context as required arguments... as are not going to be used always. Since we moved the context outside info, do we really use/need info in the general case? And therefore... it should be a required arg/kwarg?

Maybe using decorators (like with_context, with_info) could have some side effects, so I don't mind to revisit.

I would like to get into a solution that could solve both points of view, or at least get closer.

Merging args and context, info in one dict is another possible solution (like: dict(args, context=context, info=info)).
But then, we could have collision between argument names and context/info.

More proposals? Thoughts?

[Globegitter]

@syrusakbary yeah that is a quite difficult one. I mean the only other solution I could think of then is dict(args=args, context=context, info=info) to not have potential name conflict. I personally would prefer that all to the decorator.

[mjtamlyn]

Ok, so this is a tricky design decision in some ways because we want to mimic the "argument magic" that javascript can manage in python. We've also got a possible name clash issue.

Here are a few possible approaches:

• Use decorators to explicitly mark which resolvers need which arguments. This is the current proposal. It doesn't feel very pythonic. We could have a convention that context and info could be passed as _context and _info if there's a name clash, or always.
• Use the inspect module to find the signature. inspect.getargspec(resolver_func).args will return a list of names of arguments. If that includes context or info then we pass them. This is "fully" magical, but I don't find it any more distasteful than the _root/getattr pattern we have at the moment. I'm not sure if we can get access to which arguments might be relevant to deal with name clashes, but it would be easy to see if a user is expecting context AND _context and pass the arg as the former and the context as the latter.
• Define a more pythonic standard API for a resolver. Something like def resolve_foo(self, arg1, arg2, **options). Again we can deal with name clashes with _context if necessary, or just have a convention that you don't do that. I feel this is the route we would be taking if there wasn't the JS reference implementation.