From e45f26dd458a2171a12ed77ed867529ff51eb01d Mon Sep 17 00:00:00 2001 From: Richard Musiol Date: Wed, 22 Mar 2017 15:23:08 +0100 Subject: [PATCH] validation: UniqueDirectivesPerLocation --- internal/common/directive.go | 17 +++- internal/exec/exec.go | 6 +- internal/query/query.go | 10 +- internal/schema/schema.go | 15 +-- internal/tests/testdata/export.js | 2 +- internal/tests/testdata/tests.json | 149 +++++++++++++++++++++++++++++ internal/validation/validation.go | 36 ++++--- introspection/introspection.go | 18 ++-- 8 files changed, 212 insertions(+), 41 deletions(-) diff --git a/internal/common/directive.go b/internal/common/directive.go index 5139154eff7..44f01f2c4df 100644 --- a/internal/common/directive.go +++ b/internal/common/directive.go @@ -9,8 +9,8 @@ type Directive struct { Args ArgumentList } -func ParseDirectives(l *lexer.Lexer) map[string]*Directive { - directives := make(map[string]*Directive) +func ParseDirectives(l *lexer.Lexer) DirectiveList { + var directives DirectiveList for l.Peek() == '@' { l.ConsumeToken('@') d := &Directive{} @@ -19,7 +19,18 @@ func ParseDirectives(l *lexer.Lexer) map[string]*Directive { if l.Peek() == '(' { d.Args = ParseArguments(l) } - directives[d.Name.Name] = d + directives = append(directives, d) } return directives } + +type DirectiveList []*Directive + +func (l DirectiveList) Get(name string) *Directive { + for _, d := range l { + if d.Name.Name == name { + return d + } + } + return nil +} diff --git a/internal/exec/exec.go b/internal/exec/exec.go index 008bfa44545..b5db3bf46d1 100644 --- a/internal/exec/exec.go +++ b/internal/exec/exec.go @@ -670,8 +670,8 @@ type typeAssertExec struct { typeExec iExec } -func skipByDirective(r *request, directives map[string]*common.Directive) bool { - if d, ok := directives["skip"]; ok { +func skipByDirective(r *request, directives common.DirectiveList) bool { + if d := directives.Get("skip"); d != nil { p := valuePacker{valueType: reflect.TypeOf(false)} v, err := p.pack(r, r.resolveVar(d.Args.MustGet("if").Value)) if err != nil { @@ -682,7 +682,7 @@ func skipByDirective(r *request, directives map[string]*common.Directive) bool { } } - if d, ok := directives["include"]; ok { + if d := directives.Get("include"); d != nil { p := valuePacker{valueType: reflect.TypeOf(false)} v, err := p.pack(r, r.resolveVar(d.Args.MustGet("if").Value)) if err != nil { diff --git a/internal/query/query.go b/internal/query/query.go index bdf7aa116e2..8ce0f1ef2d5 100644 --- a/internal/query/query.go +++ b/internal/query/query.go @@ -42,7 +42,7 @@ type Operation struct { Name lexer.Ident Vars common.InputValueList SelSet *SelectionSet - Directives map[string]*common.Directive + Directives common.DirectiveList } type OperationType string @@ -61,7 +61,7 @@ type Fragment struct { type FragmentDecl struct { Fragment Name lexer.Ident - Directives map[string]*common.Directive + Directives common.DirectiveList } type SelectionSet struct { @@ -77,18 +77,18 @@ type Field struct { Alias lexer.Ident Name lexer.Ident Arguments common.ArgumentList - Directives map[string]*common.Directive + Directives common.DirectiveList SelSet *SelectionSet } type InlineFragment struct { Fragment - Directives map[string]*common.Directive + Directives common.DirectiveList } type FragmentSpread struct { Name lexer.Ident - Directives map[string]*common.Directive + Directives common.DirectiveList } func (Field) isSelection() {} diff --git a/internal/schema/schema.go b/internal/schema/schema.go index e266d7fd1f0..3aedfd0ca72 100644 --- a/internal/schema/schema.go +++ b/internal/schema/schema.go @@ -68,7 +68,7 @@ type Enum struct { type EnumValue struct { Name string - Directives map[string]*common.Directive + Directives common.DirectiveList Desc string } @@ -136,7 +136,7 @@ type Field struct { Name string Args common.InputValueList Type common.Type - Directives map[string]*common.Directive + Directives common.DirectiveList Desc string } @@ -271,15 +271,16 @@ func resolveField(s *Schema, f *Field) error { return resolveInputObject(s, f.Args) } -func resolveDirectives(s *Schema, directives map[string]*common.Directive) error { - for name, d := range directives { - dd, ok := s.Directives[name] +func resolveDirectives(s *Schema, directives common.DirectiveList) error { + for _, d := range directives { + dirName := d.Name.Name + dd, ok := s.Directives[dirName] if !ok { - return errors.Errorf("directive %q not found", name) + return errors.Errorf("directive %q not found", dirName) } for _, arg := range d.Args { if dd.Args.Get(arg.Name.Name) == nil { - return errors.Errorf("invalid argument %q for directive %q", arg.Name.Name, name) + return errors.Errorf("invalid argument %q for directive %q", arg.Name.Name, dirName) } } for _, arg := range dd.Args { diff --git a/internal/tests/testdata/export.js b/internal/tests/testdata/export.js index c1873b5e4f4..a0347a57b13 100644 --- a/internal/tests/testdata/export.js +++ b/internal/tests/testdata/export.js @@ -72,7 +72,7 @@ require('./src/validation/__tests__/LoneAnonymousOperation-test'); require('./src/validation/__tests__/ProvidedNonNullArguments-test'); require('./src/validation/__tests__/ScalarLeafs-test'); require('./src/validation/__tests__/UniqueArgumentNames-test'); -// require('./src/validation/__tests__/UniqueDirectivesPerLocation-test'); +require('./src/validation/__tests__/UniqueDirectivesPerLocation-test'); require('./src/validation/__tests__/UniqueFragmentNames-test'); // require('./src/validation/__tests__/UniqueInputFieldNames-test'); require('./src/validation/__tests__/UniqueOperationNames-test'); diff --git a/internal/tests/testdata/tests.json b/internal/tests/testdata/tests.json index f4466fe4799..193bf90aad2 100644 --- a/internal/tests/testdata/tests.json +++ b/internal/tests/testdata/tests.json @@ -2081,6 +2081,155 @@ } ] }, + { + "name": "Validate: Directives Are Unique Per Location/no directives", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type {\n field\n }\n ", + "errors": [] + }, + { + "name": "Validate: Directives Are Unique Per Location/unique directives in different locations", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type @directiveA {\n field @directiveB\n }\n ", + "errors": [] + }, + { + "name": "Validate: Directives Are Unique Per Location/unique directives in same locations", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type @directiveA @directiveB {\n field @directiveA @directiveB\n }\n ", + "errors": [] + }, + { + "name": "Validate: Directives Are Unique Per Location/same directives in different locations", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type @directiveA {\n field @directiveA\n }\n ", + "errors": [] + }, + { + "name": "Validate: Directives Are Unique Per Location/same directives in similar locations", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type {\n field @directive\n field @directive\n }\n ", + "errors": [] + }, + { + "name": "Validate: Directives Are Unique Per Location/duplicate directives in one location", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type {\n field @directive @directive\n }\n ", + "errors": [ + { + "message": "The directive \"directive\" can only be used once at this location.", + "locations": [ + { + "line": 3, + "column": 15 + }, + { + "line": 3, + "column": 26 + } + ] + } + ] + }, + { + "name": "Validate: Directives Are Unique Per Location/many duplicate directives in one location", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type {\n field @directive @directive @directive\n }\n ", + "errors": [ + { + "message": "The directive \"directive\" can only be used once at this location.", + "locations": [ + { + "line": 3, + "column": 15 + }, + { + "line": 3, + "column": 26 + } + ] + }, + { + "message": "The directive \"directive\" can only be used once at this location.", + "locations": [ + { + "line": 3, + "column": 15 + }, + { + "line": 3, + "column": 37 + } + ] + } + ] + }, + { + "name": "Validate: Directives Are Unique Per Location/different duplicate directives in one location", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type {\n field @directiveA @directiveB @directiveA @directiveB\n }\n ", + "errors": [ + { + "message": "The directive \"directiveA\" can only be used once at this location.", + "locations": [ + { + "line": 3, + "column": 15 + }, + { + "line": 3, + "column": 39 + } + ] + }, + { + "message": "The directive \"directiveB\" can only be used once at this location.", + "locations": [ + { + "line": 3, + "column": 27 + }, + { + "line": 3, + "column": 51 + } + ] + } + ] + }, + { + "name": "Validate: Directives Are Unique Per Location/duplicate directives in many locations", + "rule": "UniqueDirectivesPerLocation", + "query": "\n fragment Test on Type @directive @directive {\n field @directive @directive\n }\n ", + "errors": [ + { + "message": "The directive \"directive\" can only be used once at this location.", + "locations": [ + { + "line": 2, + "column": 29 + }, + { + "line": 2, + "column": 40 + } + ] + }, + { + "message": "The directive \"directive\" can only be used once at this location.", + "locations": [ + { + "line": 3, + "column": 15 + }, + { + "line": 3, + "column": 26 + } + ] + } + ] + }, { "name": "Validate: Unique fragment names/no fragments", "rule": "UniqueFragmentNames", diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 55ac421d251..cfa68293a50 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -86,7 +86,7 @@ func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { c.validateDirectives("FRAGMENT_DEFINITION", frag.Directives) t := c.resolveType(&frag.On) // continue even if t is nil - if !canBeFragment(t) { + if t != nil && !canBeFragment(t) { c.addErr(frag.On.Loc, "FragmentsOnCompositeTypes", "Fragment %q cannot condition on non composite type %q.", frag.Name.Name, t) continue } @@ -202,16 +202,22 @@ func (c *context) resolveType(t common.Type) common.Type { return t2 } -func (c *context) validateDirectives(loc string, directives map[string]*common.Directive) { - for name, d := range directives { - names := make(nameSet) +func (c *context) validateDirectives(loc string, directives common.DirectiveList) { + directiveNames := make(nameSet) + for _, d := range directives { + dirName := d.Name.Name + c.validateNameCustomMsg(directiveNames, d.Name, "UniqueDirectivesPerLocation", func() string { + return fmt.Sprintf("The directive %q can only be used once at this location.", dirName) + }) + + argNames := make(nameSet) for _, arg := range d.Args { - c.validateName(names, arg.Name, "UniqueArgumentNames", "argument") + c.validateName(argNames, arg.Name, "UniqueArgumentNames", "argument") } - dd, ok := c.schema.Directives[name] + dd, ok := c.schema.Directives[dirName] if !ok { - c.addErr(d.Name.Loc, "KnownDirectives", "Unknown directive %q.", name) + c.addErr(d.Name.Loc, "KnownDirectives", "Unknown directive %q.", dirName) continue } @@ -223,12 +229,12 @@ func (c *context) validateDirectives(loc string, directives map[string]*common.D } } if !locOK { - c.addErr(d.Name.Loc, "KnownDirectives", "Directive %q may not be used on %s.", name, loc) + c.addErr(d.Name.Loc, "KnownDirectives", "Directive %q may not be used on %s.", dirName, loc) } c.validateArguments(d.Args, dd.Args, d.Name.Loc, - func() string { return fmt.Sprintf("directive %q", "@"+d.Name.Name) }, - func() string { return fmt.Sprintf("Directive %q", "@"+d.Name.Name) }, + func() string { return fmt.Sprintf("directive %q", "@"+dirName) }, + func() string { return fmt.Sprintf("Directive %q", "@"+dirName) }, ) } return @@ -236,10 +242,16 @@ func (c *context) validateDirectives(loc string, directives map[string]*common.D type nameSet map[string]errors.Location -func (c *context) validateName(set nameSet, name lexer.Ident, rule, kind string) { +func (c *context) validateName(set nameSet, name lexer.Ident, rule string, kind string) { + c.validateNameCustomMsg(set, name, rule, func() string { + return fmt.Sprintf("There can be only one %s named %q.", kind, name.Name) + }) +} + +func (c *context) validateNameCustomMsg(set nameSet, name lexer.Ident, rule string, msg func() string) { if loc, ok := set[name.Name]; ok { c.errs = append(c.errs, &errors.QueryError{ - Message: fmt.Sprintf("There can be only one %s named %q.", kind, name.Name), + Message: msg(), Locations: []errors.Location{loc, name.Loc}, Rule: rule, }) diff --git a/introspection/introspection.go b/introspection/introspection.go index dc0d2b0c5d8..206954e4c24 100644 --- a/introspection/introspection.go +++ b/introspection/introspection.go @@ -115,7 +115,7 @@ func (r *Type) Fields(args *struct{ IncludeDeprecated bool }) *[]*Field { var l []*Field for _, f := range fields { - if _, ok := f.Directives["deprecated"]; !ok || args.IncludeDeprecated { + if d := f.Directives.Get("deprecated"); d == nil || args.IncludeDeprecated { l = append(l, &Field{f}) } } @@ -161,7 +161,7 @@ func (r *Type) EnumValues(args *struct{ IncludeDeprecated bool }) *[]*EnumValue var l []*EnumValue for _, v := range t.Values { - if _, ok := v.Directives["deprecated"]; !ok || args.IncludeDeprecated { + if d := v.Directives.Get("deprecated"); d == nil || args.IncludeDeprecated { l = append(l, &EnumValue{v}) } } @@ -220,13 +220,12 @@ func (r *Field) Type() *Type { } func (r *Field) IsDeprecated() bool { - _, ok := r.field.Directives["deprecated"] - return ok + return r.field.Directives.Get("deprecated") != nil } func (r *Field) DeprecationReason() *string { - d, ok := r.field.Directives["deprecated"] - if !ok { + d := r.field.Directives.Get("deprecated") + if d == nil { return nil } reason := common.UnmarshalLiteral(d.Args.MustGet("reason").Value.(*lexer.Literal)).(string) @@ -280,13 +279,12 @@ func (r *EnumValue) Description() *string { } func (r *EnumValue) IsDeprecated() bool { - _, ok := r.value.Directives["deprecated"] - return ok + return r.value.Directives.Get("deprecated") != nil } func (r *EnumValue) DeprecationReason() *string { - d, ok := r.value.Directives["deprecated"] - if !ok { + d := r.value.Directives.Get("deprecated") + if d == nil { return nil } reason := common.UnmarshalLiteral(d.Args.MustGet("reason").Value.(*lexer.Literal)).(string)