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

Make sure boolean data is encoded correctly #51

Merged
merged 2 commits into from
May 12, 2015
Merged

Make sure boolean data is encoded correctly #51

merged 2 commits into from
May 12, 2015

Conversation

SimonWoolf
Copy link
Member

Fixes ci breakage of #50

@paddybyers

SimonWoolf added a commit that referenced this pull request May 12, 2015
Make sure boolean data is encoded correctly
@SimonWoolf SimonWoolf merged commit 2f6cabf into master May 12, 2015
@mattheworiordan
Copy link
Member

This looks good, but out of interest, why are you supporting the Boolean type? We agreed some time back that we would support Strings, Byte Arrays and JSON objects only, see message encoding under http://docs.ably.io/client-lib-development-guide/features/#rest-channel

@paddybyers
Copy link
Member

It says "objects capable of JSON representation" which includes numbers and booleans.

@mattheworiordan
Copy link
Member

True, but not seen tests for this before so we can't expect this will work as expected in every language. I will modify the spec then to explicitly state that our expectation includes boolean, numbers, arrays and dictionary/hash/collection-like objects (specific to the language) so that in every language there is a suitable test to ensure portability. Currently if you look at PHP this has not been tested, neither is Java or Ruby it's not covered.

@paddybyers
Copy link
Member

I will modify the spec then to explicitly state that our expectation includes boolean, numbers, arrays and dictionary/hash/collection-like objects (specific to the language) so that in every language there is a suitable test to ensure portability.

I think the documentation for each language we should document the mapping between types; ie for each of the supported types in JSON (Number, Boolean, String, Object, Array):

  • how it is passed to the client in the relevant language, if received from Ably;
  • how the client is expected to pass it if they want to pass that value.

It is not always true that the mapping will be to the "natural" representation of that type in the target language; only that there is a defined type that makes it possible. For example in Java, the JSON-encodable types are JSONObject and JSONArray, not general Java collections.

@mattheworiordan
Copy link
Member

The problem is that we are not actually following the JSON spec if we do this. Supporting the value "123" as a JSON representation is in fact incorrect, from json.org:

JSON is built on two structures:
A collection of name/value pairs. In various languages, this is realized as an object, record, struct, dictionary, hash table, keyed list, or associative array.
An ordered list of values. In most languages, this is realized as an array, vector, list, or sequence.

So for example, using Ruby's native JSON library, parsing a non JSON like object (i.e. not starting with a { or [) raises an error, see below:

[4] pry(main)> JSON.parse('true')
JSON::ParserError: 757: unexpected token at 'true'

There is a serious risk that in any language that strictly supports the JSON spec we're going to have to build our own pre-parser to try and identify value types as opposed to actual JSON objects.

We spent a lot of time removing code some time back responsible for type support, and we agreed it was not a wise idea. However we backtracked in favour or JSON, however I do not think a Numeric is really a JSON type, it's simply one of the supported value types.

@mattheworiordan
Copy link
Member

FYI, I've implemented JSON ruby type support in Ruby however it fails because the JSON decoder does not accept an invalid JSON string

@paddybyers
Copy link
Member

OK, you're right, the RFC is explicit: "JSON text is a serialized object or array"

http://www.ietf.org/rfc/rfc4627.txt

By contrast, the ECMA spec permits any text that satisfies the grammar for a JSON value (which does include primitives):

http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants