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: Raise an Execution Exception before trying to decode a Query Response #150

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

maticzav
Copy link
Owner

@maticzav maticzav commented Jul 12, 2023

This PR fixes a bug where the full SwiftGraphQLClient would try to decode a faulty response and ignore errors that were raised during execution.

Now, all methods first check that the result contains no errors and only then try to decode the result.

cc #148

Important ⚠️ ⚠️ ⚠️

This is not ideal and it shows that I made some assumptions that don't hold anymore. Namely, I assumed that the client can always enforce that the server conforms to the schema and that we can safely assume that if a field is non-optional that the data will be there.

Looking back on it, this is clearly wrong as we can easily see from numerous bug reports and all the fidgeting that's in the code. As per spec,

If an error was raised before execution begins, the data entry should not be present in the result.

If an error was raised during the execution that prevented a valid response, the data entry in the response should be null.

My reasoning back then was that since we generate the query, it couldn't happen that the query wouldn't conform to the schema, hence the query would always get executed. In the recent years, however, it seems like more and more of the authentication and authorization logic happens before execution, meaning that even if we generate the query, there's still a chance that we get back null or empty values.

Plan going forward

This is an apparent issue and I am looking forward to fixing it, but I don't know when that'll be.

Workarond

The current workaround is that we defensively throw an error - given the changes from this PR. This means that the publisher may throw something (which in my ideal world wasn't the case before), and there's no way to differentiate between results that included some data and results that did not.

*You don't need to do anything, just be aware that there's a chance that the publishers for query, mutation and subscription may throw.

Next Version

Next version of SwiftGraphQL is going to rethink how we handle nil values. Perhaps we could make another GraphQLClient that has more lose requirements, and keep the one that we have now for the other times where you "just want something working".

I am looking forward to suggestions and ideas 🙂

@vercel
Copy link

vercel bot commented Jul 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
swift-graphql ✅ Ready (Inspect) Visit Preview Jul 12, 2023 1:34pm

@maticzav maticzav self-assigned this Jul 12, 2023
@maticzav maticzav changed the title Raise an Execution Exception before trying to decode a Query Response fix: Raise an Execution Exception before trying to decode a Query Response Jul 12, 2023
@maticzav maticzav merged commit 3f0687b into main Jul 12, 2023
5 checks passed
@maticzav maticzav deleted the fix/unexpected-object-type branch July 12, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant