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

add types package #437

Merged
merged 8 commits into from
Apr 23, 2021
Merged

Conversation

rudle
Copy link
Contributor

@rudle rudle commented Mar 16, 2021

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
Copy link
Contributor Author

rudle commented Mar 17, 2021

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
Copy link
Member

pavelnikolov commented Mar 18, 2021

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

//
// http://spec.graphql.org/draft/#Selection
type Selection interface {
isSelection()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rudle rudle Apr 16, 2021

Choose a reason for hiding this comment

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

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.

l := common.NewLexer(queryString, false)

var doc *Document
var doc *types.ExecutableDefinition
Copy link
Member

@pavelnikolov pavelnikolov Mar 18, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@pavelnikolov pavelnikolov Mar 18, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@rudle rudle Mar 29, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

rudle commented Mar 18, 2021

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.

types/field.go Outdated Show resolved Hide resolved
@rudle
Copy link
Contributor Author

rudle commented Mar 30, 2021

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

@rudle
Copy link
Contributor Author

rudle commented Apr 6, 2021

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
Copy link
Member

@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
Copy link
Contributor Author

rudle commented Apr 8, 2021

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

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
Copy link
Member

@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.

Part of graph-gophers#434 and related to graph-gophers#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.
This additive function shouldn't break backward compatibility will allow those
who want access to the types to get at an AST version of the `types.Schema`
This was an error. When this field was renamed from schema.Field (to avoid
ambiguity) its name field changed to match query.Field (to Ident). This caused a
cascade of useless changes that will be rolled back in the next commit
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.

4 participants