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.

Sign up for GitHub

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

Store parsed Schema on ExecutableSchema rather than use global variable #2811

Merged
merged 7 commits into from
Oct 1, 2023
Merged

Store parsed Schema on ExecutableSchema rather than use global variable #2811

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ffd3e72
codegen: executableSchema schema configurable
gitxiongpan Sep 28, 2023
c270df5
feat
gitxiongpan Sep 28, 2023
7bc43de
Merge branch '99designs:master' into codegen/configurable-executableS…
gitxiongpan Sep 28, 2023
cf54032
feat
gitxiongpan Sep 30, 2023
c6ae3f2
codegen: add schema property on Config and executableSchema
gitxiongpan Oct 1, 2023
fccf443
fix: fmt
gitxiongpan Oct 1, 2023
c9f2b6b
regenerate
StevenACoffman Oct 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions codegen/generated!.gotpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@
// NewExecutableSchema creates an ExecutableSchema from the ResolverRoot interface.
func NewExecutableSchema(cfg Config) graphql.ExecutableSchema {
return &executableSchema{
schema: cfg.Schema,
resolvers: cfg.Resolvers,
directives: cfg.Directives,
complexity: cfg.Complexity,
}
}

type Config struct {
Schema *ast.Schema
Resolvers ResolverRoot
Directives DirectiveRoot
Complexity ComplexityRoot
Expand Down Expand Up @@ -93,12 +95,16 @@

{{ if eq .Config.Exec.Layout "single-file" }}
type executableSchema struct {
schema *ast.Schema
resolvers ResolverRoot
directives DirectiveRoot
complexity ComplexityRoot
}

func (e *executableSchema) Schema() *ast.Schema {
if e.schema != nil {
return e.schema
}
return parsedSchema
}

Expand Down Expand Up @@ -268,14 +274,14 @@
if ec.DisableIntrospection {
return nil, errors.New("introspection disabled")
}
return introspection.WrapSchema(parsedSchema), nil
return introspection.WrapSchema(ec.Schema()), nil
}

func (ec *executionContext) introspectType(name string) (*introspection.Type, error) {
if ec.DisableIntrospection {
return nil, errors.New("introspection disabled")
}
return introspection.WrapTypeFromDef(parsedSchema, parsedSchema.Types[name]), nil
return introspection.WrapTypeFromDef(ec.Schema(), ec.Schema().Types[name]), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reckon line 277 and 284 have better coding style no matter we decide to make executableSchema configurable or not

Copy link
Collaborator

@StevenACoffman StevenACoffman Oct 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Avoiding global variables is good, but ec.Schema().Types[name] violates the Law of Demeter.

If any of ec, ec.Schema(), or ec.Schema().Types are nil, this will have a fatal error (panic due to nil pointer dereference) at this line, and from alone it would be difficult to figure out which of these was nil.

A temporary local variable might be more verbose, but in the event of a nil pointer dereference, the line number would then tell you immediately which of those three cases was problematic.

}

{{if .HasEmbeddableSources }}
Expand Down