Closed AadamZ5 closed 3 months ago
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?
@AadamZ5 yep this is a known issue and it's because the resolver functions are actually static methods. It was discussed briefly here: https://github.com/strawberry-graphql/strawberry/discussions/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 ?
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 🤔
@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.
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 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!
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: https://github.com/strawberry-graphql/examples/issues/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)!
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.
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?
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.
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.
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
In an example like this,
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,
test_field2
resolver method.test_field3
resolver method.__init__
. Does overriding the initializer function break anything?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: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