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

Distinct values for undefined and null #177

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

bathalh
Copy link

@bathalh bathalh commented Feb 22, 2016

Projects I have been working on need to know whether missing values are really null, or whether they were undefined (or not sent at all). Scala options can't handle that, so I've created a Tription (triple option) which makes a distinction between null and undefined.

This patch is my original work and that I license the work to the spray-json project under the project’s open source license.

@bathalh
Copy link
Author

bathalh commented May 19, 2017

@ktoso @sirthias I know it's been a while, but with https://tools.ietf.org/html/rfc7396 out, it's becoming more important to distinguish between values which do not appear in JSON and values which are null. Do y'all have any thoughts on my solution to this? Or is there already a way to do this that I'm missing?

Thank you.

@bradleymdsol
Copy link

bradleymdsol commented Jun 28, 2017

Really in need of this, as I can not distinguish between removing a value on PATCH and ignoring it as @bathalh has described in the link he provided

It there any chance of getting this merged @ktoso and @sirthias

@bathalh
Copy link
Author

bathalh commented Jun 28, 2017

I realize that spray has been replaced with akka-http, but it seems like the akka libraries still use spray-json under the hood. But if you really aren't accepting anything new in spray-json, is there somewhere else I should submit this pull request?

@ktoso @sirthias

Thank you.

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contrib; I realize this comes late however still wanted to review before we cut the next release.

The example you use in the doc here seems to be an interpretation of the JSON Patch spec. It however does not talk about the undefined value, but simply uses it to mean "key not included in the patch request". See section 4 https://tools.ietf.org/html/rfc6902#section-4

On the other hand, undefined is not really part of JSON at all (sic), as the JSON spec defines explicitly:

A JSON value can be an object, array, number, string, true, false, or null. [ ECMA-404, section 5 http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf ]

So, undefined is not a valid JSON value. JSON, even though it is valid in javascript. So by adding specil handling for undefined here we actually introduce 4 states, without being able to distinguish between explicitly set to a value called "undefined" and not-being-set-at-all.

As the use-case shown seems to indicate the use is very much in line with JSON Patch, I'd rather err on not including non-JSON in this JSON library.

If you think there's some strong line of argument or need to do so regardless please show some example that would showcase that. Thanks again for the PR and patience (we know we're pretty slow on maintaining this one, however we see it pretty much as "feature complete", in it's own, minimal, feature-set)

@bathalh
Copy link
Author

bathalh commented Oct 23, 2017

Thanks for the feedback, @ktoso .
I didn't realize undefined is not part of standard JSON. I assume if that's the case, something that sends {"property":undefined} from the client would just transmit {} over the wire, and we wouldn't even need to handle the undefined case. The good news is the code I wrote also handles the case where the property isn't there at all, not just the case where it's marked as undefined. If I remove the special handling for the undefined keyword, would that be enough for you to consider incorporating it?

@ktoso
Copy link
Member

ktoso commented Oct 24, 2017

I assume if that's the case, something that sends {"property":undefined} from the client would just transmit {} over the wire

That depends what the client library decides to do with this value, we can't really assume that; though if it would send undefined then indeed it would not be legal JSON but just transmitting ecma-script assumption-carrying JSON-like data :)

If I remove the special handling for the undefined keyword, would that be enough for you to consider incorporating it?

You mean the Tription right? Yeah I guess that could be useful to detect Some(value) / null -> None / "not defined at all" -> Undefined. I'd be OK with that personally.

@bathalh
Copy link
Author

bathalh commented Nov 2, 2017

@ktoso I have removed the support for undefined while leaving in support for undefined values in with Tription. However, I am getting some mima errors from travis CI when it compiles for Scala 2.10 and 2.11. I'm getting

[error]  * method triptionFormat(spray.json.JsonFormat)spray.json.JsonFormat in trait spray.json.StandardFormats is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("spray.json.StandardFormats.triptionFormat")

for my triptionFormat, but I'm seeing the same error for code I didn't write, which must be new since I submitted my initial PR:

[error]  * method organiseMembers(scala.collection.immutable.Map)scala.collection.Seq in trait spray.json.PrettyPrinter is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("spray.json.PrettyPrinter.organiseMembers")

I'm not that familiar with mima, so I'm not sure what the issue is here...

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

Successfully merging this pull request may close these issues.

3 participants