Add CoerceResult() #836
Merged
Add CoerceResult() #836
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leebyron
requested review from
benjie,
andimarek,
dschafer,
IvanGoncharov and
a team
leebyron
force-pushed
the
editorial-coerce-internal-value
branch
3 times, most recently
from
9d5ec04 to
abee282
Compare
leebyron
added
the
✏️ Editorial
|
I agree with that change, but I am not sure about the naming:
|
spec/Section 6 -- Execution.md
Outdated
| * Return the result of calling the internal method provided by the type | ||
| system for determining the "result coercion" of {leafType} given the value | ||
| {internalValue}. This internal method must return a valid value for the | ||
| type or otherwise throw a field error. |
There was a problem hiding this comment.
I would add that "result coercion" can't be null, otherwise, we are breaking the symmetry of coercing values and completing values. It can happen in the following scenario we have scalar with parseValue/serialize(graphql-js terminology) so serialize is return null, but if you pass null as the input value of the same scalar it will be interpreted by input coercing algorithm and will never hit parseValue.
This factors out a step from `CompleteValue()` about "coercing" a value to a formal sub-algorithm `CoerceInternalValue()` that mirrors the language used in `ResolveAbstractType()` - both refer to "internal methods" provided for each type, which is in fact how the reference implementation applies this coercion behavior. As editorial, this provides a place to expand on the behavior and purpose as well as link back to the relevant subsection from the type system section. This also makes a *very important* clarification. The previous step read that if coercion failed then "otherwise null". This actually described a much older version of the reference implementation which did not always produce errors for failed coercion. Years back the reference implementation and the spec changed the "result coercion" subsections of all the built-in scalars to clearly state that field errors are thrown if coercion fails. This step now creates ambiguitity about which behavior is correct - returning null or throwing a field error? In practice both may be correct because of the error handling behavior - but we should be clear that the returned null is a result of that error behavior rather than it being an explicit behavior that's part of `CompleteValue()`. The new pulled out sub-algo makes it clear that the return type must be valid or a field error is produced - which is exactly the behavior described by each built-in scalar.
leebyron
force-pushed
the
editorial-coerce-internal-value
branch
from
abee282 to
378fb5b
Compare
benjie
approved these changes
Apr 8, 2021
leebyron
added a commit
that referenced
this pull request
Apr 8, 2021
This factors out a step from `CompleteValue()` about "coercing" a value to a formal sub-algorithm `CoerceResult()` that mirrors the language used in `ResolveAbstractType()` - both refer to "internal methods" provided for each type, which is in fact how the reference implementation applies this coercion behavior. As editorial, this provides a place to expand on the behavior and purpose as well as link back to the relevant subsection from the type system section. This also makes a *very important* clarification. The previous step read that if coercion failed then "otherwise null". This actually described a much older version of the reference implementation which did not always produce errors for failed coercion. Years back the reference implementation and the spec changed the "result coercion" subsections of all the built-in scalars to clearly state that field errors are thrown if coercion fails. This step now creates ambiguitity about which behavior is correct - returning null or throwing a field error? In practice both may be correct because of the error handling behavior - but we should be clear that the returned null is a result of that error behavior rather than it being an explicit behavior that's part of `CompleteValue()`. The new pulled out sub-algo makes it clear that the return type must be valid or a field error is produced - which is exactly the behavior described by each built-in scalar.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This factors out a step from
CompleteValue()about "coercing" a value to a formal sub-algorithmCoerceInternalValue()that mirrors the language used inResolveAbstractType()- both refer to "internal methods" provided for each type, which is in fact how the reference implementation applies this coercion behavior.As editorial, this provides a place to expand on the behavior and purpose as well as link back to the relevant subsection from the type system section.
This also makes a very important clarification. The previous step read that if coercion failed then "otherwise null". This actually described a much older version of the reference implementation which did not always produce errors for failed coercion. Years back the reference implementation and the spec changed the "result coercion" subsections of all the built-in scalars to clearly state that field errors are thrown if coercion fails. This step now creates ambiguity about which behavior is correct - returning null or throwing a field error? In practice both may be correct because of the error handling behavior - but we should be clear that the returned null is a result of that error behavior rather than it being an explicit behavior that's part of
CompleteValue(). The new pulled out sub-algo makes it clear that the return type must be valid or a field error is produced - which is exactly the behavior described by each built-in scalar.Inspired by #780