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

Updating a local cache mutation with an optional field fails with a ApolloAPI.JSONDecodingError.missingValueerror #2697

Closed
amseddi opened this issue Dec 2, 2022 · 5 comments · Fixed by #2751
Assignees
Labels
bug Generally incorrect behavior codegen Issues related to or arising from code generation

Comments

@amseddi
Copy link

amseddi commented Dec 2, 2022

Bug report

Updating a local cache mutation with an optional field fails with a ApolloAPI.JSONDecodingError.missingValueerror.

Versions

  • apollo-ios SDK version: 1.0.5
  • Xcode version: 14.1
  • Swift version: 5.7
  • Package manager: SPM

Steps to reproduce

I created a failing unit test here amseddi@a4e1190.

ReadWriteFromStoreTests.swift

func test_updateCacheMutationWithOptionalField_updateNestedField_updatesObjects() throws {
     // 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
           data.hero.name = "Artoo"
         }
       }, completion: { result in
         defer { updateCompletedExpectation.fulfill() }
         /// Expected success, but result was an error: GraphQLExecutionError(path: hero.nickname, underlying: ApolloAPI.JSONDecodingError.missingValue)
         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.name, "Artoo")
         XCTAssertNil(data.hero.nickname)
       }
     }
   }
@calvincestari
Copy link
Member

Hi @amseddi, thanks for creating this issue and for the sample test. We'll take a deeper look and get back to you.

@AnthonyMDev
Copy link
Contributor

Interesting! I have a feeling that this has to do with the execution layer expecting nickname to be omitted rather than NSNull. But I do think that's a bug? If I remember correctly, we don't write null values into the cache normally. If you get one in a response from the server, we just ignore it and omit the field from the cache.

@amseddi in your actual code that revealed this issue, are you explicitly setting a field to NSNull? If so, can you try setting the field to nil and see if that works. Either way, we should probably fix this, so thanks for bringing it to our attention!

@amseddi
Copy link
Author

amseddi commented Dec 7, 2022

If I omit the value from the cache setup I get an error on the cache loading.

The following unit test fails if I don't add a NSNull() value in the cache:

func test_readQuery_givenQueryDataWithVariableInCache_readsQuery() throws {
    // given
    enum Episode: String, EnumType {
      case JEDI
    }

    class HeroNameSelectionSet: MockSelectionSet {
      override class var __selections: [Selection] { [
        .field("hero", Hero.self, arguments: ["episode": .variable("episode")])
      ]}

      class Hero: MockSelectionSet {
        override class var __selections: [Selection] {[
          .field("__typename", String.self),
          .field("name", String.self),
          .field("nickname", String?.self),
        ]}
      }
    }

    let query = MockQuery<HeroNameSelectionSet>()
    query.__variables = ["episode": Episode.JEDI]

    mergeRecordsIntoCache([
      "QUERY_ROOT": ["hero(episode:JEDI)": CacheReference("hero(episode:JEDI)")],
      "hero(episode:JEDI)": ["__typename": "Droid", "name": "R2-D2"]
      // The next line makes this test succeed
      // "hero(episode:JEDI)": ["__typename": "Droid", "name": "R2-D2", "nickname": NSNull()]
    ])

    // when
    runActivity("read query") { _ in
      let readCompletedExpectation = expectation(description: "Read completed")
      store.withinReadTransaction({ transaction in
        let data = try transaction.read(query: query)

        // then
        expect(data.hero?.__typename).to(equal("Droid"))
        expect(data.hero?.name).to(equal("R2-D2"))
        expect(data.hero?.nickname).to(beNil())

      }, completion: { result in
        defer { readCompletedExpectation.fulfill() }
        /// test_readQuery_givenQueryDataWithVariableInCache_readsQuery(): failed - Expected success, but result was an error: GraphQLExecutionError(path: hero.nickname, underlying: ApolloAPI.JSONDecodingError.missingValue)
        XCTAssertSuccessResult(result)
      })

      self.wait(for: [readCompletedExpectation], timeout: Self.defaultWaitTimeout)
    }
  }

@AnthonyMDev
Copy link
Contributor

Odd that definitely seems like a bug then, thank you for looking into it! We'll look deeper here.

@calvincestari calvincestari added bug Generally incorrect behavior codegen Issues related to or arising from code generation and removed needs investigation labels Dec 7, 2022
@calvincestari calvincestari added this to the Release 1.0.6 milestone Dec 7, 2022
@AnthonyMDev AnthonyMDev self-assigned this Jan 3, 2023
@AnthonyMDev
Copy link
Contributor

This one ended up being a lot harder than I expected, but the PR is up! #2751

Thanks for surfacing this bug to us @amseddi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Generally incorrect behavior codegen Issues related to or arising from code generation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants