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

Support @skip and @include directives #208

Merged
merged 6 commits into from
Jul 23, 2018
Merged

Conversation

mathewbyrne
Copy link
Contributor

@mathewbyrne mathewbyrne commented Jul 20, 2018

Adds support for @skip and @include directives. These should function according to the specification and skip or include fields, fragments, and inline fragments.

This change also includes passing the request context through to CollectFields.

See #156

Also thanks to @sergeyvlakh for his PRs #187 and #130

panic(err)
}
ret, ok := value.(bool)
if !ok {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vektah I noticed this panics when a variable is omitted from the query altogether. "Invalid" values like strings/numbers seem to get coerced into a bool. But there's probably somewhere better to report on missing variables?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, theres a post-validation step thats still missing. Ill add a card and make sure its covered before we :shipit:

Copy link
Collaborator

@vektah vektah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Could also use a test on default values

@@ -25,7 +26,8 @@ func (ec *executionContext) _{{$object.GQLType}}(ctx context.Context, sel ast.Se
}
{{- else }}
func (ec *executionContext) _{{$object.GQLType}}(ctx context.Context, sel ast.SelectionSet{{if not $object.Root}}, obj *{{$object.FullName}} {{end}}) graphql.Marshaler {
fields := graphql.CollectFields(ec.Doc, sel, {{$object.GQLType|lcFirst}}Implementors, ec.Variables)
reqCtx := graphql.GetRequestContext(ctx)
fields := graphql.CollectFields(reqCtx, sel, {{$object.GQLType|lcFirst}}Implementors)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe ctx?

@@ -51,23 +54,25 @@ func collectFields(doc *ast.QueryDocument, selSet ast.SelectionSet, satisfies []

f.Selections = append(f.Selections, sel.SelectionSet...)
case *ast.InlineFragment:
if !instanceOf(sel.TypeCondition, satisfies) {
if !shouldIncludeNode(sel.Directives, reqCtx.Variables) || !instanceOf(sel.TypeCondition, satisfies) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong, but when I did this, reqCtx had no default arguments (ones from document). So this query

query MyAwesomeQuery($shouldIncludeField: Boolean = false) {
    resolver @include(if: $shouldIncludeField) {
        __typename
    }
}

would still include in case no variables were explicitly passed. That's why I did this:
https://github.com/vektah/gqlgen/pull/187/files#diff-47a8f73d5b7f0dbad02ee4611bbedb10R79
BUT again, I might be very-very wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 it's probably worth a testcase

Mathew Byrne added 4 commits July 23, 2018 11:52
This is a snowflake implementation for skip/include directives based on
the graphql-js implementation.  Skip takes precedence here.
Adds a set of test cases for skip and include directives to the todo
example. Also now conforms to spec if both are included.
Internally it can still get to RequestContext as required.
@mathewbyrne mathewbyrne merged commit ffe4265 into next Jul 23, 2018
@vektah vektah deleted the directives-skip-include branch August 3, 2018 00:09
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
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

Successfully merging this pull request may close these issues.

3 participants