Add unique field names validation #1135
Conversation
Fixes absinthe-graphql#1133 Also fixes absinthe-graphql#1049 as the schema's won't compile anymore, so the warning no longer happens
| ] do | ||
| fields = | ||
| for field <- object.fields do | ||
| if duplicate?(object.fields, field, key) do |
There was a problem hiding this comment.
I think the first arg here should be a Map of the object.field names, otherwise this becomes O(n^2). We can do:
name_counts = Enum. frequencies_by(object.fields, &Map.get(&1, key))
def duplicate?(name_counts, field, key) do
field_identifier = Map.get(field, key)
Map.get(name_counts, field_identifier, 0) > 1
end
There was a problem hiding this comment.
Done :)
I copied the duplicate approach from other parts of the code. E.g. Absinthe.Phase.Document.Validation.UniqueVariableNames. These could probably also be optimized, esp since they are run in the document pipeline and not in the schema pipeline, so the cost is incurred for every request.
There was a problem hiding this comment.
Ah that sounds like a great followup!
There was a problem hiding this comment.
For now this PR can be merged
|
Oh, one last thing, can you add a CHANGELOG.md entry. I'm gonna start requiring that cause it's super hard to track down changes people have made in the future. |
|
Thank you! |
Fixes #1133
Also fixes #1049
as the schema's won't compile anymore, so the warning no longer happens