search

awslabs / aws-mobile-appsync-sdk-android

Android SDK for AWS AppSync.
https://docs.amplify.aws/sdk/api/graphql/q/platform/android/
Apache License 2.0
105 stars 58 forks source link

fix for correctly JSON-marshalling subscription input variables within WebSocketConnectionManager #351

Closed gmulhearn closed 3 years ago

gmulhearn commented 3 years ago

Issue #, if available:

352

Description of changes: The startSubscription method in WebSocketConnectionManager now attempts to marshal subscription variables using the generated marshaller rather than just trying to call JSONObject(map) over the subscription's valueMap. More information about the bug attached in the issue.

startSubscription now adds the 'data' JSON field with a value equal to what is returned from htttpRequestBody(subscription).

httpRequestBody is a slightly modified version of the httpRequestBody method from AppSyncOfflineMutationManager. An extension has been made on the function to handle IOException's by reverting to the old way of creating the field's value (i.e. with a JSONObject(map) over the valueMap). This defense still can throw a JSONException however that is already handled by the caller function (startSubscription)

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

gmulhearn commented 3 years ago

hey Richard, thanks for the quick reply. I'll get onto that unit test now. Thanks for the tip about amplify, will definitely opt for that on my next project.

gmulhearn commented 3 years ago

Hey @richardmcclellan , i've wrote a unit test that fails before the change and succeeds after. I created a dummy version of a Subscription containing an ENUM field. and then check if the JSON string sent to the mocked WebSocket has the enum value nulled out or not.

I didn't want to have to create an additional Subscription class for testing in my test, but i couldn't think of any other way.

Let me know if there's any changes you'd recommend, cheers!

P.S. don't be alarmed by the force-pushes to my fork above ^... i was changing my git author of my commits as i've forgotten to change the git config on this machine im using..

gmulhearn commented 3 years ago

hey @richardmcclellan any chance you got to have a look at this?

gmulhearn commented 3 years ago

BUMP: merged in recent changes from upstream

gmulhearn commented 3 years ago

UPDATE: moved over to a new repo for management reasons. Please see the new PR: https://github.com/awslabs/aws-mobile-appsync-sdk-android/pull/365