search

apollographql / apollo-ios

📱  A strongly-typed, caching GraphQL client for iOS, written in Swift.
https://www.apollographql.com/docs/ios/
MIT License
3.89k stars 728 forks source link

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

Closed yramocan closed 4 months ago

yramocan commented 4 months ago

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.

calvincestari commented 4 months ago

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 commented 4 months ago

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.