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

self parameter is None in resolver methods #654

Closed
AadamZ5 opened this issue Jan 11, 2021 · 12 comments
Closed

self parameter is None in resolver methods #654

AadamZ5 opened this issue Jan 11, 2021 · 12 comments

Comments

@AadamZ5
Copy link

AadamZ5 commented Jan 11, 2021

In an example like this,

import strawberry

@strawberry.type
class Query:

    @strawberry.field
    def test_field(self) -> str:
        return "Hello"

    @strawberry.field
    def test_field2(self) -> str:
        assert self is None # <= True :(
        return str(self)

    @strawberry.field
    def test_field3(self, test_param: str) -> str:
        assert self is None # <= True still :(
        print(self)
        print(test_param)
        return str(self)

schema = strawberry.Schema(query=Query)

I noticed while trying to use instance attributes that self isn't actually passed into the method. Are the Query, Mutation, and Subscription class instances actually passed back into the methods?

A couple things to note here,

  • Notice I have no parameters on the test_field2 resolver method.
  • Notice the same behavior even with parameter(s) in test_field3 resolver method.
  • This behaves the same when overriding __init__. Does overriding the initializer function break anything?
  • Unknown behavior on Mutation and Subscription type classes.

Personally I can work around this using dependency injection, but I wanted to bring it up. The reason being, is because I noticed that on your documentation, the examples you provide (like in the README, or on the website home page) also contain the self parameter in the resolver functions, although not actually utilized in the examples.

Reproduction is simple, just try one of your examples, modified to utilize the self parameter, or the example I've copied above, and run it with:

strawberry server <module_name>

I'm not sure when the problem first started occurring, I've only been playing with this library for a couple days.

I'm using the latest version of strawberry from PyPI. My python environment is running python 3.8. I'm running it on WSL 2.

I haven't had time to inspect the source code, but I will try if I get some free time. School starts again tomorrow... 📚🥱

Thanks!

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@AadamZ5
Copy link
Author

AadamZ5 commented Jan 11, 2021

Another quick note,

Actually taking an instance of Query does indeed pass a self parameter, as expected.

import strawberry

@strawberry.type
class Query:
    @strawberry.field
    def test_field(self) -> str:
        return "Hello"
    @strawberry.field
    def test_field2(self) -> str:
        print(self)
        return str(self)
    @strawberry.field
    def test_field3(self, test_param: str) -> str:
        print(self)
        print(test_param)
        return str(self)

q = Query()
assert q.test_field2() == 'None' # <= AssertionError! So `self` isn't None!

This leads me to believe that the resolver engine doesn't actually use instances of classes while executing resolver methods. If I had to take one stab at a possible fix, it would be to design expecting instances into strawberry.Schema(...) instead of types. You can still gain types from instances, so this should be a nice explicit way to pass an instance, and hang on to it to let the resolver methods receive a true self instance. Maybe this isn't possible due to GraphQL implementation.

q = Query()
#m = Mutation()
#s = Subscription()

schema = strawberry.Schema(query=q)

(I'll make note that if you run this example now, it seems like strawberry.Schema(...) still takes only the type of the query object)

That's just an idea making many assumptions, and I don't know enough of how strawberry works to make a valid argument here. What are your thoughts?

@jkimbo
Copy link
Member

jkimbo commented Jan 11, 2021

@AadamZ5 yep this is a known issue and it's because the resolver functions are actually static methods. It was discussed briefly here: #515

You can "fix" it by setting the root_value parameter when executing a query as an instance of the Query type:

result = schema.execute(query, root_value=Query())

(for the framework integrations you can override the get_root_value functions: https://strawberry.rocks/docs/django/#get_root_value)


I'm in favour or defaulting the root value to always be an instance of the Query type so that this doesn't confuse more people. I think we agreed on that approach @patrick91 ?

@patrick91
Copy link
Member

I'm in favour or defaulting the root value to always be an instance of the Query type so that this doesn't confuse more people. I think we agreed on that approach @patrick91 ?

I wonder if that might be a problem or not due to potential side effects 🤔

@AadamZ5
Copy link
Author

AadamZ5 commented Jan 11, 2021

@jkimbo

I'm in favour or defaulting the root value to always be an instance of the Query type so that this doesn't confuse more people. I think we agreed on that approach @patrick91 ?

As long as it's a shared instance, and not a newly generated one for each individual query. I think having the self parameter work as expected is important, otherwise it should be documented how these functions don't get a self parameter by default. If documenting, where should that information be put? The first instance @strawberry.field is used for a resolver is in Schema basics | The Mutation Type.

Right now, I'm not using any sort of framework. I am simply playing with the strawberry server app test server. So I'm not exactly sure how I'd even pass in root_value or override any get_root_value function. Eventually I want to integrate strawberry with some sort of Flask implementation on my app. I'll need to serve both a webpage and strawberry as an API.

@jkimbo
Copy link
Member

jkimbo commented Jan 12, 2021

As long as it's a shared instance, and not a newly generated one for each individual query.

@AadamZ5 what do you mean by that? In my mind the Query instance would be created each time execute is called. I think that makes the most sense.

Right now, I'm not using any sort of framework. I am simply playing with the strawberry server app test server. So I'm not exactly sure how I'd even pass in root_value or override any get_root_value function.

Ah yeah it's not possible to customise the root value when using the dev server. You will have to setup a custom view inside Flask to be able to do that.


Also I realised that defaulting the root value to an instance of Query will only work for queries and is not possible for Mutations (or Subscriptions though I'd have to check that). Thats because the GraphQL execute method doesn't differentiate between queries and mutations (in fact you can execute a query and a mutation at the same time). For mutations I think we should be leaning into people defining their mutations as static functions anyway so maybe it's not such a big deal.

@AadamZ5
Copy link
Author

AadamZ5 commented Jan 12, 2021

@AadamZ5 what do you mean by that? In my mind the Query instance would be created each time execute is called. I think that makes the most sense.

@jkimbo I was planning to use instance variables on the Query object for storing data and instances of services. I was trying to use it more as a python class rather than a GraphQL static query object. In my application, I tend to use instances of everything, rather than static objects, so that's just the idea that was in my head. My goal was to inject and retain my "device" service on to the Query instance, so the resolver methods could use it for gathering the data, which is how I stumbled into the topic of this issue. Maybe I have the wrong idea as to how and why it should work that way. Regardless, I can solve my issue with dependency-injecting in every resolver method, even if it is some repeated code.

Ah yeah it's not possible to customise the root value when using the dev server. You will have to setup a custom view inside Flask to be able to do that.

This brings me to another topic I'd like to discuss. Do you have a roadmap for when we can expect more usage examples? For instance, I see some code for ASGI implementation with uvicorn and such, but no examples for Flask (although I am new to Flask as well...), and I was really hoping to eventually see examples of using the library without any big frameworks too. I believe this could be done using schema.execute(...). This definitely goes beyond the topic of this issue, but I didn't think it warranted me opening yet another issue.

Also I realised that defaulting the root value to an instance of Query will only work for queries and is not possible for Mutations (or Subscriptions though I'd have to check that). Thats because the GraphQL execute method doesn't differentiate between queries and mutations (in fact you can execute a query and a mutation at the same time). For mutations I think we should be leaning into people defining their mutations as static functions anyway so maybe it's not such a big deal.

With this info, my opinion is that all of the root-level objects (Query, Mutation, Subscription) should be structured and behave the same. If the other two root-level objects can't actually receive an instance of themselves, then none of them should, for consistency. In other words, I believe you should lean into people defining every resolver method as static. I believe I discussed this on #515.

Loving this library so far! I hope I'm not being a nuisance, or asking some un-useful questions! I'm trying to get involved more but I don't have a bunch of professional experience, so bear with me if something I ask is... seemingly obvious. Thanks!

@patrick91
Copy link
Member

patrick91 commented Jan 13, 2021

Also I realised that defaulting the root value to an instance of Query will only work for queries and is not possible for Mutations (or Subscriptions though I'd have to check that). Thats because the GraphQL execute method doesn't differentiate between queries and mutations (in fact you can execute a query and a mutation at the same time). For mutations I think we should be leaning into people defining their mutations as static functions anyway so maybe it's not such a big deal.

That's annoying, so basically there would always one root value? I wonder if can do that (and if we should). From what I understood the GraphQL spec don't specify what the very first root value should be, maybe we can see if there's any other framework that do things differently?

Regardless, I can solve my issue with dependency-injecting in every resolver method, even if it is some repeated code.

@AadamZ5 maybe you can use info.context for that?

Do you have a roadmap for when we can expect more usage examples?

This is something we discussed in another issue related to subscriptions, we made a repo for adding examples, but we haven't started yet, feel free to suggest examples, I've added some ideas here: strawberry-graphql/examples#1

This definitely goes beyond the topic of this issue, but I didn't think it warranted me opening yet another issue.
Feel free to open an issue or discussion, or join our discord http://strawberry.rocks/discord 😊

With this info, my opinion is that all of the root-level objects (Query, Mutation, Subscription) should be structured and behave the same. If the other two root-level objects can't actually receive an instance of themselves, then none of them should, for consistency. In other words, I believe you should lean into people defining every resolver method as static. I believe I discussed this on #515.

I think that makes sense, I'm personally only worried that having something like this:

@strawberry.type
class A:
    @strawberry.field
    def field_a() -> str:
       return "Example"

Might look weird to beginners as they would expect a self argument, but maybe I'm worrying too much.

Loving this library so far! I hope I'm not being a nuisance, or asking some un-useful questions! I'm trying to get involved more but I don't have a bunch of professional experience, so bear with me if something I ask is... seemingly obvious. Thanks!

No worries at all, feel free to ask any question you have in mind (here or on discord)!

@jkimbo
Copy link
Member

jkimbo commented Jan 13, 2021

That's annoying, so basically there would always one root value? I wonder if can do that (and if we should). From what I understood the GraphQL spec don't specify what the very first root value should be, maybe we can see if there's any other framework that do things differently?

As far as I'm aware yes, there will only be one root value. The root value can be anything you want it to be but I think it's fair for Strawberry to default it to something if it makes sense within the context of the library. I don't think other frameworks suffer from the same problem because they treat resolvers as static methods or they don't have the same python convention of self. Graphene suffers from this confusion as well so it would be great if we could avoid it in Strawberry.

I think that makes sense, I'm personally only worried that having something like this:

@strawberry.type
class A:
    @strawberry.field
    def field_a() -> str:
       return "Example"

Might look weird to beginners as they would expect a self argument, but maybe I'm worrying too much.

Because we have the @strawberry.field decorator omitting the self parameter might not be so confusing but I remember someone saying that editors like PyCharm will assume that the method should have a self parameter and show an error if it doesn't have one. I think mypy also complains.

Loving this library so far! I hope I'm not being a nuisance, or asking some un-useful questions! I'm trying to get involved more but I don't have a bunch of professional experience, so bear with me if something I ask is... seemingly obvious. Thanks!

You're not being a nuisance at all! It's great to question these assumptions and it's always helpful to hear the problems that new users have. It's easy to miss these things once you've been working on it for a while.


Another option would be to change how the root types are constructed to avoid having to define a type at all. For example:

import strawberry

@strawberry.field
def get_current_user(info) -> User:
	return User(username="jkimbo")

@strawberry.field
def get_user_by_username(username: str) -> User:
	return User(username=username)

@strawberry.mutation
def add_user(info, username: str) -> User:
	return User(username=username)

schema = strawberry.Schema(
	queries=[get_current_user, get_user_by_username],
	mutations=[add_user]
)

This way it's not surprising that get_current_user, get_user_by_username and add_user don't have a self parameter. I personally think this is a better way of defining Mutations but it could also be applied to defining the Query type. It doesn't change how other types are constructed, just the root types.

@patrick91
Copy link
Member

patrick91 commented Jan 14, 2021

Another option would be to change how the root types are constructed to avoid having to define a type at all. For example:

I'd avoid this, query, mutation and subscription are special types, but not that special enough to treat them different I think 🤔

Maybe I can try and see if I can find a way to pass the correct self to queries, mutations and subscriptions, even when sending multiple operations at once, if that's possible. What do you think?

@AadamZ5
Copy link
Author

AadamZ5 commented Jan 24, 2021

I'd avoid this, query, mutation and subscription are special types, but not that special enough to treat them different I think 🤔

I agree with this statement too. The query, mutation, and subscription types are important, and shouldn't just be a list IMO. The extra functionality of a class (even as a static one) might be important to a lot of users.

Maybe I can try and see if I can find a way to pass the correct self to queries, mutations and subscriptions, even when sending multiple operations at once, if that's possible. What do you think?

If the root-level query objects (query, subscription, mutation) get an instance of themselves in self, then that would be great. I think a lot of new users would expect it to work this way before they read the docs, however that doesn't fully justify that it should be done. If you move forward with the idea, perhaps you could implement some sort of factory approach for creating the root-level query object instances? That way you can let the user customize how the class is initialized.

One thing I think you should consider as the project evolves (beyond what it has already): Is this library going to build GraphQL in Python, or make Python effortlessly turn into GraphQL? For example, the self parameter makes this library feel more like a standard "python thing", and it works how every new user would assume before they read the docs more. If the library deviates from the self parameter and functions how GraphQL natively does, with context parameters and logically static resolvers, then you deviate from the initial ideas most Python users have, and thats not an issue! I think if it's technically possible to do what's being discussed, go back and think about the purpose of the library, and what goal does it set out to accomplish. In short, should this feel "pythonic", or feel like a GraphQL implementation in Python?

My two cents: The library's decorator implementation feels magical, and it's wonderfully easy to get most of the setup and typing done. I was delighted when I found a more modern code-first approach. The resolving side is where I hit a learning curve. The idea of resolvers being (or working like) instance-bound functions was fresh in my head, which is how this issue was created.

@jkimbo
Copy link
Member

jkimbo commented Mar 16, 2021

Maybe I can try and see if I can find a way to pass the correct self to queries, mutations and subscriptions, even when sending multiple operations at once, if that's possible. What do you think?

@patrick91 did you managed to achieve this? I think I might have been mistaken when I said that you can execute a mutation and a query at the same time. According to the spec a document can define multiple operations (query or mutation) but when executing you need to define which one to run: https://spec.graphql.org/June2018/#sec-Executing-Requests

I still think that the root types are different enough to justify treating them differently. Even if we manage to automatically instantiate the right Query or Mutation instance, as @AadamZ5 points out, we would need to provide a factory method to allow people to customise how the instances get created. I think it's cleaner to sidestep the whole problem and just create schemas with a list of queries, mutations and subscriptions.


One thing I think you should consider as the project evolves (beyond what it has already): Is this library going to build GraphQL in Python, or make Python effortlessly turn into GraphQL?

@AadamZ5 in my head Strawberry is a library to build GraphQL in Python. That doesn't mean it can't be "pythonic" though and we should strive to make it as "pythonic" as possible.

@patrick91
Copy link
Member

I think it might ok to close this issue, we have documented how to access self/parent here: https://strawberry.rocks/docs/guides/accessing-parent-data#accessing-parents-data-in-a-method-resolver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants