Retrieving optional field containing null in cache during ReadWriteTransaction causes crash

[dabby-wombo]

Summary

executing a LocalCacheMutation with an optional field containing nil and fetching said optional field during readWriteTransaction causes the the ReadWriteTransaction to crash with:

Could not cast value of type 'NSNull' (0x1b9055fa8) to 'NSString' (0x1b90518c0). (In this case the optional field is a String?)

This seems to be an issue that was introduced by #2784 in an effort to fix the previous crashing issue that arose for devices iOS 14.4 and lower.

I can confirm this is the case as reverting this piece of code back to what it was before the PR resolves the crash

  @inlinable public subscript<T: AnyScalarType & Hashable>(_ key: String) -> T {
    get { _data[key]?.base as! T } (reverting back to _data[key] as! T fixes things)
    set { _data[key] = newValue }
    _modify {
      var value = _data[key] as! T
      defer { _data[key] = value }
      yield &value
    }
  }

But seeing as that code was introduces to fix a crash, I dug a bit deeper into understanding what was happening, and it seems that specifically in the case of the ReadWriteTransaction.updateObject, we need to set the GraphQLSelectionSetMapper to stripNullValues.

accumulator: GraphQLSelectionSetMapper<SelectionSet>(
          stripNullValues: true,
          allowMissingValuesForOptionalFields: true
        )
      )

My reasoning here is that since the role of stripNullValues is to replace instances of NSNull with the scalar nil type, this is exactly what we want as the original issue for the crash seems to arise from the fact that Swift isn't able to rightly convert an NSNull to an NSString, but it can convert a nil scalar to an Optional<String>

Version

apollo-ios SDK version: 1.0.7

Steps to reproduce the behavior

I created a failing unit test in the ReadWriteFromStoreTests as follows:

func test_updateCacheMutationWithOptionalField_containiningNull_retrievingOptionalField_returns_nil() {
    // given
    struct GivenSelectionSet: MockMutableRootSelectionSet {
      public var __data: DataDict = DataDict([:], variables: nil)
      init(data: DataDict) { __data = data }
      
      static var __selections: [Selection] { [
        .field("hero", Hero.self)
      ]}
      
      var hero: Hero {
        get { __data["hero"] }
        set { __data["hero"] = newValue }
      }
      
      struct Hero: MockMutableRootSelectionSet {
        public var __data: DataDict = DataDict([:], variables: nil)
        init(data: DataDict) { __data = data }
        
        static var __selections: [Selection] { [
          .field("name", String.self),
          .field("nickname", String?.self)
        ]}
        
        var name: String {
          get { __data["name"] }
          set { __data["name"] = newValue }
        }
        
        var nickname: String? {
          get { __data["nickname"] }
          set { __data["nickname"] = newValue }
        }
      }
    }
    
    let cacheMutation = MockLocalCacheMutation<GivenSelectionSet>()
    
    mergeRecordsIntoCache([
      "QUERY_ROOT": ["hero": CacheReference("QUERY_ROOT.hero")],
      "QUERY_ROOT.hero": ["__typename": "Droid", "name": "R2-D2", "nickname": NSNull()]
    ])
    
    runActivity("update mutation") { _ in
      let updateCompletedExpectation = expectation(description: "Update completed")
      
      store.withinReadWriteTransaction({ transaction in
        try transaction.update(cacheMutation) { data in
          // doing a nil-coalescing to replace nil with <not-populated>
          data.hero.nickname = data.hero.nickname ?? "<not-populated>"
        }
      }, completion: { result in
        defer { updateCompletedExpectation.fulfill() }
        XCTAssertSuccessResult(result)
      })
      
      self.wait(for: [updateCompletedExpectation], timeout: Self.defaultWaitTimeout)
    }
    
    let query = MockQuery<GivenSelectionSet>()
    
    loadFromStore(operation: query) { result in
      try XCTAssertSuccessResult(result) { graphQLResult in
        XCTAssertEqual(graphQLResult.source, .cache)
        XCTAssertNil(graphQLResult.errors)
        
        let data = try XCTUnwrap(graphQLResult.data)
        XCTAssertEqual(data.hero.nickname, "<not-populated>")
      }
    }
  }

Logs

No response

Anything else?

No response

[dabby-wombo]

Also on a related note, I was curious in what cases would we want the allowMissingValuesForOptionalFields in the GraphQLSelectionSetMapper<>.init() to be false when dealing with the cache (i.e ApolloStore)?

I'm under the assumption that for cached data, we would always want allowMissingValuesForOptionalFields to be true for both load, read & write (but so far, allowMissingValuesForOptionalFields is set to false in all occurrences of GraphQLSelectionSetMapper in ApolloStore.swift 🤔

Thanks!

[dabby-wombo]

Hi folks, just wanted to bump on this issue 🙏

[AnthonyMDev]

Hi @dabby-wombo, I've resolved this in #2912. That will be merged into main shortly, and will be included in the 1.1 GA release which we aim to push out at the end of this week!