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

toJSON drops names of named vectors #76

Closed
wch opened this issue Mar 9, 2015 · 17 comments
Closed

toJSON drops names of named vectors #76

wch opened this issue Mar 9, 2015 · 17 comments

Comments

@wch
Copy link
Contributor

wch commented Mar 9, 2015

For example:

jsonlite::toJSON(list(c(a="A", b="B")))
# [["A","B"]] 

This differs from the behavior of RJSONIO:

cat(RJSONIO::toJSON(list(c(a="A", b="B"))))
# [
#  {
#  "a": "A",
# "b": "B" 
# }

It would be great to have an option to control the behavior of this. (There are some features of Shiny don't work correctly because they send named vectors and expect them to be translated into objects instead of arrays).

@jeroen
Copy link
Owner

jeroen commented Mar 17, 2015

I'm not so sure if this is a good idea. We could add it as some sort of legacy mode, but I really believe you are better off without this feature. Here my concern:

  • Type safety: jsonlite tries to consistently map every R class to a particular json structure, so that the consumer of the json knows what to expect even if the data is dynamic and the content is unknown. In R, adding a names attribute to a vector is harmless, and many functions quite liberally add or drop names to vectors. However if the json output of a particular vector completely changes depending on the presence of a names attribute, you can get bugs where the client was expecting an array but got an object.
  • Using json objects exclusively for lists and data frames results in much better reversibility. If we start mapping named vectors to json objects, then people are going to expect that fromJSON will simplify the json objects back into vectors. This opens up a new pandora's box of edge cases.
  • It is very easy for the user to use as.list on their named vector and get the desired json output.
  • R does not allow for x$foo for named vectors anymore (it used to), so it is not quite the same as a dictionary. IMO, the names attribute is commonly used for annotation rather than as a primary key (even though R does allow for x["foo"])
  • Atomic vectors are the building blocks for matrices and data frames. We will have to come up with rules for how to map named vectors when they appear in here. RJSONIO has not done this and is broken for these cases.
  • If we are going to incorporate the names attribute for vectors, we are probably going to need to incorporates row and column names for matrices as well, which makes things even more complex and will completely destroy the reversibility of the json mapping.

So all in all I feel it is more safe and simple to require users to as.list their named vector if they want it to turn it into a json object, rather than add more magic to the json encoder. Of course I understand the argument of keeping things as much as possible compatible with RJSONIO to easy the transition for shiny users, but I think in the long run adding this option will cause you more damage than it solves.

@wch
Copy link
Contributor Author

wch commented Mar 18, 2015

Those are all fair points. However... there is already quite a bit of code out there for Shiny, and I know that some of it uses named vectors with the expectation that they will be turned into JSON objects. This code isn't common, but it is out there.

How about having a backward-compatibility option for this, where it will output JSON objects and also print a message saying that a named list should be used instead? Then I think after some time, it could be changed to an error, and then later it could be removed.

@jeroen
Copy link
Owner

jeroen commented Mar 18, 2015

Ugh, first thing I try:

x <- structure(as.complex(1:3), names=c("foo", "bar", "baz"))
RJSONIO::toJSON(x)

@wch
Copy link
Contributor Author

wch commented Mar 18, 2015

Another reason that I'd like to move away from RJSONIO. :)

@jeroen
Copy link
Owner

jeroen commented Mar 18, 2015

This is also interesting:

x <- matrix(1:4, 2)
names(x) <- letters[1:4]
cat(RJSONIO::toJSON(x))

It gets even better when you add:

colnames(x) <- c("foo", "bar")
cat(RJSONIO::toJSON(x))

@jeroen
Copy link
Owner

jeroen commented Mar 18, 2015

I'm not sure if there is a sensible way to add compatibility with an implementation that is broken for so many edge cases :/

@wch
Copy link
Contributor Author

wch commented Mar 18, 2015

Well, I was just planning on making it work for 1-d named vectors, and ignoring matrices and arrays with names. As far as I'm concerned, those can be treated as though they're unnamed, and I don't feel a need to keep backward compatibility with them.

I think that in order to migrate shiny to jsonlite, we need at least some way of working with named 1-d vectors, otherwise people will start experience mysterious breakage (that's how I encountered this issue in the first place). My thought is that the compatibility option would just be there as a stopgap solution, until developers have enough time to switch their code to named lists.

I've written a partial implementation for asJSON.numeric, which has a part that looks like this (it may need modification to work with arrays and matrices):

  if (isTRUE(keep_vec_names) && !is.null(names(x))) {
    message("Input to asJSON(keep_vec_names=TRUE) is a named vector. ",
      "In a future version of jsonlite, this option will not be supported, ",
      "and named vectors will be translated into arrays instead of objects. ",
      "If you need JSON object output, please use a named list. See ?toJSON.")

    return(asJSON(as.list(x), digits = digits, use_signif = use_signif, na = na,
      auto_unbox = TRUE, collapse = collapse, ...))
  }

@jeroen
Copy link
Owner

jeroen commented Mar 18, 2015

This is similar to the RJSONIO implementation. It will result in strange output if the vector at hand is part of a data frame or matrix.

@wch
Copy link
Contributor Author

wch commented Mar 18, 2015

Oh, good point. I'll look into this some more later and hopefully I'll come up with something that's not too horrible.

@jeroen
Copy link
Owner

jeroen commented Mar 18, 2015

We could also not change the mapping, but still give a warning/error for named vectors. Then the shiny apps will still be broken, but it will be immediately obvious what the problem is and we can help people fix it.

@wch
Copy link
Contributor Author

wch commented Mar 18, 2015

I'm really reluctant to just allow apps to break. It's really unpleasant for a user to upgrade their packages and have it break their code. It's even worse if a package has a Shiny app that breaks - then the package has to be updated, submitted, accepted, and the user has to install the updated package.

I think that perfect compatibility with RJSONIO isn't necessary. For example, as you pointed out, RJSONIO's conversion of named matrices is almost nonsensical, and I doubt anyone would want that output -- but I believe that it's not that rare for users to expect named vectors to turn into JSON objects.

@mattflor
Copy link

So I am getting the warning

Input to asJSON(keep_vec_names=TRUE) is a named vector. In a future version of jsonlite, this option will not be supported, and named vectors will be translated into arrays instead of objects. If you want JSON object output, please use a named list instead. See ?toJSON.

in a (rather large) shiny app. How do I find out which part of my app is causing this?

@wch
Copy link
Contributor Author

wch commented Feb 29, 2016

If you use options(warn=2), it will raise the warning to an error, and then you'll get a stack trace. If you use the latest version of Shiny from CRAN, it has better stack trace printing, so that should help you find the source of the problem.

@mattflor
Copy link

Thanks for the lightning fast reply! I am using the latest shiny 0.13.1. However, setting options(warn = 2) does not give me a stack trace. I tried also setting options(shiny.error = recover) and options(error = recover). The latter finally produced a stack trace but only after I manually stop the app. Is this expected?

Anyway, the stack trace I see does not involve any of my app code but to me looks like shiny internals...

Now, I've turned on options(shiny.trace = TRUE), and I see a somewhat frightening Warning: Error in : C stack usage 14770274 is too close to the limit right after the asJSON(keep_vec_names=TRUE) warnings. Guess I'll have to read up on the debugging techniques you guys describe at http://shiny.rstudio.com/articles/debugging.html

@rchen1207
Copy link

Hi @mattflor , I am having this issue as well. Have you found a way to get a stack trace?

@mattflor
Copy link

mattflor commented May 8, 2016

Well, as described in the debugging article there's options(shiny.trace = TRUE) as well as options(shiny.fullstacktrace = TRUE). If I remember correctly, it turned out that in my case I was inadvertently sending a large object to a hidden output slot which resulted in both the asJSON warnings and the C stack error.

@rchen1207
Copy link

Thank you!

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 a pull request may close this issue.

4 participants