strawberry: [bug?] custom directives have their first argument cut off during introspection

$ pip freeze | grep strawberry
strawberry-graphql==0.96.0

Here’s a quick repro:

import strawberry
from strawberry.directive import DirectiveLocation
from typing import Optional

@strawberry.directive(locations=[DirectiveLocation.QUERY])
def hello_world(
    foo: str,
    bar: str,
    baz: str,
):
    pass


@strawberry.type
class Query:
    _service: Optional[str]


schema = strawberry.federation.Schema(query=Query, directives=[hello_world])
print(schema.execute_sync("""
    query SubgraphIntrospectQuery {
        _service {
            sdl
        }
    }
""").data['_service']['sdl'])

$ python test.py
directive @helloWorld(bar: String!, baz: String!) on QUERY

type Query {
  _service: _Service!
  Service: String
}

scalar _Any

type _Service {
  sdl: String!
}

Let’s zoom in on that directive SDL: directive @helloWorld(bar: String!, baz: String!) on QUERY - what happened to thefoo arg?

I guess this behaviour is coming from the slicing here: https://github.com/strawberry-graphql/strawberry/blob/b8ead94d0cceb9eb5f9a6ea5a37123efd9bf48a3/strawberry/directive.py#L26

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 25 (25 by maintainers)

Commits related to this issue

Update schema-directives.md fix typo found in https://github.com/strawberry-graphql/strawberry/issues/1666 — committed to magicmark/strawberry by magicmark 2 years ago
Update schema-directives.md (#1667) fix typo found in https://github.com/strawberry-graphql/strawberry/issues/1666 — committed to strawberry-graphql/strawberry by magicmark 2 years ago
Fix first directive argument from being cut off This commit closes Issue #1666 by re-using the `_SPECIAL_ARGS` filtering logic of `StrawberryResolver` to prevent the first argument to directives from... — committed to skilkis/strawberry by skilkis 2 years ago
Improve reserved argument injection for resolvers This commit closes Issue #1666 by improving the injection of reserved arguments by providing an opt-in functionality to match reserved arguments by t... — committed to skilkis/strawberry by skilkis 2 years ago
Improve reserved argument injection for resolvers This commit closes Issue #1666 by improving the injection of reserved arguments by providing an opt-in functionality to match reserved arguments by t... — committed to skilkis/strawberry by skilkis 2 years ago
Improve reserved argument injection for resolvers This commit closes Issue #1666 by improving the injection of reserved arguments by providing an opt-in functionality to match reserved arguments by t... — committed to skilkis/strawberry by skilkis 2 years ago
Fix first directive argument from being cut off (#1713) * add failing test (cherry picked from commit 48549806c8bf2abfa2127b25ed37c91f6a65a26a) * Improve reserved argument injection for resolve... — committed to strawberry-graphql/strawberry by skilkis 2 years ago

Most upvoted comments

@skilkis, I’ll take a look at this soon, I’ll also chat with @bellini666 regards to context vars

patrick91 on Mar 23, 2022

@magicmark

So then how do we define DirectiveValue 🤔

i did try to type it correctly as well, but didn’t figure anything out. it should be possible if we make mypy plugin logic, but thats not very convenient and won’t work right with pyright / pycharm.

It would the same as Private 😃 https://github.com/strawberry-graphql/strawberry/blob/316ccb2d3cf1082969da0721bc494eb002188883/strawberry/private.py#L10-L13

This should be supported by all type checkers 😊

my only thought is that it’s a slight mind-bender coming from graphql-js and other frameworks where stuff is threaded around as params to resolvers, which are thought of as pure functions.

I also agree with this. would prefer it to come from the arguments. though at worst case in my codebase i would only do the magic retrieval of context variables in my strawberry layer.

+1, I still didn’t have time to get more familiar with contextvar, I wonder if there’s any performance implication too

I’d personally go for “dependency injection style” params, seems easier to reason about as a end user 😊

patrick91 on Mar 10, 2022

@bellini666 I spent some time to understand what you wrote and it’s really cool stuff, contextvars seem awesome and I can’t wait to use them in other projects! Just to confirm, was your intention to eventually remove info and root arguments from resolvers?

Yeah, contextvars works really well for cases like this! 😃

The removal of info and root arguments from resolvers is a possibility (not mandatory) if we go to the contextvars route. The major pro here is that we can simplify the resolvers a lot by removing the info/root “magic” from them, and also we can access the current root/info at any place, not only inside the resolver itself (as long as we are currently resolving it).

bellini666 on Mar 9, 2022

@skilkis sure, go for it 😊

patrick91 on Mar 8, 2022

I’m not familiar with contextvar so I’m not sure if they would work in all the cases 😃 aslo info and root are based on the current field/resolver, how would you update the contextvar?

I have started messing around with contextvars lately, and AFAIK, they should work in all cases. It is actually built for this use case in mind!

We just need to set the context values during the field resolution. It will also keep each async context separated, so 2 or more async fields being resolved at the same time will each have their own contexts.

Since code speaks better than words, I wrote a small proof of concept (https://github.com/strawberry-graphql/strawberry/pull/1672) and some tests to show how it works. Note that it works for sync/async resolvers, and the more interesting one is the no_params_field, where I could retrieve its root and info values without receiving them by args 😃

bellini666 on Feb 24, 2022

Looking at taking a stab at a fix for this - to clarify the (breaking?) logic you want: to receive the parent value (e.g. the value returned from the resolver that we’re directifying), the directive function must have a variable named value (could be in any position) and we’ll pass that in as a named argument. Then we can change this logic annotations = dict(islice(annotations.items(), 1, None)) to grab all the arguments, but filter out any argument named value

does this sound correct? my understanding:

pros: simple+understandable logic, works similar to the root param for resolvers

con: may be a breaking change and prevent folks from using that identifier as a directive arg name in the schema

yes, that’s all correct 😃

I was thinking about using a custom type for annotating root and value for example:

def resolver(instance: Root[MyType]) -> str: ...

def directive(v: FieldValue[str]) -> str: ...

what do you think of this approach? we could do this for the info parameter too 😃

/cc @strawberry-graphql/core

patrick91 on Feb 23, 2022

@magicmark that’s probably a bug, the code currently assumes that the first argument in the directive is the value returned from the resolver, for example:
@strawberry.directive(
    locations=[DirectiveLocation.FIELD], description="Make string uppercase"
)
async def uppercase(value: str):
    return value.upper()
We should probably change this logic to use the name of the argument 🤔

@patrick91

Looking at taking a stab at a fix for this - to clarify the (breaking?) logic you want: to receive the parent value (e.g. the value returned from the resolver that we’re directifying), the directive function must have a variable named value (could be in any position) and we’ll pass that in as a named argument. Then we can change this logic annotations = dict(islice(annotations.items(), 1, None)) to grab all the arguments, but filter out any argument named value

does this sound correct? my understanding:

pros: simple+understandable logic, works similar to the root param for resolvers con: may be a breaking change and prevent folks from using that identifier as a directive arg name in the schema

magicmark on Feb 23, 2022