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

fix: deserialization of custom scalars #536

Merged
merged 3 commits into from
May 29, 2022

Conversation

jellybeansoup
Copy link
Contributor

Issue #, if available:
None.

Description of changes:
We use a number of custom scalars with AWS AppSync in our codebase, which are successfully parsed from the AppSync service, and get stored within the SQLite cache, but fail to be deserialised and thus cause cache reads involving these types to completely fail. This has primarily meant that we are unable to cache these calls at all, relying on live data for calls relying on our custom scalar types.

This is exactly the same as an issue raised on the Apollo library in 2018, and was resolved by returning the fieldJSONValue from the SQLiteSerialization.deserialize(fieldJSONValue:), which correctly returns the custom scalar value as-is (in this PR).

I've replicated the changes here, including the removal of the then defunct AWSAppSyncQueriesCacheError.invalidRecordValue case.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jellybeansoup jellybeansoup requested a review from a team as a code owner May 17, 2022 03:48
@atierian
Copy link
Contributor

Thanks for opening this PR (and the other one). We'll review and come back to you on this.

Copy link
Contributor

@lawmicha lawmicha left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR @jellybeansoup. Let's verify integration tests are passing as expected (SDK owners can assist in running this if it's difficult for developer contributor to perform this)

throw AWSAppSyncQueriesCacheError.invalidRecordValue(value: fieldJSONValue)
return fieldJSONValue
Copy link
Contributor

Choose a reason for hiding this comment

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

From the Apollo issue linked in the description, has the PR with the same change to the SQLiteSerialization file here https://github.com/apollographql/apollo-ios/pull/1144/files#diff-e9bbcdac7903360aed17a5df82e644121e800adbc8b56bc03ffcf1dccc7dfe0a . With this, this change looks accurate to do since I think the file was lifted from the apollo codebase.

@jellybeansoup
Copy link
Contributor Author

Let's verify integration tests are passing as expected (SDK owners can assist in running this if it's difficult for developer contributor to perform this)

I'd certainly appreciate it if someone could assist; I had trouble getting the appropriate AWS environment set up (the lambda instructions in particular), but I did verify that the other tests run and pass as expected.

@lawmicha lawmicha changed the title Fix deserialization of custom scalars fix: deserialization of custom scalars May 27, 2022
@lawmicha
Copy link
Contributor

Let's verify integration tests are passing as expected (SDK owners can assist in running this if it's difficult for developer contributor to perform this)

I'd certainly appreciate it if someone could assist; I had trouble getting the appropriate AWS environment set up (the lambda instructions in particular), but I did verify that the other tests run and pass as expected.

Ran the integration tests and they're all passing

@jellybeansoup jellybeansoup requested a review from lawmicha May 28, 2022 01:21
@jellybeansoup
Copy link
Contributor Author

Thanks for running the integration tests on my behalf, @lawmicha! I've cherry-picked the commit to return the removed error case as suggested, so it should be good for another look 😊

@lawmicha lawmicha merged commit e64aec4 into awslabs:main May 29, 2022
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.

3 participants