Proposal: Strawberry decorator #473

jkimbo · 2020-09-28T16:41:01Z

Add make_strawberry_decorator function to create decorators that can be applied to resolvers.

I'm creating this as draft PR to see if this approach seems like a viable one. If people are happy with it then I can add some documentation as well.

Description

Being able to apply decorators to resolver functions is a valuable feature because it allows the developer to encapsulate common logic that needs to be applied across multiple resolvers. It also solves most of the use cases for directives in SDL based GraphQL libraries. However, because Strawberry uses dependency injection to determine what arguments to pass to the resolver function, a plain decorator implementation would only have access to the arguments that the resolver function defines. This unnecessarily couples the decorator and the resolver function.

This PR introduces a new function make_strawberry_decorator that decorates a decorator function (functional programming ftw) and wraps up the required logic so that the decorator function body can have access to the source and info arguments even if the resolver doesn't require them.

For example:

from strawberry.decorator import make_strawberry_decorator

@make_strawberry_decorator
def upper_case(resolver, source, info, **kwargs):
	result = resolver(**kwargs)
    return result.upper()

@strawberry.type
class Query:
	@strawberry.field
    @upper_case
    def greeting() -> str:
    	return "hi"

schema = strawberry.Schema(query=Query)
result = schema.execute_sync("query { greeting }")

assert not result.errors
assert result.data == {"greeting": "HI"}

Note: in the PR I also hoisted the get_func_args call to init time rather than runtime. This probably means the cache on get_func_args can be removed but I haven't done that as part of this change because I'm not sure what other implications that would have.

Extra note: this is an aside but IMO using decorators replaces the need to have specific permissions support on strawberry.field because it could be replaced with a decorator like this:

class IsAuthenticated(BasePermission):
	message = "User is not authenticated"

    def has_permission(self, source, info):
    	return False

def requires_permissions(permission_classes):
	@make_strawberry_decorator
	def wrapped(resolver, source, info, **kwargs):
		for permission_class in permission_classes:
            permission = permission_class()

            if not permission.has_permission(source, info, **kwargs):
                message = getattr(permission, "message", None)
                raise PermissionError(message)

		return resolver(**kwargs)

	return wrapped

@strawberry.type
class Query:
	@strawberry.field
	@requires_permissions([IsAuthenticated])
    def user(self, info) -> str:
    	return "patrick"

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

botberry · 2020-09-28T16:41:37Z

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

Release type: patch

Description of the changes, ideally with some examples, if adding a new feature.

Release type can be one of patch, minor or major. We use semver,so make sure to pick the appropriate type. If in doubt feel free to ask :)

codecov · 2020-09-28T16:43:22Z

Codecov Report

Merging #473 (8b796e0) into main (877f888) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #473   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files          70       70           
  Lines        2273     2273           
  Branches      312      312           
=======================================
  Hits         2236     2236           
  Misses         19       19           
  Partials       18       18

AndrewIngram · 2020-09-28T17:15:08Z

On naming, maybe as_resolver_decorator, or to_resolver_decorator or even just resolver_decorator?

BryceBeagle · 2020-09-28T19:03:33Z

Extra note: this is an aside but IMO using decorators replaces the need to have specific permissions support on strawberry.field because it could be replaced with a decorator like this:

Could this also be used to implement a @deprecated decorator/directive?

BryceBeagle · 2020-09-28T19:08:17Z

strawberry/decorator.py

+            # has the opportunity to modify them
+            return func(wrapped, source, info=info, **kwargs)
+
+        wrapped_resolver._strawberry_decorator = True


Could we return a Callable wrapper class instead of the function monkeypatched? A benefit now would be the ability to use isinstance instead of hasattr

This would also play into my effort to try to remove a lot of the monkeypatching from elsewhere in the codebase

Sure that would also work. I'll refactor the code to that.

As an aside what's the benefit of using isinstance instead of hasattr? To me they seem pretty much equivalent and I'd imagine that isinstance has (marginally) worse performance.

At minimum, we can add typing annotations for mypy/IDEs to provide warnings/suggestions (without having to wait for typing.Protocol in 3.8).

Symbols are also more easily refactored and indexed than strings (although, this specific argument could potentially be fixed with variables for setattr/getattr/hasattr).

without having to wait for typing.Protocol in 3.8

we can already use them via typing_extensions :)

So I've just had a go at this I don't actually think it's possible. I changed it to something like this:

class StrawberryDecorator: def __init__(self, resolver): self.resolver = resolver functools.update_wrapper(self, resolver) # this is necessary so that `StrawberryField` can extract the arguments and name def __call__(self, *args, **kwargs): return self.resolver(*args, **kwargs) def make_strawberry_decorator(func): def decorator(resolver): function_args = get_func_args(resolver) @functools.wraps(resolver) def wrapped_resolver(source, info, **kwargs): def wrapped(**kwargs): if isinstance(resolver, StrawberryDecorator): # <-- this does not work return resolver(source, info, **kwargs) args, extra_kwargs = get_resolver_arguments(function_args, source, info) return resolver(*args, **extra_kwargs, **kwargs) return func(wrapped, source, info=info, **kwargs) return StrawberryDecorator(wrapped_resolver) return decorator

Because of the functools.update_wrapper call the isinstance(resolver, StrawberrryDecorator) doesn't work which defeats the point of using the class in the first place.

I think the best option is to just stick with the monkeypatching even if it's not ideal.

BryceBeagle · 2020-09-28T19:15:03Z

On naming, maybe as_resolver_decorator, or to_resolver_decorator or even just resolver_decorator?

Or even more simply: just strawberry.decorator Do we anticipate needing custom decorator support for anything other than resolvers?

AndrewIngram · 2020-09-28T19:21:04Z

On naming, maybe as_resolver_decorator, or to_resolver_decorator or even just resolver_decorator?

Or even more simply: just strawberry.decorator Do we anticipate needing custom decorator support for anything other than resolvers?

I feel like being explicit about the use case is a good idea. I can't think of any further use cases, but that doesn't negate my main reasoning here.

AndrewIngram · 2020-09-28T19:23:35Z

Extra note: this is an aside but IMO using decorators replaces the need to have specific permissions support on strawberry.field because it could be replaced with a decorator like this:

Could this also be used to implement a @deprecated decorator/directive?

In GraphQL terms, deprecated isn't actually a directive, it's just represented as one in the SDL. Given Strawberry doesn't lean on the SDL, it doesn't feel right to parrot that misunderstanding

AndrewIngram · 2020-09-28T19:25:51Z

Actually, scrub that. As long as it's clear in the docs that decorators != directives, a deprecated decorator feels like a good idea, it'd be quite ergonomic. Worth spiking out?

jkimbo · 2020-09-28T20:44:29Z

Actually, scrub that. As long as it's clear in the docs that decorators != directives, a deprecated decorator feels like a good idea, it'd be quite ergonomic. Worth spiking out?

How would the decorator mark the field as deprecated? The decorator gets applied before the @strawberry.field decorator so it can't modify the field.

patrick91 · 2020-09-28T22:26:37Z

Extra note: this is an aside but IMO using decorators replaces the need to have specific permissions support on strawberry.field because it could be replaced with a decorator like this [...]

I like the sound of this, but how can we replicate the same behaviour with simple fields, like this:

@strawberry.type
class User:
    email: str = strawberry.field(permission_classes=[])

?

AndrewIngram · 2020-09-28T22:50:11Z

Extra note: this is an aside but IMO using decorators replaces the need to have specific permissions support on strawberry.field because it could be replaced with a decorator like this [...]

I like the sound of this, but how can we replicate the same behaviour with simple fields, like this:
@strawberry.type
class User:
    email: str = strawberry.field(permission_classes=[])
?

What about exposing graphql-core's default resolver (https://github.com/graphql-python/graphql-core/blob/3359fe60d9940b174270426438a89d629b6cc6e2/src/graphql/execution/execute.py#L1224), then you can just wrap it.

@strawberry.type
class User:
    email: str = strawberry.field(resolver=permission_classes([a,b,c])(strawberry.default_resolver))

or

@strawberry.type
class Query:
   name: str = strawberry.field(resolver=upper_case(strawberry.default_resolver))

But if it becomes a common use case, maybe provide a little sugar:

@strawberry.type
class Query:
   name: str = strawberry.field(resolver_decorators=[permission_classes([a,b,c]), upper_case)])

But the latter could be done in user-land by wrapping strawberry.field anyway.

jkimbo · 2020-09-29T08:33:37Z

Extra note: this is an aside but IMO using decorators replaces the need to have specific permissions support on strawberry.field because it could be replaced with a decorator like this [...]

I like the sound of this, but how can we replicate the same behaviour with simple fields, like this:

Good point, I hadn't considered that. Exposing the default resolver (as @AndrewIngram suggested) makes sense to me. Replacing the permission_classes argument is not a core part of this proposal though and I think the proposal still adds value. It's probably worth discussing in another setting.

patrick91 · 2020-09-29T10:05:15Z

Good point, I hadn't considered that. Exposing the default resolver (as @AndrewIngram suggested) makes sense to me. Replacing the permission_classes argument is not a core part of this proposal though and I think the proposal still adds value. It's probably worth discussing in another setting.

Yup, didn't try to dismiss the proposal, I was curious about the plain field usecase, I think it is worth discussing it now as well, since this decorator pattern can change how we do things (for the better) :)

strawberry/decorator.py

patrick91 · 2020-09-29T10:13:24Z

strawberry/decorator.py

+            # has the opportunity to modify them
+            return func(wrapped, source, info=info, **kwargs)
+
+        wrapped_resolver._strawberry_decorator = True


without having to wait for typing.Protocol in 3.8

we can already use them via typing_extensions :)

patrick91 · 2020-09-29T10:13:53Z

strawberry/resolvers.py

@@ -6,7 +6,34 @@
 from .utils.inspect import get_func_args


+def get_resolver_arguments(function_args, source, info):


tests/test_decorator.py

jkimbo · 2020-10-19T16:41:44Z

So @patrick91 @AndrewIngram I've spent some time trying to get decorators working for simple fields and it's turned out to be much harder than I expected. Exposing the default resolver is not that straightforward since the resolver needs to know the name of the current field which is not available when the decorator is called. It could probably be done with lots of special casing but I don't think it's worth it.

That has lead me to this API instead:

@strawberry.type
class Query:
   name: str = strawberry.field(decorators=[upper_case])

What do you think?

AndrewIngram · 2020-10-19T18:30:58Z

@jkimbo what did the code look like where you needed the field but didn't have it?

jkimbo · 2020-10-19T19:19:45Z

@AndrewIngram like this:

@strawberry.type
class Query:
   name: str = strawberry.field(resolver=upper_case(strawberry.default_field_resolver))

AndrewIngram · 2020-10-20T08:49:22Z

@AndrewIngram like this:

@strawberry.type
class Query:
   name: str = strawberry.field(resolver=upper_case(strawberry.default_field_resolver))

I'm still not seeing why this needs to know the field, isn't the default resolver just a function that looks at the info object when it's executed? What's not working at the time you're wrapping the function?

jkimbo · 2020-10-20T08:59:49Z

Because strawberry camel cases the field name you can't rely on the name on the info object to get the right attribute. I guess we could un-camel-case it to try and get the correct attribute...

BryceBeagle · 2020-10-21T08:14:01Z

What if we did something along the lines of this:

class StrawberryDecorator:
    def __init__(self, func: Callable, *args, **kwargs):
        self.func = func
    
    def __call__(self, field: StrawberryField) -> StrawberryField:
        wrapped_resolver = lambda: self.func(field.resolver)
        field.resolver = wrapped_resolver
        return field

#######

upper_case = StrawberryDecorator(lambda string: string.upper())

@strawberry.type
class Query:
    @upper_case
    @strawberry.field
    def name() -> str:
        return "blah"

    other_name: str = upper_case(strawberry.field())

Pros:

Default resolver isn't exposed
Normal decorator syntax

Cons:

Order of decorators matters (this could potentially be solved by allowing strawberry.field to take StrawberryDecorators)

jkimbo · 2020-10-22T18:58:36Z

@BryceBeagle hmm that might be a better option. I'd have to play around to see if it's possible.

patrick91 · 2020-10-22T21:27:05Z

@jkimbo what if, at type generation we replace the default resolver with something that had the field name in it like partial(default_resolver, field_name='abc')? 🤔

jkimbo · 2020-10-23T09:18:26Z

@patrick91 I tried that but the issue is that the decorator function calls the resolver so it needs to know the field name when it's created (i.e. when strawberry.field(resolver=upper_case()) is evaluated. The only way around it I can think of is if the resolver can gain access to the field object somehow but I think that would get quite messy.

benwis · 2021-02-02T07:21:09Z

Where did we land on this? I’d love to use this for some auth decorators and it looks pretty good to me :)

patrick91 · 2021-02-02T17:15:20Z

Where did we land on this? I’d love to use this for some auth decorators and it looks pretty good to me :)

@benwis we are working on some refactoring before landing this, can't commit to a timeline yet, but it shouldn't take too long.

In the meanwhile I'll rebase the PR later today so it's easier to merge it in future :)

patrick91 · 2021-03-12T20:32:12Z

@jkimbo @BryceBeagle I've rebased and broken this PR. Will try to fix it over the weekend

And add check for strawberry decorators

patrick91 · 2021-03-15T20:56:59Z

I've spent a bit of time on this PR and looks @BryceBeagle updates plus @functools.wraps made things much easier to implement, this just works now:

def upper_case(resolver):
    @wraps(resolver)
    def wrapped(*args, **kwargs):
        return resolver(*args, **kwargs).upper()

    return wrapped

@strawberry.type
class Query:
    @strawberry.field
    @upper_case
    def greeting(self) -> str:
        return "hi"

schema = strawberry.Schema(query=Query)
result = schema.execute_sync("query { greeting }")

assert not result.errors
assert result.data == {"greeting": "HI"}

so I think we add the tests and add some docs for this.

functools.wraps is used to copy type annotations (and other things too, but mostly that)

benwis · 2021-04-02T00:14:10Z

I love this feature and have already started using it based on this PR. If you guys can point me in the right direction, I'm happy to try my hand at creating some docs/tests to get this merged in.

patrick91 · 2021-04-03T15:10:04Z

I love this feature and have already started using it based on this PR. If you guys can point me in the right direction, I'm happy to try my hand at creating some docs/tests to get this merged in.

hey @benwis I think you should be able to use decorators already (based on the tests), I think we might want to provide our own version of functools.wraps that know how to deal with the info parameter, but you can already use decorators, as long as you always pass info :)

benwis · 2021-04-07T02:27:57Z

I love this feature and have already started using it based on this PR. If you guys can point me in the right direction, I'm happy to try my hand at creating some docs/tests to get this merged in.

hey @benwis I think you should be able to use decorators already (based on the tests), I think we might want to provide our own version of functools.wraps that know how to deal with the info parameter, but you can already use decorators, as long as you always pass info :)

That's great news. Saves me a lot of trouble. Still willing to help write tests or docs if that's helpful!

navyamehta · 2021-11-12T22:31:30Z

+1 for this feature, really looking forward as we need it in our prod setup. Would be glad to help out

patrick91 · 2023-03-10T14:29:43Z

Closing at this was implemented by @erikwrede in #2567

jkimbo requested review from patrick91 and BryceBeagle September 28, 2020 16:41

BryceBeagle reviewed Sep 28, 2020

View reviewed changes

patrick91 reviewed Sep 29, 2020

View reviewed changes

jkimbo force-pushed the strawberry-decorator branch from 6bf308e to b44f7a4 Compare October 18, 2020 15:18

patrick91 mentioned this pull request Oct 26, 2020

Add support for deprecation_reason on field #530

Merged

11 tasks

patrick91 mentioned this pull request Nov 26, 2020

Cannot decorate @field method #601

Closed

patrick91 force-pushed the strawberry-decorator branch 2 times, most recently from fed79be to 45a1737 Compare March 12, 2021 19:46

Base automatically changed from master to main March 14, 2021 19:02

jkimbo and others added 4 commits March 15, 2021 19:51

Move argument resolution outside of runtime

1087043

And add check for strawberry decorators

Add make_strawberry_decorator function

6e466c4

Use decorators argument

3729dee

WIP

25f7419

patrick91 force-pushed the strawberry-decorator branch from 45a1737 to 25f7419 Compare March 15, 2021 19:52

Remove decorator function

eb6f014

patrick91 force-pushed the strawberry-decorator branch from b1c0c5c to eb6f014 Compare March 15, 2021 20:26

Move tests inside schema tests

8b796e0

patrick91 mentioned this pull request Apr 5, 2021

WIP: field data processors and validators #809

Closed

11 tasks

jkimbo mentioned this pull request May 7, 2021

Proposal: Custom fields #920

Closed

jkimbo self-assigned this Sep 7, 2021

erikwrede mentioned this pull request Mar 10, 2023

Custom field API discussion #2168

Closed

patrick91 closed this Mar 10, 2023

		@@ -6,7 +6,34 @@
		from .utils.inspect import get_func_args


		def get_resolver_arguments(function_args, source, info):

Proposal: Strawberry decorator #473

Proposal: Strawberry decorator #473

Conversation

jkimbo commented Sep 28, 2020

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

botberry commented Sep 28, 2020

codecov bot commented Sep 28, 2020 • edited Loading

Codecov Report

AndrewIngram commented Sep 28, 2020

BryceBeagle commented Sep 28, 2020

BryceBeagle Sep 28, 2020

Choose a reason for hiding this comment

BryceBeagle Sep 28, 2020

Choose a reason for hiding this comment

jkimbo Sep 28, 2020

Choose a reason for hiding this comment

BryceBeagle Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

patrick91 Sep 29, 2020

Choose a reason for hiding this comment

jkimbo Oct 18, 2020

Choose a reason for hiding this comment

BryceBeagle commented Sep 28, 2020

AndrewIngram commented Sep 28, 2020

AndrewIngram commented Sep 28, 2020

AndrewIngram commented Sep 28, 2020

jkimbo commented Sep 28, 2020 • edited Loading

patrick91 commented Sep 28, 2020

AndrewIngram commented Sep 28, 2020

jkimbo commented Sep 29, 2020

patrick91 commented Sep 29, 2020

patrick91 Sep 29, 2020

Choose a reason for hiding this comment

patrick91 Sep 29, 2020

Choose a reason for hiding this comment

jkimbo commented Oct 19, 2020

AndrewIngram commented Oct 19, 2020

jkimbo commented Oct 19, 2020

AndrewIngram commented Oct 20, 2020

jkimbo commented Oct 20, 2020

BryceBeagle commented Oct 21, 2020 • edited Loading

jkimbo commented Oct 22, 2020

patrick91 commented Oct 22, 2020

jkimbo commented Oct 23, 2020

benwis commented Feb 2, 2021

patrick91 commented Feb 2, 2021

patrick91 commented Mar 12, 2021

patrick91 commented Mar 15, 2021

benwis commented Apr 2, 2021

patrick91 commented Apr 3, 2021

benwis commented Apr 7, 2021

navyamehta commented Nov 12, 2021

patrick91 commented Mar 10, 2023

codecov bot commented Sep 28, 2020 •

edited

Loading

BryceBeagle Sep 28, 2020 •

edited

Loading

jkimbo commented Sep 28, 2020 •

edited

Loading

BryceBeagle commented Oct 21, 2020 •

edited

Loading