Should class resolvers always have the self argument?

[jkimbo]

I wanted to ask people about how they feel about class resolvers having a self argument or not? It's not strictly necessary because resolver functions are all actually treated as static methods and so the first parameter is the value that was resolved from the "parent" in the schema.

For example:

import strawberry

@strawberry.type
class User:
    @strawberry.field
    def name(self) -> str:
        # This is the value returned from the `user` resolver in `Query` not an instance of `User`
        assert self == "Patrick"

@strawberry.type
class Query:
    @strawberry.field
    def user(self) -> User:
        assert self is None  # by default the "root" value is `None`
        return "Patrick"

The fact that resolvers for Object Types don't have to return an instance of that type can be a useful optimisation tool (especially when dealing with things that often look like the type i.e. Django models) but in my experience (from Graphene) it does lead to a lot of confusion because it's not very "pythonic".

As far as I see it we have a few options:

1. Lean into the fact that resolvers are static methods and so don't ever use self as the first argument in documentation. Instead use root or source to make it clear that it's not an instance of the current type.
2. Use self consistently in documentation and also make sure that resolvers that return another type always return an instance of that type so that self is always "correct". Document the fact that you can return anything from a resolver as an advanced feature only.
3. Enforce that resolvers for Object Types always return an instance of that type so that self is always "correct".

My current thinking is that we go with option 2 but what do other people think?

[patrick91]

I'd go for option 2, especially since we have a good-ish support for MyPy, that plus a good convention should be enough for basic usages, and we still allow for more advanced use cases :)

I also want to see if we can make MyPy think that a strawberry type is a protocol too, so that when returning a compatible type it won't complain :)

[AadamZ5]

I think option 2 is viable, but as it stands now, option 1 looks like the easiest fix from what the current functionality is. A variable rename seems easier than type-enforcement, but maybe the type-enforcement is a feature of the library you'd like to have for users. Nonetheless, a variable rename to something like root makes more sense, but the JavaScript execution examples on GraphQL show the same type of parameter with a name of obj which I believe is also perfectly suitable.

The big point to get across, is that this variable -or the value passed into it- is not the same as self. It's simply an object that is passed into the resolver, if one is available. It isn't guaranteed that it the value is an instance of the class you're writing the resolver for. Since it isn't the same as self, it shouldn't be called self IMO.

If you choose option 1, I think it's important to conceptually differentiate the resolver method by telling users it's like a static method, but in this case, the first parameter will always be the resolved object from the parent resolver, whatever the value actually is. Even if this isn't documented at the first use of @strawberry.field in the documentation, a different variable name like root or obj will surely communicate that the behavior is not the same as self, and lead users to investigate more before being confused.

All that being said, I definitely would like the functionality of option 2 or 3. Ensuring a proper instance on self would be very pythonic. The only thing that would still bug me is that (I presume) the Query, Mutation, and Subscription root resolvers still won't get a self parameter, because they generate the initial object value. If this were changed, and they did get an instance of themselves, this would be a wonderfully pythonic solution to the problem.

Option 1 seems the easiest most immediate fix, but Option 2 and 3 seem like the more pythonic way to do things.

[mattalbr]

Ran into this recently, and one thing that's frustrating is that there is currently no way for users to implement option 1 because of how https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/types/fields/resolver.py is implemented to explicitly match "self" "root" and "info". As a user, if you use a library that returns a different type, your only option right now is to use "self" which is IMO very confusing since it's not accurate.

IMO a nice way to implement 1) would be to make a type annotation similar to strawberry.Private, but like strawberry.Source and do parameter resolution for that. That would remove the reliance on name matching, which feels not super pythonic. Then you could have something like

@strawberry.type
class Foo:
  @strawberry.field
  @staticmethod  # Needed for MyPy
  async def bar(can_be_whatever_name_youd_like: strawberry.Source[CanBeWhateverYoudLike], info: strawberry.Info):
    pass

I'm open to writing a PR implementing strawberry.Source if it's something you're open to @patrick91

[patrick91]

We do that for info ☺️

I think we discussed doing it for root too, @mattalbr happy to review the PR for this! Let's only make sure we use the right name, I don't remember how if Root is more correct than Source, or even Parent

[mattalbr]

Of those, Parent seems the most intuitive to me! I'll draft the PR with Parent and it's not hard to do a search and replace if we change our mind on naming.

[mattalbr]

Closing the loop here (or really continuing the discussion), #3017 is merged implementing strawberry.Parent (thank you @patrick91 for the reviews and support!). What remains for this discussion is:

A. What parent aliases do we recommend, and thus choose for our documentation?
B. What parent aliases do we support, potentially as legacy options to maintain backwards compatibility?
C. Do we want to enforce type annotations on resolvers?

My personal take is that I find self and the hidden staticmethod within strawberry.field to be convenient and concise, but ultimately confusing, and I believe they contributed to my team and myself struggling to understand the dataflow of strawberry resolvers. While it's nice to hide internals, in this case I feel the lack of understanding of the dataflow makes it harder to adapt strawberry outside of the simple examples given in the documentation. It feels, to me, magic and a bit misleading.

An example I experienced: the strawberry-sqlalchemy library that my team uses, similar to a lot of ORM mapper libraries, returns a different type than the resolver's annotation. Because the self made the resolvers look like methods, I wrongly assumed that we would call returned_object.resolver_name(args) and I spun my wheels for quite a while because of this, not understanding why self.other_resolver() from within a resolver (yes I know now not a good design) gave me an error message about 'ORMType' has no attribute 'other_resolver', thinking I would have to convert my ORM objects to strawberry types when returning them but realizing the strawberry types generated by the library don't have useful constructors, mistakenly trying to convert the ORM object returned into the strawberry type by doing some nasty things, and really just being fundamentally confused about how all of this worked. I'm not sure how I could have known that we actually call return_type_annotation.resolver_name(returned_object, args) without reading through the code. I personally would way rather be explicit and transparent even at the cost of being slightly verbose.

With that said, my preference is:

A. Use strawberry.Parent and staticmethod or resolver= throughout documentation.
B. Deprecate at least root, and probably self, but give users plenty of time to migrate with a deprecation warning and a codemod. If we do continue to allow self, treat it as equivalent to strawberry.Parent[type(self)] for the purposes of C.
C. Allow a strict_types flag when creating the schema that would do runtime type enforcement. Default this flag to false, both for backwards compatibility and for performance, but suggest that people enable it in tests.

Had a nice conversation with Patrick about this yesterday, who I know has his own thoughts, but I figured it would be good to move the discussion here so others can weigh in, and whatever we do we might want to wait a minute to see how strawberry.Parent is used and perceived by others in real-world settings.