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

ApolloSQLite -> Fix deserialize method for custom scalars #1144

Merged

Conversation

ppowers10
Copy link

@ppowers10 ppowers10 commented Apr 10, 2020

Fix:

See Spectrum conversation regarding this issue

Changes:

  • Removed invalidRecordValue as it is no longer used anywhere

  • Updated deserialize method to return fieldJSONValue when a lack of a $reference value is found. This allows custom scalar types to return properly rather than throwing the error.

  • Updated tests to remove any invalidRecordValue thrown errors from SQLiteNormalizedCacheError as they are not used anymore. Instead, this test fails in the same manner that InMemoryNormalizedCache due to a JSONDecodingError.

  • Tests Passing

@apollo-cla
Copy link

@ppowers10: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@ppowers10 ppowers10 changed the title Fix/sqlite deserialize ApolloSQLite -> Fix deserialize method for custom scalars Apr 10, 2020
case .couldNotConvert(value: _, to: _):
break
case .missingValue, .nullValue, .wrongType:
XCTFail("Invalid error type.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just do this and have this be a lot simpler:

switch graphQLError.underlying {
case JSONDecodingError.couldNotConvert(value: _, to: _)?:
  break
default: 
   XCTFail("Invalid error type")
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@designatednerd - The above change was made, but it looks like CircleCI is complaining about ci/circleci: Apollo iOS 12.4 build. I ran the tests locally and didn't see any issues. I'm unable to trigger the build process again to see if it might pass and it was a glitch. My previous code passed all the tests on all the CircleCI checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there's one test that gets flaky a decent amount - I'll kick the build

@designatednerd designatednerd merged commit 149806e into apollographql:master Apr 11, 2020
@designatednerd
Copy link
Contributor

(approved, obviously)

@designatednerd designatednerd added this to the Next Release milestone Apr 11, 2020
@gsabran
Copy link

gsabran commented Apr 21, 2020

👍

1 similar comment
@gsabran
Copy link

gsabran commented Apr 21, 2020

👍

@designatednerd designatednerd linked an issue Apr 28, 2020 that may be closed by this pull request
gsabran pushed a commit to gsabran/apollo-ios that referenced this pull request May 14, 2020
…deserialize

ApolloSQLite -> Fix deserialize method for custom scalars

(cherry-pick 149806e)
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.

iOS Cache Serialisation to use custom scalar types
4 participants