Test: Missing subscription field should not be an "internal error"

[leebyron]

Test: Missing subscription field should not be an "internal error"

A remaining issue with subscriptions is attempting to subscribe to a missing field. The existing implementation creates an internal error which is an improper blame since there is no possibility to fix it internally - it is an issue with the query.

Since there is no clear consistent behavior relative to query/mutation operations (which simply skip over unknown fields), and the corner-case where this occurs should deal with the error - this changes the internal error to an external yielded GraphQL error.

Note: we still require a spec change since there is an ambiguity of this exact scenario.

[robzhu]

When trying to execute a subscription with an invalid selection set, I think we should throw when attempting to create the subscription.

EDIT: I used "throw" too loosely. I meant in accordance with how we want to handle 400-class errors elsewhere. My impressions is that returning an emptyAsyncIterator feels too subtle for an error case.

[robzhu]

I think asking for an unknown field is a 400-class error code (something about the request was malformed). In that case, I think the await createSubscription call should throw.

My reasonings is that if we don't throw there, we'll have a subscription that is in the closed state, which does not communicate that the request was invalid or what was wrong with it.

[robzhu]

This doesn't feel like the right return value for a missing field case.

[leebyron]

What would be a better one? Presuming an error becomes the domain of a validation step, we should produce something

[robzhu]

In https://github.com/facebook/graphql/blob/master/spec/Section%206%20--%20Execution.md#executing-requests we describe Subscribe as the async variant of ExecuteQuery. For ExecuteQuery, the spec says, "Return an unordered map containing {data} and {errors}."while Subscribe says Return {responseStream}.. Perhaps we should update these to be more similar, for example, 'Subscribe` could resolve to an unordered map of {responseStream} and {errors}.

[leebyron]

Also remember that "subscribe" is the second half of a presumed "validate & subscribe" path. I think the missing piece is a validation rule that a subscription is well-formed.

There is symmetry if my proposed behavior to execute which skips over unknown fields such that { unknownField } yields {}.

My suggestion here is that the "subscribe" operation is the wrong place for this error, it should instead be caught earlier during validation, and instead this operation should be durable to the malformed input

[leebyron]

http://facebook.github.io/graphql/draft/#sec-Subscription-Operation-Definitions seems to handle this case, validation errors are typically reported as 400-level.

Perhaps we should update these to be more similar, for example, 'Subscribe` could resolve to an unordered map of {responseStream} and {errors}.

I think the flow types we've defined more closely refer to a responseStream or a single error response payload

[leebyron]

Updated description and commit to reflect "400" level error rather than current "500" level error after offline review with @robzhu