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

Encode JSON message data as string. #459

Merged
merged 7 commits into from Aug 11, 2016

Conversation

3 participants
@tcard
Copy link
Contributor

tcard commented Aug 3, 2016

Previously, we were just inlining it:

"messages": [{"data": {"foo": "bar"}}]

What the JS library expects, and really what RSL4d3 says:

"messages": [{"data": "{\"foo\": \"bar\"}"}]

So now we do that.

tcard added some commits Aug 3, 2016

Encode JSON message data as string.
Previously, we were just inlining it:

    "messages": [{"data": {"foo": "bar"}}]

What the JS library expects, and really what (RSL4d3)[http://docs.ably.io/client-lib-development-guide/features/#RSL4d3] says:

    "messages": [{"data": "{\"foo\": \"bar\"}"}]

So now we do that.

@tcard tcard force-pushed the json-encode-string branch from ac81d2f to fff2732 Aug 8, 2016

@tcard

This comment has been minimized.

Copy link
Contributor Author

tcard commented Aug 8, 2016

I've added a test at fff2732 for what we've talked about at ably/wiki#22. If it looks good I can do it in Java as well.

Please review @mattheworiordan @paddybyers @SimonWoolf.

@mattheworiordan

This comment has been minimized.

Copy link
Member

mattheworiordan commented Aug 8, 2016

This looks good, but got a few comments:

  • It seems from https://github.com/ably/ably-common/blob/messages-encoding-fixtures/test-resources/messages-encoding.json#L6-L7 that you are checking that a RAW post of the payload and encoding translates to a realtime publish of that decoded body, but you are not checking the body is ever as expected. For example, if the realtime library incorrectly decoded an object, and then incorrect encoded the object, we'd never know as the first RAW post check and subsequent history check will pass, but the intervening payload may be incorrect. I think we need to add a "decodedData" object to messages which we check matches exactly what we get following the RAW post
  • I don't think we should call the expectedType map and array as that is platform specific. Instead, we have typically described those types as JSON Objects and JSON Arrays. I think we should stick to that for consistency and to avoid confusion.
@mattheworiordan

This comment has been minimized.

Copy link
Member

mattheworiordan commented Aug 8, 2016

BTW. Once done, I will add a corresponding test to Ruby and then a new spec item to the 0.9 spec

@tcard

This comment has been minimized.

Copy link
Contributor Author

tcard commented Aug 8, 2016

For example, if the realtime library incorrectly decoded an object, and then incorrect encoded the object, we'd never know as the first RAW post check and subsequent history check will pass, but the intervening payload may be incorrect.

It would be very unlikely that an incorrectly decoded object is then encoded correctly just as the fixture expects, wouldn't it? Plus, we really can't specify this in a platform-independent manner. Other tests should check this for each platform (they do already if they test RSL4 properly); this one is focused on interoperability.

I don't think we should call the expectedType map and array as that is platform specific.

Renamed "map" to "object" at e7f68e1. (I just picked "map" randomly really, they're called dictionaries in Swift.)

@mattheworiordan

This comment has been minimized.

Copy link
Member

mattheworiordan commented Aug 8, 2016

It would be very unlikely that an incorrectly decoded object is then encoded correctly just as the fixture expects, wouldn't it? Plus, we really can't specify this in a platform-independent manner. Other tests should check this for each platform (they do already if they test RSL4 properly); this one is focused on interoperability.

But it's not testing interoperability because of exactly that reason. Every platform supports JSON-like objects. If a developer is unable to rely on our platform to provide consistent JSON-like objects then we have a much bigger problem. We state clearly we support string, binary and JSON, so those are three types we need to test. I am sorry, but I disagree here and feel we must test that the object received is what we expected else we're open to yet more problems.

Renamed "map" to "object" at e7f68e1. (I just picked "map" randomly really, they're called dictionaries in Swift.)

Still not sure I would agree with that. We don't support Objects (that is very generic), we support JSON Objects and JSON Arrays, see http://docs.ably.io/client-lib-development-guide/features/#RSL4c3 and http://docs.ably.io/client-lib-development-guide/features/#RSL4d3

@paddybyers

This comment has been minimized.

Copy link
Member

paddybyers commented Aug 8, 2016

But it's not testing interoperability because of exactly that reason. Every platform supports JSON-like objects. If a developer is unable to rely on our platform to provide consistent JSON-like objects then we have a much bigger problem. We state clearly we support string, binary and JSON, so those are three types we need to test. I am sorry, but I disagree here and feel we must test that the object received is what we expected else we're open to yet more problems.

Yes but the issue is how we represent the expected value in the fixture data when the value is a JSON object or array, or binary, without things getting a bit circular.

I think we do have to include an expectedValue as a javascript literal value (for string, object and array cases), expecting that any platform capable of parsing the fixture data also parses these values, and allows a test to be written that does a deep comparison of the decoded value and the parsed fixture expectedValue.

In the case of binary we just have to include a serialised representation in the fixture data (lets say hex instead of base64 because it minimises ambiguity), and the tests know that if the expectedType is binary, then it does a hex decode of the expectedValue before comparing.

Still not sure I would agree with that. We don't support Objects (that is very generic), we support JSON Objects and JSON Arrays

Yes, but since the JSON spec defines what those values are it seems to make sense to use the names of the corresponding types from the JSON BNF (ie object and array, as in http://rfc7159.net/rfc7159#rfc.section.3.

@paddybyers

This comment has been minimized.

@tcard

This comment has been minimized.

Copy link
Contributor Author

tcard commented Aug 8, 2016

PTAL, implemented following @paddybyers ' proposal, and prepended "json" to "array" and "object".

@mattheworiordan

This comment has been minimized.

Copy link
Member

mattheworiordan commented Aug 8, 2016

Thanks lgtm 👍

@tcard

This comment has been minimized.

Copy link
Contributor Author

tcard commented Aug 10, 2016

PTAL

fail("\(err)")
// https://github.com/ably/wiki/issues/22
it("should encode and decode fixture messages as expected") {
let options = AblyTests.commonAppSetup()

This comment has been minimized.

@mattheworiordan

mattheworiordan Aug 10, 2016

Member

Should this not explicitly use JSON i.e. binaryProtocol = false and should the test description be updated to reflect that these tests are about JSON based interoperability?

This comment has been minimized.

@tcard

tcard Aug 10, 2016

Author Contributor

Hmm, I don't see why. Whether this uses JSON or MsgPack doesn't matter, that's not the layer we're testing. We have two fully independent layers:

  • Message data encoding JSON-like to string and back again.
  • Whole protocol message encoding to JSON or MsgPack and back again.

This test is about the former. When the encoded message data hits the latter, it is already a string in all cases.

This comment has been minimized.

@mattheworiordan

mattheworiordan Aug 10, 2016

Member

Message data encoding JSON-like to string and back again.

Well you are never technically doing that because every time you use JSON on one end MsgPack on the other.

This comment has been minimized.

@tcard

tcard Aug 10, 2016

Author Contributor

I really don't see your point here. How am I never doing that? I am pretty literally doing that, concretely here. Then I'm turning that string (doesn't matter if it's JSON or not) into either MsgPack or a JSON blob or any other protocol-level encoding we might use in the future. Then I'm turning that into maybe a gzipped blob over HTTP and then encrypting that with TLS and then Wi-Fi signals or whatever, all those encodings that also don't affect the first one at all. Am I wrong here?

This comment has been minimized.

@mattheworiordan

mattheworiordan Aug 10, 2016

Member

The point here is that tests don't know how the internals of a library work and should not care. The point of our interoperability tests was to check that a message when decoded/encoded via JSON or MsgPack are consistent with the types we expect to see. We agreed the only reliable way to get data into the Ably system was via a raw REST request to Ably, and then to check that it was as expected, was also via a raw REST history request to Ably and then to parse the JSON (or inspect the type if binary/string) and compare with what we see the library interprets that over JSON and MsgPack. So if we want to test how our Ably library encoded/decodes objects with a JSON transport it should use that, and if we want to see how our Ably library does that MsgPack it should use a MsgPack transport. This test uses MsgPack, so the JSON encoding / decoding of a message injected or received via a raw request is not being done. That is why in Ruby I have a set of tests using the JSON transport and then using the MsgPack transport but all the time using a raw JSON request to inject or check the fixture in Ably irrespective of the transport being tested for interoperability.

This comment has been minimized.

@tcard

tcard Aug 10, 2016

Author Contributor

The point of our interoperability tests was to check that a message when decoded/encoded via JSON or MsgPack are consistent with the types we expect to see.

That's the point of the second interoperability test. The point of this one is to check it the message data (not the message itself) when decoded/encoded as string, JSON or base64. We need both precisely because they test two completely independent things.

We agreed the only reliable way to get data into the Ably system was via a raw REST request to Ably, and then to check that it was as expected, was also via a raw REST history request to Ably and then to parse the JSON

That part I agree with, and if that's what you meant then OK. But since I'm doing a direct raw POST with a JSON payload (here) it doesn't matter what I set useBinaryProtocol to. Same for the raw GET for history. It still doesn't affect how the message gets transmitted via realtime.

This test uses MsgPack, so the JSON encoding / decoding of a message injected or received via a raw request is not being done.

Yes it is, because what we're encoding/decoding is the JSON inside that message, which comes as a string. These lines that decode the JSON from the message data are being executed both if the message came as JSON or as MsgPack. At that point, it's just an ARTMessage.

This comment has been minimized.

@mattheworiordan

mattheworiordan Aug 10, 2016

Member

Perhaps I am misunderstanding still, but in these tests, at what point do we publish with a raw JSON message (i.e. getting the fixture into Ably in a guaranteed portable way) and get that message over a JSON transport and confirm the message.data is as expected then?

This comment has been minimized.

@mattheworiordan

mattheworiordan Aug 10, 2016

Member

As above, and where do we publish over JSON using the library, and then confirm with a raw HTTP request that the JSON in Ably is as expected?

This comment has been minimized.

@tcard

tcard Aug 10, 2016

Author Contributor

at what point do we publish with a raw JSON message

Here. That's a POST /channels/.../messages. fixtureMessage is a JSON object (from a library for easy JSON manipulation) and rawData() converts it into a JSON string. That's quite like cutting and pasting from the messages-encoding.json directly into the POST request body.

and get that message over a JSON transport

Here. data there is the HTTP response body, which the JSON library parses.

and confirm the message.data is as expected

Here. Both persistedMessage and fixtureMessage are JSON objects that don't go through the ARTRealtime instance at any point. The ARTRealtime instance is used to test that the iOS library encodes and decodes the data as expected (ie. as expectedValue or expectedHexValue tell us), be it JSON, string or base64.

As above, and where do we publish over JSON using the library, and then confirm with a raw HTTP request that the JSON in Ably is as expected?

That's in the second test, here.

This comment has been minimized.

@mattheworiordan

mattheworiordan Aug 10, 2016

Member

and get that message over a JSON transport

Here. data there is the HTTP response body, which the JSON library parses.

But that's using client.rest.executeRequest to get that data. So it's not passing through either a Realtime subscribe API or a history API request. The goal was end-to-end API test, yet that is not using an API that handles encoding & decoding implicitly i.e. publish/subscribe/history.

As above, and where do we publish over JSON using the library, and then confirm with a raw HTTP request that the JSON in Ably is as expected?

That's in the second test, here.

Against, I don't think that is doing the job. You are mixing two concerns into one. We explicitly don't couple publishing & subscribing (regardless of protocol) into a single interoperability test because then you are simply testing that the client library when encoding & then decoding gives you the answer you expect. But that's not the point of interoperability tests.

@mattheworiordan

This comment has been minimized.

Copy link
Member

mattheworiordan commented Aug 10, 2016

One comment otherwise LGTM 👍

@tcard

This comment has been minimized.

Copy link
Contributor Author

tcard commented Aug 11, 2016

@mattheworiordan PTAL. If it looks good I'll do the same for Java.

@mattheworiordan

This comment has been minimized.

Copy link
Member

mattheworiordan commented Aug 11, 2016

LGTM. Once both are done, can we bump versions so that I can update that customer who had the issue?

@tcard tcard merged commit 235cfdc into master Aug 11, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@ricardopereira ricardopereira deleted the json-encode-string branch Sep 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.