Skip to content

Commit

Permalink
Fix Flaky Order of Directive Validation
Browse files Browse the repository at this point in the history
Due to the use of a map behind the directiveNames, iterating over them is done
in random order. When there are multiple directives that are duplicated, the
order of errors can change between runs, and be inconsistent with the
expecated result.

This issue is also the case for other types which use a map to validate names,
however the other cases currently have no tests validating this condition. To
resolve the immediate issue of flaky tests, addressing just the directives case
for now. Resolution of other cases should be accompanied by tests exposing the
issue.
  • Loading branch information
dackroyd committed Apr 3, 2024
1 parent a3d0ced commit 731ec86
Showing 1 changed file with 12 additions and 6 deletions.
18 changes: 12 additions & 6 deletions internal/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,15 @@ func validateDirectives(c *opContext, loc string, directives ast.DirectiveList)
)
}

for n := range directiveNames {
// Iterating in the declared order, rather than using the directiveNames ordering which is random
for _, d := range directives {
n := d.Name.Name

ds := directiveNames[n]
if len(ds) <= 1 {
continue
}

dd, ok := c.schema.Directives[n]
if !ok {
// Invalid directive will have been flagged already
Expand All @@ -709,11 +717,6 @@ func validateDirectives(c *opContext, loc string, directives ast.DirectiveList)
continue
}

ds := directiveNames[n]
if len(ds) <= 1 {
continue
}

for _, loc := range ds[1:] {
// Duplicate directive errors are inconsistent with the behaviour for other types in graphql-js
// Instead of reporting a single error with all locations, errors are reported for each duplicate after the first declaration
Expand All @@ -722,6 +725,9 @@ func validateDirectives(c *opContext, loc string, directives ast.DirectiveList)
return fmt.Sprintf("The directive %q can only be used once at this location.", "@"+n)
})
}

// drop the name from the set to prevent the same errors being re-added for duplicates
delete(directiveNames, n)
}
}

Expand Down

0 comments on commit 731ec86

Please sign in to comment.