Improve type checks on variables #1141
Conversation
Necessary to get the name of the type when the underlying type may not exist in the schema and therefore `Absinthe.Type.name/1` won't work.
spec: https://spec.graphql.org/October2021/#sec-All-Variable-Usages-are-Allowed fixes absinthe-graphql#1134 Several cases did not return errors * when the location type was non nullable and the variable nullable * when the location type was a list and the variable wasn't Some cases are allowed: * contravariant, when the location type is nullable and the variable non-nullable * the the argument is non-nullable but either the variable or argument has a default value
With the changes on allowed variable usage they would only test whether the directive variable in the operation was present. This is covered elsewhere.
|
Great to see improved alignment to the spec, but I also wasn't expecting previously working queries to be rejected following a minor version bump, even after glancing through the changelog. I'm not sure how best to handle that sort of thing. Maybe a warning on the changelog entry? |
|
The changelog could definitely be improved to list these sort of spec compliance bug fixes. I've done a PR here #1143 but more improvements are welcome. |
|
@giddie I think that's a fair concern. We could retire 1.6.7 and do a 1.7.0 release. @maartenvanvliet thoughts? |
|
I think a 1.7 release makes sense. There may be other spec violation fixes that would break backwards compatibility but don't think there are any as impactful in day to day graphql usage as this one. Some others like #921 would warrant clear opt-in behaviour |
|
1.6.7 has been retired, and 1.7.0 has been released. |
|
If this is a potentially breaking change, why is this being pinned at 1.7.0 instead of 2.0.0? |
Currently the check was whether the unwrapped type of the variable and e.g. the argument were the same. The spec is stricter than this. In particular this section: https://spec.graphql.org/October2021/#sec-All-Variable-Usages-are-Allowed
This PR adds checks to see if the wrapped types are the same or the variable is stricter than the argument. Also, when default values are given some of these checks can be relaxed.
This is a bugfix in respect to the spec but will have the effect that some previously valid documents are now marked as invalid.
This fixes #1134