-
Notifications
You must be signed in to change notification settings - Fork 879
chore: update graphql dependencies #9436
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
Changes from all commits
57a8864
0d02a52
f47a3e2
3ed2e25
c58d816
a0cc144
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,17 +10,35 @@ import ( | |
| "sync" | ||
|
|
||
| "github.com/99designs/gqlgen/graphql" | ||
| "github.com/99designs/gqlgen/graphql/errcode" | ||
| "github.com/99designs/gqlgen/graphql/handler" | ||
| "github.com/iancoleman/strcase" | ||
| "github.com/opencontainers/go-digest" | ||
| "github.com/sourcegraph/conc/pool" | ||
| "github.com/vektah/gqlparser/v2/ast" | ||
| "github.com/vektah/gqlparser/v2/gqlerror" | ||
| "github.com/vektah/gqlparser/v2/parser" | ||
| "github.com/vektah/gqlparser/v2/validator" | ||
| "github.com/vektah/gqlparser/v2/validator/rules" | ||
| "golang.org/x/sync/errgroup" | ||
|
|
||
| "github.com/dagger/dagger/dagql/call" | ||
| ) | ||
|
|
||
| func init() { | ||
| // HACK: these rules are disabled because some clients don't send the right | ||
| // types: | ||
| // - PHP + Elixir SDKs send enums quoted | ||
| // - The shell sends enums quoted, and ints/floats as strings | ||
| // - etc | ||
| validator.RemoveRule(rules.ValuesOfCorrectTypeRule.Name) | ||
| validator.RemoveRule(rules.ValuesOfCorrectTypeRuleWithoutSuggestions.Name) | ||
|
|
||
| // HACK: this rule is disabled because PHP modules <=v0.15.2 query | ||
| // inputArgs incorrectly. | ||
| validator.RemoveRule(rules.ScalarLeafsRule.Name) | ||
| } | ||
|
|
||
| // Server represents a GraphQL server whose schema is dynamically modified at | ||
| // runtime. | ||
| type Server struct { | ||
|
|
@@ -98,6 +116,11 @@ func NewServer[T Typed](root T) *Server { | |
| return srv | ||
| } | ||
|
|
||
| func NewDefaultHandler(es graphql.ExecutableSchema) *handler.Server { | ||
| // TODO: avoid this deprecated method, and customize the options | ||
| return handler.NewDefaultServer(es) //nolint: staticcheck | ||
| } | ||
|
|
||
| var coreScalars = []ScalarType{ | ||
| Boolean(false), | ||
| Int(0), | ||
|
|
@@ -317,19 +340,27 @@ func (s *Server) Schema() *ast.Schema { // TODO: change this to be updated whene | |
| defer s.installLock.Unlock() | ||
| // TODO track when the schema changes, cache until it changes again | ||
| queryType := s.Root().Type().Name() | ||
| schema := &ast.Schema{} | ||
| schema := &ast.Schema{ | ||
| Types: make(map[string]*ast.Definition), | ||
| PossibleTypes: make(map[string][]*ast.Definition), | ||
| } | ||
| for _, t := range s.objects { // TODO stable order | ||
| def := definition(ast.Object, t, s.View) | ||
| if def.Name == queryType { | ||
| schema.Query = def | ||
| } | ||
| schema.AddTypes(def) | ||
| schema.AddPossibleType(def.Name, def) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this right? I think "possible types" is meant to be a mapping from unions and interfaces to their concrete implementations, whereas this seems to imply every type has itself as a possible type
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't know actually 🤔 But this validation rule implies that it is meant to be set like this? https://github.com/vektah/gqlparser/blob/e21b122b4e0ebb42ad98fe4ecf738bf380e487a3/validator/rules/possible_fragment_spreads.go#L36-L36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it fail without it? The linked is regarding fragment spreads which should only be relevant to type switching on interfaces/unions, but it's for sure weird that it seems to pass the spreaded (concrete) type to // GetPossibleTypes will enumerate all the definitions for a given interface or union
func (s *Schema) GetPossibleTypes(def *Definition) []*Definition {
return s.PossibleTypes[def.Name]
}If it doesn't fail I would just skip it - we don't use interfaces anyway (and I don't think we use unions either). My main concern is that it might make the schema introspection a bit funky
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does fail, which was the reason I actually caught that linting wasn't ever enabled in the first place: I think potentially maybe this is just... wrong? Might submit a patch upstream.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this is why it works - as part of loading the schema, it adds all So we should do the same thing here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright - seems weird, but I'm convinced it's at least consistent 😛 |
||
| } | ||
| for _, t := range s.scalars { | ||
| schema.AddTypes(definition(ast.Scalar, t, s.View)) | ||
| def := definition(ast.Scalar, t, s.View) | ||
| schema.AddTypes(def) | ||
| schema.AddPossibleType(def.Name, def) | ||
| } | ||
| for _, t := range s.typeDefs { | ||
| schema.AddTypes(t.TypeDefinition(s.View)) | ||
| def := t.TypeDefinition(s.View) | ||
| schema.AddTypes(def) | ||
| schema.AddPossibleType(def.Name, def) | ||
| } | ||
| schema.Directives = map[string]*ast.DirectiveDefinition{} | ||
| for n, d := range s.directives { | ||
|
|
@@ -398,6 +429,14 @@ func (s *Server) ExecOp(ctx context.Context, gqlOp *graphql.OperationContext) (m | |
| if err != nil { | ||
| return nil, gqlErrs(err) | ||
| } | ||
|
|
||
| listErr := validator.Validate(s.Schema(), gqlOp.Doc) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yay! |
||
| if len(listErr) != 0 { | ||
| for _, e := range listErr { | ||
| errcode.Set(e, errcode.ValidationFailed) | ||
| } | ||
| return nil, listErr | ||
| } | ||
| } | ||
| results := make(map[string]any) | ||
| for _, op := range gqlOp.Doc.Operations { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be cleaned up someday? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! The first group can be as soon as the next release is out - they're actually fixed in this PR. But because of how we don't bundle those SDKs into the engine, we can't actually test those changes in this PR (so it would break). I think we need to fix this somehow, but kinda still thinking through ideas.
The second group is pending an @dagger/sdk-php fix: https://discord.com/channels/707636530424053791/1162025276872609832/1333814026731262012