Encode JSON message data as string. #459

tcard · 2016-08-03T23:16:42Z

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.

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.

/~https://github.com/ably/wiki/issues/22

tcard · 2016-08-08T13:21:34Z

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 · 2016-08-08T13:47:51Z

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 · 2016-08-08T13:48:24Z

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

tcard · 2016-08-08T13:56:45Z

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 · 2016-08-08T14:37:46Z

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 · 2016-08-08T14:59:52Z

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 · 2016-08-08T15:07:00Z

See /~https://github.com/ably/ably-common/tree/expected-value-proposal

tcard · 2016-08-08T16:02:05Z

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

mattheworiordan · 2016-08-08T16:28:08Z

Thanks lgtm 👍

tcard · 2016-08-10T10:29:43Z

PTAL

mattheworiordan · 2016-08-10T11:40:28Z

Spec/RealtimeClientConnection.swift

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


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?

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.

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.

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?

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.

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.

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?

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?

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.

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 · 2016-08-10T11:42:28Z

One comment otherwise LGTM 👍

tcard · 2016-08-11T12:28:11Z

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

mattheworiordan · 2016-08-11T12:56:52Z

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

tcard added 2 commits August 8, 2016 15:19

Add messages fixtures test.

fff2732

/~https://github.com/ably/wiki/issues/22

tcard force-pushed the json-encode-string branch from ac81d2f to fff2732 Compare August 8, 2016 13:19

Rename "map" to "object" in messages encoding fixtures.

e7f68e1

Use expectedValue from messages encoding fixtures.

318fcbb

mattheworiordan mentioned this pull request Aug 8, 2016

Ensure interoperability with other libraries with JSON protocol ably/ably-ruby#94

Merged

tcard mentioned this pull request Aug 9, 2016

Expected value proposal ably/ably-common#17

Merged

tcard added 2 commits August 9, 2016 14:00

Update ably-common.

3176c62

Ensure interoperability with other libraries over MsgPack.

1ee851c

mattheworiordan reviewed Aug 10, 2016
View reviewed changes

mattheworiordan mentioned this pull request Aug 10, 2016

0.9 compatibility - version HTTP headers, fallbackHosts, request() and decoding interop. test ably/ably-php#37

Merged

tcard mentioned this pull request Aug 10, 2016

Interoperability and event handler robustness ably/docs#146

Merged

Do message encoding tests in both MsgPack and JSON.

235cfdc

tcard merged commit 235cfdc into master Aug 11, 2016

ricardopereira deleted the json-encode-string branch September 28, 2016 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode JSON message data as string. #459

Encode JSON message data as string. #459

tcard commented Aug 3, 2016 •

edited

Loading

tcard commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

tcard commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

paddybyers commented Aug 8, 2016

paddybyers commented Aug 8, 2016

tcard commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

tcard commented Aug 10, 2016

mattheworiordan Aug 10, 2016

tcard Aug 10, 2016 •

edited

Loading

mattheworiordan Aug 10, 2016

tcard Aug 10, 2016

mattheworiordan Aug 10, 2016

tcard Aug 10, 2016

mattheworiordan Aug 10, 2016

mattheworiordan Aug 10, 2016

tcard Aug 10, 2016

mattheworiordan Aug 10, 2016 •

edited

Loading

mattheworiordan commented Aug 10, 2016

tcard commented Aug 11, 2016

mattheworiordan commented Aug 11, 2016

Encode JSON message data as string. #459

Encode JSON message data as string. #459

Conversation

tcard commented Aug 3, 2016 • edited Loading

tcard commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

tcard commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

paddybyers commented Aug 8, 2016

paddybyers commented Aug 8, 2016

tcard commented Aug 8, 2016

mattheworiordan commented Aug 8, 2016

tcard commented Aug 10, 2016

Choose a reason for hiding this comment

tcard Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

mattheworiordan commented Aug 10, 2016

tcard commented Aug 11, 2016

mattheworiordan commented Aug 11, 2016

tcard commented Aug 3, 2016 •

edited

Loading

tcard Aug 10, 2016 •

edited

Loading

mattheworiordan Aug 10, 2016 •

edited

Loading