add types package

[rudle]

Part of #434 and related to #116 this change adds a new package containing all
types used by graphql-go in representing the GraphQL specification. The names
used in this package should match the specification as closely as possible.

In order to have cohesion, all internal packages that use GraphQL types have
been changed to use this new package.

This change is large but mostly mechanical. I recommend starting by reading
through the types package to build familiarity. I'll call out places in the
code where I made decisions and what the tradeoffs were.

No new tests were added but existing tests were changed to use the new types.

Regarding backward compatibility, the external MustParseSchema entrypoint still works as expected. We verified that no code changes were required by importing this change into our main GraphQL application.

[rudle]

I started integrating this package with an internal tool we have and realized I made an oversight in my initial submission. The *types.Schema is not actually publicly accessible! I added a getter but I'm open to other solutions that preserve backward compatibility too https://github.com/graph-gophers/graphql-go/pull/437/files#diff-98bf38fb290ecb143188e1585622022aaa0711a888da5e74b254e01a66d0eb99R86

[pavelnikolov]

Just out of curiosity - what is the internal tool that you are using with this code?

[pavelnikolov]

What is the point of making this interface public if its only method is private?

[rudle]

This interface was carried over from the old code where it had the same quirks of visibility. It needs to be public because there is code in internal/common and elsewhere that depends on it. As best as I can tell, the private function isSelection() is a clever way of imitating the Abstract Type pattern common in languages like Java. It doesn't really fit in Go. I don't think IsSelection() should be part of the public API but type Selection needs to be, otherwise we cannot represent all ASTs using the types package.

An alternative to this approach that I'd be interested to hear your thoughts on is to do away with this Abstract Type and do something like:

type Selection struct {
  Field *types.Field
  FragmentSpread *types.FragmentSpread
  InlineFragment *types.InlineFragment
}

where only one of the fields is set based on what the concrete type of Selection is.

This is more inline with the rest of the code in this repo, for example see ExecutableDefinition for a simpler way of representing the one-or-the-other pattern. The downside is that code that handles a Selection will need to use a chain of if statements instead of a switch case. Another downside is that it's not clear that a Selection will have exactly one of its fields set.

[pavelnikolov]

What is the common thing between the Field, FragmentSpread and InlineFragment that you want to use? Can't we come up with some reasonable single-method interface?

[rudle]

They don't have any behavior in common, they are different types. The reason they are related is because together they are types of Selection, which is defined in the specification here: http://spec.graphql.org/June2018/#Selection. Once they are inside of this interface, code can use a switch statement to unpack the abstract types into concrete types. Here is an example of how they are used.

I understand that in Go, we often use an interface to abstract behavior (ex. Writer, Reader) which is maybe where your question about what they have in common is coming from. This code, in my opinion, is a misuse of Go interfaces but it is also this way on the master branch and there isn't another obvious way to express this type of abstract type.

[pavelnikolov]

I understand that in Go, we often use an interface to abstract behavior (ex. Writer, Reader) which is maybe where your question about what they have in common is coming from.
Yes, this is exactly where the question is coming from.

is a misuse of Go interfaces
I wouldn't call it a misuse, but I wish there were a more elegant solution to this.

[pavelnikolov]

Here is an example of how they are used.
@rudle I reviewed your example and I think that this code can be significantly improved. It can have a method that returns the flattened selection. Then we can loop through all selections and append them to a slice. That way we'll use an interface with a common method and we'll not have to typecast. They do have something in common and we better use it properly to avoid the casting.

[rudle]

Hi @pavelnikolov is this what you were describing? This function could be used as an interface for Selections. We could improve on the names, this is just a proof of concept.

func (f *Field) FlatSelections(exDef ExecutableDefinition) []*Field {
	return []*Field{f}
}

func (f *FragmentSpread) FlatSelections(exDef ExecutableDefinition) []*Field {
	sels := exDef.Fragments.Get(f.Name.Name).Fragment.Selections
	flatFields := make([]*Field, 0, len(sels))
	for _, s := range sels {
		flatFields = append(flatFields, s.FlatSelections(exDef)...)
	}
	return flatFields
}

func (f *InlineFragment) FlatSelections(exDef ExecutableDefinition) []*Field {
	sels := f.Selections
	flatFields := make([]*Field, 0, len(sels))
	for _, s := range sels {
		flatFields = append(flatFields, s.FlatSelections(exDef)...)
	}
	return flatFields
}

After writing this function, I looked for places that took in types.Selection as an argument. I wasn't able to find any places where the need for type-casting would be removed by having this FlatSelection function available. The one place that this function would maybe help is in validateMaxDepth if we added error handling for the case of nil FragmentSpread.

I'm not convinced that this complicated code simplifies the rest of the library enough to include it.

[pavelnikolov]

Why is Document replaced with ExecutableDefinition?
As the spec says a document might include other things other than just ExecutableDefinition, no?

A GraphQL Document describes a complete file or request string operated on by a GraphQL service or client.

Although, it also says that:

GraphQL services which only seek to provide GraphQL query execution may choose to only include ExecutableDefinition and omit the TypeSystemDefinition and TypeSystemExtension rules from Definition.

The question is do we want to exclude TypeSystemDefinition and TypeSystemExtension?
This is not just a rename, it potentially changes the semantics of the API a little.

[rudle]

We felt that the type called Document did not match up with the specification of a Document. As you mention, a document may contain an ExecutableDefinition but also a TypeSystemDefinition and extensions. It's essentially the root type. The grammar describes this here: http://spec.graphql.org/June2018/#sec-Appendix-Grammar-Summary.Document and the type itself is documented here: https://github.com/graph-gophers/graphql-go/pull/437/files/130895f516efd0fc6c23dca24bcdf2a8cf0c24bf#diff-b349fbf81a907441c5bb68aa1c730e71b6492354cd594b63626d781dc0bd51e7R9

Rather than add more things to Document and refactor everything to work based on that root type, we thought that making this type more narrow (in name only) was a better choice as it makes this change a bit more surgical and less risky. Another advantage of this naming choice is that the name types.Document is free to use as the root of the AST to match the grammar and specification. I'm open to making this change, but it would mean changing a lot of code and likely retiring types.Schema and schema.Schema.

I noticed that the variable name (doc) did not get renamed along with the type. That's an oversight and I'll do a pass to check for this type of error later on today.

[pavelnikolov]

I noticed that the variable name (doc) did not get renamed along with the type. That's an oversight and I'll do a pass to check for this type of error later on today.

That would be a good idea, to avoid confusion in future.

[pavelnikolov]

Just one question re:

Rather than add more things to Document and refactor everything to work based on that root type
Why do we need to add everything and why is refactoring required? Do you mean adding ExecutableDefinition under document?

[rudle]

Why do we need to add everything and why is refactoring required? Do you mean adding ExecutableDefinition under document?

I think we have some crossed wires, so I'm going to try and re-establish my line of thinking.

It's my belief that a type called types.Document should match the specification for a Document as closely as is reasonable. The type called Document in the query package did not match the specification, as it was missing 2/3 of the fields. http://spec.graphql.org/June2018/

The type now called types.ExecutableDefinition matches the relevant part of the specification. It's clear to everyone what it does and which part of the specification it implements.

An orthogonal option available to us in this PR (or a followup) is to introduce this type:

// http://spec.graphql.org/June2018/#sec-Language.Document
type Document struct {
  ExecutableDefinitions []ExecutableDefinition
  TypeSystemDefinitions []TypeSystemDefinition
  TypeSystemExtension []TypeSystemExtension
}

and

// http://spec.graphql.org/June2018/#TypeSystemDefinition
type TypeSystemDefinition struct {
  SchemaDefinition SchemaDefinition
  TypeDefinition       []TypeDefinition
  DirectiveDefinition []DirectiveDefinition
}

and so on.

The main work of this approach would be to replace references to types.Schema with types.Document or types.TypeSystemDefinition. Given how much code depends on types.Schema and its fields, I don't find this option very appealing but it is the most correct way to proceed. I'm interested to hear your thoughts on this.

[rudle]

@pavelnikolov any thoughts here? I feel pretty strongly that we shouldn't have a type called Document in the public API that doesn't match the specification.

[pavelnikolov]

I agree with you. Your reason to use types.ExecutableDefinition sounds pretty compelling. Let's proceed with executable definition and come back to Document in a followup PR.

[rudle]

Just out of curiosity - what is the internal tool that you are using with this code?

Good question!

We wrote a linter to catch common errors made by contributors to our shared codebase. It checks for things like:

• do mutations and their inputs have similar names?
• does the type and its fields repeat words (eg avoid: FriendRequests { friendRequestID })
• is pagination implemented correctly
  and so on.

[rudle]

I found another issue and pushed a fix. ab449f0 describes the issue

[rudle]

Hi @pavelnikolov, there are still a couple of outstanding discussions on this PR that I'd like to get resolved soon. Thanks!

On our side, we have successfully integrated our GraphQL schema linter with this branch of the repo which, in my opinion, supports this as a good solution for allowing tooling to be built on top of the parser.

[pavelnikolov]

@rudle which are the outstanding discussions? I'd be happy to help. Feel free to ask questions proactively and ping me if I don't respond promptly.

[rudle]

Hi @pavelnikolov there are two discussions that I think are blocking this PR from being merged to master.

• add types package #437 (comment) this discussion about the Selection interface
• add types package #437 (comment) this discussion about types.Document vs types.ExecutableDefinition

If you'd prefer to discuss this sort of thing in high-bandwidth & real-time, I'd be happy to schedule a video chat at a time that works for you.

[pavelnikolov]

@rudle thank you for your patience. I'm looking forward to a quick chat about the Selection interface. You can find me in he Kubernetes or Gophers Slack with the same username.