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

[Legacy] Remove nil values from variables field in requestBody #3400

Closed

Conversation

yramocan
Copy link

@yramocan yramocan commented Jun 30, 2024

Summary

Fixes an issue in legacy versions of the Apollo SDK. In my testing, this only occurred in my project when working with Xcode 16 Beta. This issue did not occur with previous versions of Xcode.

Apollo version: 0.53.0 (legacy)

Issue Description

I have a GQL query which takes in an optional value as part of the request body. During serialization, I'd hit a fatalError in the Apollo library which reads Dictionary is only JSONEncodable if Value is (and if Key is String) (permalink). This occurred because the variables dictionary contained a key-value pair where the key was present and the value was nil. In this version of Xcode, casting an Optional<Wrapped> value to Wrapped does not succeed.

I don't believe this is an issue in v1.0+ versions of the library, however, we cannot yet upgrade the Apollo version due to this issue.

Proposed Solution

In this PR, I've changed the way that values in the variables dictionary are included in the requestBody. Now, nil values in variables are removed before serialization. Additionally, if all values in variables are nil, variables won't be included in the requestBody.

@yramocan yramocan marked this pull request as ready for review June 30, 2024 04:12
@calvincestari
Copy link
Member

Hi @yramocan - I think there might be unintended side-effects to this change, and if this works for your case that might not be the case for all server implementations.

I'm not 100% familiar with the legacy version's implementation of GraphQL nullability but I believe nil would have been converted to null in the request sent to the server and there is a semantic difference in GraphQL between nil (not present) and null.

As you can see in the GraphQL spec example there are two ways to send an optional variable and your proposed change would lock everyone into the second way. Right now I don't think we can accept this change as it is, it's going to need more debugging to try understand what is causing the issue with Xcode 16.

@yramocan
Copy link
Author

Hi @yramocan - I think there might be unintended side-effects to this change, and if this works for your case that might not be the case for all server implementations.

I'm not 100% familiar with the legacy version's implementation of GraphQL nullability but I believe nil would have been converted to null in the request sent to the server and there is a semantic difference in GraphQL between nil (not present) and null.

As you can see in the GraphQL spec example there are two ways to send an optional variable and your proposed change would lock everyone into the second way. Right now I don't think we can accept this change as it is, it's going to need more debugging to try understand what is causing the issue with Xcode 16.

Sounds good. I'll close this PR and investigate further.

@yramocan yramocan closed this Jul 11, 2024
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.

2 participants