typelevel / grackle

Grackle: Functional GraphQL for the Typelevel stack
http://typelevel.org/grackle/
Apache License 2.0
176 stars 25 forks source link

support for scalar argument without apostrophes #529

Closed michludw closed 10 months ago

michludw commented 10 months ago

GraphQL queries with custom scalar arguments (e.g. moviesLongerThan(duration: PT3H)) are allowed by GraphQL and used to be supported in some previous grackle versions (before Elaborator rework). This PR brings it back.

There is no need to support such inputs for variables since untypedVars are passed as Json and we can not pass the value without apostrophes.

I've decided to treat a custom scalar argument passed in enumeration-style (no apostrophes) as StringValue, so the GraphQL server always supports the input with and without apostrophes out of the box (it's enough to define support StringValue value handler). Another option would be to keep the Enum value - this would allow to decide whether the enum-type value should be supported, but require the user to handle both StringValue and EnumValue values is so.

codecov-commenter commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e0b85de) 72.61% compared to head (16175f3) 72.64%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #529 +/- ## ========================================== + Coverage 72.61% 72.64% +0.03% ========================================== Files 32 32 Lines 4352 4354 +2 Branches 939 949 +10 ========================================== + Hits 3160 3163 +3 + Misses 1192 1191 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

milessabin commented 10 months ago

The spec doesn't actually have explicit wording that justifies this, however I've just checked with a GraphQL working group member, and it seems both that this is intended, even if not explicit, and also widely supported in many implementations.

This wouldn't be an enum, it would be using the literal form of the ID scalar type. Let me review the change I made that excluded it, and your proposed fix ... I'll try and get this, or something similar merged today.

milessabin commented 10 months ago

This wouldn't be an enum, it would be using the literal form of the ID scalar type

Update: no, this is wrong, an ID literal would still need quotes. So you're right that grackle was previously parsing the unquoted value as an enum value. I'll check again that this is acceptable.

milessabin commented 10 months ago

This is the support we have,

I don't see anything that explicitly forbids it. My intuition was that if not mentioned in coercion it would be in the validation section, but I don't see anything there forbidding it. I've not noticed this being used in the wild, but it does seem like it's not forbidden.

I would say that this is a grey area in the spec and basically "undefined and implementation dependent".

I think the likelihood is that if the spec were tightened up it would probably explicitly support the unquoted form, so I'll merge this PR now.

However I would encourage you to move away from the unquoted form in case the spec is revised in a way which excludes it, in which case Grackle would have to follow suit.