Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of empty JSON object response and no field selection #248

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

klarstrup
Copy link

@klarstrup klarstrup commented Apr 24, 2020

Background: I have to work against an API where certain DELETE requests will respond happily with that empty {} object, and RestLink asplodes.

This PR makes it so that a {} 200 API response succesfully becomes a null when no fields have been requested, as there is no such thing as an empty object in GraphQL.

Not sure whether this would be considered a bugfix or a new feature, I couldn't find any prior discussion about behavior for this scenario.

@apollo-cla
Copy link

@klarstrup: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@abrahamcuenca
Copy link

@klarstrup looks like there was a failing test at src/__tests__/restLink.ts Is this related to the changes or was this already failing before?

@fbartho
Copy link
Collaborator

fbartho commented Jan 5, 2022

@klarstrup various GraphQL schema parsers throw linting or parsing errors when you don't select fields for every type of Query.

(We had a workaround/convention of typing such responses IrrelevantResponse for that situation, and you can always query for __typename)

I'm not sure I want to merge this if it's supporting an "invalid" variant of the GraphQL language. What are your thoughts? /cc @abrahamcuenca

@klarstrup
Copy link
Author

klarstrup commented Jan 5, 2022

I'm not sure I follow @fbartho, this doesn't concern GraphQL parsing at all but rather an occasional scenario where some JSON APIs will respond with an empty object in some cases. This can't exactly be modeled in GraphQL so I reasoned for falling back to null as that corresponds semantically to the "empty response" that {} conveys.

Edit: Ah I see now what you mean, it has been a while. But top level fields with no selection set(primitive values) are valid to my recollection.

Edit 2: Yeah it's even an example in the docs https://spec.graphql.org/June2018/#example-e2969

@abrahamcuenca
Copy link

@klarstrup various GraphQL schema parsers throw linting or parsing errors when you don't select fields for every type of Query.

(We had a workaround/convention of typing such responses IrrelevantResponse for that situation, and you can always query for __typename)

I'm not sure I want to merge this if it's supporting an "invalid" variant of the GraphQL language. What are your thoughts? /cc @abrahamcuenca

I agree @fbartho we should avoid any invalid variants as it could throw anyone trying to troubleshoot into a rabbit hole. We can hold off on merging until we get a better workaround.

@klarstrup
Copy link
Author

Still not following. Every field of Query(or indeed Mutation) doesn't necessarily have fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants