fix for correctly JSON-marshalling subscription input variables within WebSocketConnectionManager

[gmulhearn]

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.

[richardmcclellan]

Thanks for this fix! The bug explanation and solution seem reasonable to me.

FWIW, we don't have this same bug in Amplify because the variables map is serialized using gson, rather than JsonObject. For this AppSync SDK though, using the ScalarTypeAdapters does make the most sense.

The one request I have is - could you add a unit test that fails without this change, and passes with this change?

[gmulhearn]

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]

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]

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

[gmulhearn]

BUMP: merged in recent changes from upstream

[gmulhearn]

UPDATE: moved over to a new repo for management reasons. Please see the new PR:
#365