Added support for @include/@skip directives. #130

sergeyvlakh · 2018-06-11T12:29:43Z

Overview

Resolves #126
Added support for builtin @skip/@include directives. They only work with $if argument (!).
This supports syntax with inline variables and default ones.

It is very-very-very WIP PR. I'm really looking into ways of improving this code to actually bring in support for directives!

Default variables case

Query:

query($hero:Boolean = true){
  hero @include (if:$hero) {
    name
  }
}

Result:

{
  "data": {}
}

As expected.

Inline variables case

Query:

{
  hero @include (if:false) {
    name
  }
}

Result:

{
  "data": {}
}

As expected.

Passed variables with default values

Query:

query($hero:Boolean = false){
  hero @include (if:$hero) {
    name
  }
}

Variables:

{
"hero": true
}

Result:

{
  "data": {
    "hero": {
      "name": "R2-D2"
    }
  }
}

As expected.

vektah · 2018-06-12T07:50:55Z

Neat!

I think I'm happy to add a hacky solution temporarily until the new parser lands. Once thats in we will need a more generic user pluggable way to define directives, and there will need to be a bit of planning around that to support various use cases (authentication, api fetch resolvers, caching).

vektah · 2018-06-12T07:52:47Z

graphql/exec.go

+	fieldAllowed := true
+	switch arg.(type) {
+	case *common.Variable:
+		{


I'm surprised none of the linters fire on this, you don't need curlies here.

vektah · 2018-06-12T07:53:28Z

graphql/exec.go

+		}
+	default:
+		// Custom directives are not supported yet
+		return fieldAllowed


probably should return true. Do nothing rather then do something unexpected. Or error.

vektah · 2018-06-12T07:56:08Z

graphql/exec.go

 func collectFields(doc *query.Document, selSet []query.Selection, satisfies []string, variables map[string]interface{}, visited map[string]bool) []CollectedField {
 	var groupedFields []CollectedField
-
+	oop, err := doc.GetOperation("")


😟

Maybe these methods should be refactored to take the request context instead of document so they can get the correct operation

This comment sill applies, everywhere that calls CollectFields is grabbing the doc out of the RequestContext embedded in execution context, they could pretty easily be updated to do

fields := graphql.CollectFields(ec.RequestContext, sel, implementors)

instead of

fields := graphql.CollectFields(ec.Doc, sel, implementors, ec.Variables)

and then you have access to the right operation instead of assuming it was an anonymous operation.

vektah · 2018-06-12T07:58:54Z

graphql/exec.go

+	if variables == nil {
+		variables = make(map[string]interface{})
+	}
+	for _, variable := range operation.Vars {


This seems like a lot of work to do on every fieldset, I think this goes away if we pass context in?

If there will be one specific place which will have both variables and their default values, this will be super good!

You can do this check inline, the one place you need the value from a variable, this function shouldn't exist.

And it really shouldn't mutate variables, they should be static after the initial validation pass.

sergeyvlakh · 2018-06-12T13:44:19Z

Thanks for addressing some comments. I will try to update PR ASAP.

sergeyvlakh · 2018-06-13T09:23:14Z

@vektah could you please do review once again? Thanks in advance :}

vektah · 2018-06-21T10:13:47Z

graphql/exec.go

+	fieldAllowed := true
+	switch arg.(type) {
+	case *common.Variable:
+		value := arg.Value(variables)


Why can't you pass the operation in here and do the variable lookup only when its actually referenced, instead of every resolver?

populateVariables is a big smell to me, its going to run on every resolver regardless of if there is even a directive.

Will fix ASAP!

vektah · 2018-06-21T10:19:45Z

graphql/exec.go

 func collectFields(doc *query.Document, selSet []query.Selection, satisfies []string, variables map[string]interface{}, visited map[string]bool) []CollectedField {
 	var groupedFields []CollectedField
-
+	oop, err := doc.GetOperation("")


This comment sill applies, everywhere that calls CollectFields is grabbing the doc out of the RequestContext embedded in execution context, they could pretty easily be updated to do

fields := graphql.CollectFields(ec.RequestContext, sel, implementors)

instead of

fields := graphql.CollectFields(ec.Doc, sel, implementors, ec.Variables)

and then you have access to the right operation instead of assuming it was an anonymous operation.

sergeyvlakh · 2018-06-21T14:19:01Z

@vektah thanks for comments! Will try to fix ASAP!

sergeyvlakh · 2018-06-25T07:00:35Z

@vektah
I've moved populateVariables to block only if directive present. Regarding your comment about request ctx I'm not sure. Should I change gotpl to achieve this one? Because we call fields := graphql.CollectFields(ec.Doc, sel, implementors, ec.Variables) in there.
And If it's not necessary, maybe merge this PR and land CollectFieldsWithCtx in another PR ?

vektah · 2018-06-25T14:43:51Z

Yeah, the template changes are going to be required. A graphql query can have a few forms:

a single, unnamed query, eg { hello { world }.
a single, named query, eg query Hello { hello { world } }
multiple named queries, and the name of the one you want to run.

The only thing this works for is the first one, in order to get the operation name (you are using "", but thats 100% a bug). All this resolution has already been done though, when the validation happened, and it was put in context so the rest of the runtime knows whats going on.

sergeyvlakh · 2018-06-26T10:48:58Z

Actually, as I understood, we are only taking one operation. And doesn't matter if it was named or not according to this code:

if operationName == "" {
		if len(d.Operations) > 1 {
			return nil, fmt.Errorf("more than one operation in query document and no operation name given")
		}
		for _, op := range d.Operations {
			return op, nil // return the one and only operation
		}
	}

vektah · 2018-06-29T05:54:50Z

Thats only if operationName == ""

sergeyvlakh · 2018-07-02T08:08:24Z

@vektah
Updated. I've added OperationName to requestCtx. It wasn't in in previously. Now I'm calling
getOperationName(reqctx.OperationName)
I'm not sure what are those errors in circleci and how do I fix them ?

- prevents possible collision with an object type named `Short`

* Update generated.gotpl * Update models.gotpl * Update data.go * update go generate * revert code changes

vektah · 2018-07-13T08:20:50Z

The new parser is nearly done now, full directive support should only be a few weeks away.

sergeyvlakh · 2018-07-13T08:52:52Z

Great. I'll close second one then. Thanks

Added support for @include/@Skip directives.

ff727d6

vektah reviewed Jun 12, 2018

View reviewed changes

sergeyvlakh added 2 commits June 13, 2018 12:13

CR fixes

3968db2

Prevent panic in case more than 1 operation in document.

ae67139

vektah reviewed Jun 21, 2018

View reviewed changes

Call populate variables only if directive present

9f65c35

Call populate variables only if directive present

b801614

sergeyvlakh and others added 13 commits July 3, 2018 11:20

go generate ./...

5099961

go generate ./... once again

4316541

skip fields with incompatible types

612cbe5

add "short" resolver interface

e97c4c3

from probably makes more sense

ff73dd5

fix template formatting

5104250

fix GOPATH case mismatch issue

4e6d17b

fix example links

3c0a6d2

run go generate

6ea844a

change from ShortResolver to ShortResolvers

38b116d

- prevents possible collision with an object type named `Short`

Use recognized Code generated header

0dcb4b4

Update getting started guide

babfa68

Update generated headers to match convention. (#139)

d2dba66

* Update generated.gotpl * Update models.gotpl * Update data.go * update go generate * revert code changes

vvakame and others added 24 commits July 13, 2018 11:08

Refactor types.json parsing

72ce679

Implement types.yaml parsing

0130f84

Add format check to -typemap argument

104fe74

address comment

e7ef9b7

Convert todo example to new resolver syntax

5a267e7

Dont show compilation errors until after codegen

baf05ff

shh errcheck

a374607

cleanup new config

743574f

add .idea/ to .gitignore

05bc301

move to .gqlgen.yml

f3e8798

update docs

5f041cb

add resolver option support to field

de6042f

gometalinter should cover all packages

574a88f

Search for config

b8c6a3d

Add some basic docs on the new config file

529b829

Check models config for package-only specs

e223a2b

Add error message when specifying path in package name

b1c3a0a

Handle package name mismatch with dirname

b75dc36

Check that exec and model filenames end in *.go

de259c1

Inline ImportPath strings

8dfd28e

Better handling of generated package name

a3cf2e4

Fix a bug custom scalar marshallers in external packages

e0cd22e

Call populate variables only if directive present

f1f7fc0

go generate ./...

e748fa2

sergeyvlakh closed this Jul 13, 2018

vvakame mentioned this pull request Jul 15, 2018

Planning for directive support #156

Closed

mathewbyrne mentioned this pull request Jul 20, 2018

Support @skip and @include directives #208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for @include/@skip directives. #130

Added support for @include/@skip directives. #130

sergeyvlakh commented Jun 11, 2018

vektah commented Jun 12, 2018

vektah Jun 12, 2018

vektah Jun 12, 2018

vektah Jun 12, 2018

vektah Jun 21, 2018

vektah Jun 12, 2018

sergeyvlakh Jun 12, 2018

vektah Jun 25, 2018

sergeyvlakh commented Jun 12, 2018

sergeyvlakh commented Jun 13, 2018

vektah Jun 21, 2018

sergeyvlakh Jun 21, 2018

vektah Jun 21, 2018

sergeyvlakh commented Jun 21, 2018

sergeyvlakh commented Jun 25, 2018

vektah commented Jun 25, 2018

sergeyvlakh commented Jun 26, 2018

vektah commented Jun 29, 2018

sergeyvlakh commented Jul 2, 2018 •

edited

Loading

vektah commented Jul 13, 2018

sergeyvlakh commented Jul 13, 2018

Added support for @include/@skip directives. #130

Added support for @include/@skip directives. #130

Conversation

sergeyvlakh commented Jun 11, 2018

Overview

Default variables case

Inline variables case

Passed variables with default values

vektah commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyvlakh commented Jun 12, 2018

sergeyvlakh commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyvlakh commented Jun 21, 2018

sergeyvlakh commented Jun 25, 2018

vektah commented Jun 25, 2018

sergeyvlakh commented Jun 26, 2018

vektah commented Jun 29, 2018

sergeyvlakh commented Jul 2, 2018 • edited Loading

vektah commented Jul 13, 2018

sergeyvlakh commented Jul 13, 2018

sergeyvlakh commented Jul 2, 2018 •

edited

Loading