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 support for repeatable directives #502

Merged
merged 14 commits into from
Mar 20, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
14 changes: 13 additions & 1 deletion internal/schema/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package schema

import (
"fmt"
"regexp"
"text/scanner"

"github.com/graph-gophers/graphql-go/errors"
Expand Down Expand Up @@ -511,10 +512,21 @@ func parseDirectiveDef(l *common.Lexer) *types.DirectiveDefinition {
l.ConsumeToken(')')
}

l.ConsumeKeyword("on")
switch x := l.ConsumeIdent(); x {
case "on":
// no-op; Go doesn't fallthrough by default
Copy link
Member

@pavelnikolov pavelnikolov Mar 20, 2022

Choose a reason for hiding this comment

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

Shouldn't you explicitly consume keyword on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on got consumed by the ConsumeIdent() on the previous line, didn't it? Looking at the implementations of ConsumeIdent / ConsumeKeyword, it looks like they both call TokenText() and then ConsumeWhitespace(), so I expect they will consume the same number of tokens; and if this code didn't consume on, then I would expect on to be interpreted as the location, which would make this check fail in this test.

case "repeatable":
d.Repeatable = true
l.ConsumeKeyword("on")
speezepearson marked this conversation as resolved.
Show resolved Hide resolved
default:
l.SyntaxError(fmt.Sprintf(`unexpected %q, expecting "on" or "repeatable"`, x))
}

for {
loc := l.ConsumeIdent()
if ok, err := regexp.MatchString("^[A-Z_]+$", loc); err != nil || !ok {
l.SyntaxError(fmt.Sprintf("expected directive location-spec to be SNAKE_CASE, but got %q", loc))
}
pavelnikolov marked this conversation as resolved.
Show resolved Hide resolved
d.Locations = append(d.Locations, loc)
if l.Peek() != '|' {
break
Expand Down
52 changes: 52 additions & 0 deletions internal/schema/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ Second line of the description.
| ENUM_VALUE
| INPUT_OBJECT
| INPUT_FIELD_DEFINITION
directive @repeatabledirective repeatable on SCALAR

interface NamedEntity @directive { name: String }

Expand All @@ -834,6 +835,8 @@ Second line of the description.
}

union Union @uniondirective = Photo | Person

scalar Mass @repeatabledirective @repeatabledirective
`,
validateSchema: func(s *types.Schema) error {
namedEntityDirectives := s.Types["NamedEntity"].(*types.InterfaceTypeDefinition).Directives
Expand Down Expand Up @@ -864,6 +867,55 @@ Second line of the description.
if len(unionDirectives) != 1 || unionDirectives[0].Name.Name != "uniondirective" {
return fmt.Errorf("missing directive on Union union, expected @uniondirective but got %v", unionDirectives)
}

massDirectives := s.Types["Mass"].(*types.ScalarTypeDefinition).Directives
if len(massDirectives) != 2 || massDirectives[0].Name.Name != "repeatabledirective" || massDirectives[1].Name.Name != "repeatabledirective" {
return fmt.Errorf("missing directive on Repeatable scalar, expected @repeatabledirective @repeatabledirective but got %v", massDirectives)
}
return nil
},
},
{
name: "Sets Directive.Repeatable iff `repeatable` keyword is given",
speezepearson marked this conversation as resolved.
Show resolved Hide resolved
sdl: `
directive @nonrepeatabledirective on SCALAR
directive @repeatabledirective repeatable on SCALAR
`,
validateSchema: func(s *types.Schema) error {
if dir := s.Directives["nonrepeatabledirective"]; dir.Repeatable {
return fmt.Errorf("did not expect directive to be repeatable: %v", dir)
}
if dir := s.Directives["repeatabledirective"]; !dir.Repeatable {
return fmt.Errorf("expected directive to be repeatable: %v", dir)
}
return nil
},
},
{
name: "Directive definition does not allow double-`repeatable`",
sdl: `
directive @mydirective repeatable repeatable SCALAR
scalar MyScalar @mydirective
`,
validateError: func(err error) error {
msg := `graphql: syntax error: unexpected "repeatable", expecting "on" (line 2, column 38)`
if err == nil || err.Error() != msg {
return fmt.Errorf("expected error %q, but got %q", msg, err)
}
return nil
},
},
{
name: "Directive definition does not allow double-`on` instead of `repeatable on`",
sdl: `
directive @mydirective on on SCALAR
scalar MyScalar @mydirective
`,
validateError: func(err error) error {
msg := `graphql: syntax error: expected directive location-spec to be SNAKE_CASE, but got "on" (line 2, column 33)`
if err == nil || err.Error() != msg {
return fmt.Errorf("expected error %q, but got %q", msg, err)
}
pavelnikolov marked this conversation as resolved.
Show resolved Hide resolved
return nil
},
},
Expand Down
11 changes: 6 additions & 5 deletions types/directive.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ type Directive struct {
//
// http://spec.graphql.org/draft/#sec-Type-System.Directives
type DirectiveDefinition struct {
Name string
Desc string
Locations []string
Arguments ArgumentsDefinition
Loc errors.Location
Name string
Desc string
Repeatable bool
Locations []string
Arguments ArgumentsDefinition
Loc errors.Location
}

type DirectiveList []*Directive
Expand Down