Make Json.stringify throw on JsUndefined #3888

Merged
merged 1 commit into from Feb 4, 2015

Conversation

Projects
None yet
4 participants
@gmethvin
Member

gmethvin commented Feb 3, 2015

No description provided.

@cchantep

This comment has been minimized.

Show comment
Hide comment
@cchantep

cchantep Feb 3, 2015

Member

"null" for undefined?

Member

cchantep commented Feb 3, 2015

"null" for undefined?

@gmethvin

This comment has been minimized.

Show comment
Hide comment
@gmethvin

gmethvin Feb 3, 2015

Member

@cchantep That is the default way it is stringified by Json.stringify. Another option would be to always throw an exception when stringifying a JsUndefined.

Member

gmethvin commented Feb 3, 2015

@cchantep That is the default way it is stringified by Json.stringify. Another option would be to always throw an exception when stringifying a JsUndefined.

@cchantep

This comment has been minimized.

Show comment
Hide comment
@cchantep

cchantep Feb 3, 2015

Member

Or introducing some Try of Future which would handle error case. What's weird to me is to have "null" for a missing value (I would have say null if that not an error case).

Member

cchantep commented Feb 3, 2015

Or introducing some Try of Future which would handle error case. What's weird to me is to have "null" for a missing value (I would have say null if that not an error case).

@gmethvin

This comment has been minimized.

Show comment
Hide comment
@gmethvin

gmethvin Feb 3, 2015

Member

I would actually much rather define some other type for dealing with traversal so JsUndefined doesn't need to be a JsValue. But that's a pretty significant API change.

Anyway, the point of this was to make sure these methods always return valid JSON, which is at least an incremental improvement.

We should consider a few API changes in the future though. In particular I'd like to separate structured values (object and array) from primitive values so you can't do things like Ok(JsString("foo")).

Member

gmethvin commented Feb 3, 2015

I would actually much rather define some other type for dealing with traversal so JsUndefined doesn't need to be a JsValue. But that's a pretty significant API change.

Anyway, the point of this was to make sure these methods always return valid JSON, which is at least an incremental improvement.

We should consider a few API changes in the future though. In particular I'd like to separate structured values (object and array) from primitive values so you can't do things like Ok(JsString("foo")).

@cchantep

This comment has been minimized.

Show comment
Hide comment
@cchantep

cchantep Feb 3, 2015

Member

If the scope of change is limited my question would only be why "null" instead null, which would make the undefined value a sub case of string (not at ease with that).

Why would Ok(JsString("..")) be removed?

Member

cchantep commented Feb 3, 2015

If the scope of change is limited my question would only be why "null" instead null, which would make the undefined value a sub case of string (not at ease with that).

Why would Ok(JsString("..")) be removed?

@gmethvin

This comment has been minimized.

Show comment
Hide comment
@gmethvin

gmethvin Feb 4, 2015

Member

Oh. I missed what you were asking before. The output is actually the string null (no quotes). That was supposed to be a Scala expression, i.e. Json.stringify(JsUndefined()) === "null".

My other point was that only array and object are valid top-level types in application/json, so ideally we'd want to further restrict the possible result types to just those by default.

Member

gmethvin commented Feb 4, 2015

Oh. I missed what you were asking before. The output is actually the string null (no quotes). That was supposed to be a Scala expression, i.e. Json.stringify(JsUndefined()) === "null".

My other point was that only array and object are valid top-level types in application/json, so ideally we'd want to further restrict the possible result types to just those by default.

@cchantep

This comment has been minimized.

Show comment
Hide comment
@cchantep

cchantep Feb 4, 2015

Member

Some client lib (such as jQuery) allow primitive types as top level. I have already seen such use in some project.

Member

cchantep commented Feb 4, 2015

Some client lib (such as jQuery) allow primitive types as top level. I have already seen such use in some project.

@julienrf

This comment has been minimized.

Show comment
Hide comment
@julienrf

julienrf Feb 4, 2015

Contributor

Why is JsUndefined a JsValue? Anyway, just throw an exception if one tries to output it but please do not output null.

Contributor

julienrf commented Feb 4, 2015

Why is JsUndefined a JsValue? Anyway, just throw an exception if one tries to output it but please do not output null.

@jroper

This comment has been minimized.

Show comment
Hide comment
@jroper

jroper Feb 4, 2015

Member

Having JsUndefined a JsValue means you can safely:

(json \ "foo" \ "bar").asOpt[String]

and this will return None if either foo or bar is not defined, which is pretty convenient from a users point of view - I've taken advantage of this feature before. Essentially, it means that during json path traversal, the undefinedness of one part of the traversal can be carried to when you try to get some concrete value out of it. There are a number of other ways that this could be achieved, but having JsUndefined is an incredibly convenient one, and means that at the end of the traversal, you still have a JsValue, which you can pass around etc. The caveat of course is that that JsValue may be undefined, which makes it awkward in certain circumstances, especially the one that this pull request is about.

I don't have a strong opinion on what it should be, but I would like to see impact to users of any changes be minimal.

Member

jroper commented Feb 4, 2015

Having JsUndefined a JsValue means you can safely:

(json \ "foo" \ "bar").asOpt[String]

and this will return None if either foo or bar is not defined, which is pretty convenient from a users point of view - I've taken advantage of this feature before. Essentially, it means that during json path traversal, the undefinedness of one part of the traversal can be carried to when you try to get some concrete value out of it. There are a number of other ways that this could be achieved, but having JsUndefined is an incredibly convenient one, and means that at the end of the traversal, you still have a JsValue, which you can pass around etc. The caveat of course is that that JsValue may be undefined, which makes it awkward in certain circumstances, especially the one that this pull request is about.

I don't have a strong opinion on what it should be, but I would like to see impact to users of any changes be minimal.

@jroper

This comment has been minimized.

Show comment
Hide comment
@jroper

jroper Feb 4, 2015

Member

@gmethvin RFC 7159 (Tim Bray's updated JSON spec which is not yet final) allows any top level type, with a note that implementations that only produce objects or arrays at the top level are "interoperable":

A JSON text is a sequence of tokens. The set of tokens includes six structural characters, strings, numbers, and three literal names.

A JSON text is a serialized value. Note that certain previous specifications of JSON constrained a JSON text to be an object or an array. Implementations that generate only objects or arrays where a JSON text is called for will be interoperable in the sense that all implementations will accept these as conforming JSON texts.

Member

jroper commented Feb 4, 2015

@gmethvin RFC 7159 (Tim Bray's updated JSON spec which is not yet final) allows any top level type, with a note that implementations that only produce objects or arrays at the top level are "interoperable":

A JSON text is a sequence of tokens. The set of tokens includes six structural characters, strings, numbers, and three literal names.

A JSON text is a serialized value. Note that certain previous specifications of JSON constrained a JSON text to be an object or an array. Implementations that generate only objects or arrays where a JSON text is called for will be interoperable in the sense that all implementations will accept these as conforming JSON texts.

@jroper

This comment has been minimized.

Show comment
Hide comment
@jroper

jroper Feb 4, 2015

Member

Thinking about this a little more, it's probably a good idea to throw an exception when we try to serialise JsUndefined. There may be some impact on users, but other methods that expect a value throw an exception (eg, as), so I think this would be consistent.

Member

jroper commented Feb 4, 2015

Thinking about this a little more, it's probably a good idea to throw an exception when we try to serialise JsUndefined. There may be some impact on users, but other methods that expect a value throw an exception (eg, as), so I think this would be consistent.

@gmethvin gmethvin changed the title from Always use Json.stringify instead of _.toString to Make Json.stringify throw on JsUndefined Feb 4, 2015

@gmethvin

This comment has been minimized.

Show comment
Hide comment
@gmethvin

gmethvin Feb 4, 2015

Member

I updated the PR to throw an exception when we write a JsUndefined.

Member

gmethvin commented Feb 4, 2015

I updated the PR to throw an exception when we write a JsUndefined.

cchantep added a commit that referenced this pull request Feb 4, 2015

Merge pull request #3888 from gmethvin/jsvalue-tostring
Make Json.stringify throw on JsUndefined

@cchantep cchantep merged commit 069b5a4 into playframework:master Feb 4, 2015

1 check passed

default Merged build finished.
Details

@gmethvin gmethvin deleted the gmethvin:jsvalue-tostring branch Feb 4, 2015

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