You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is somewhat related to #62, but only that it was discovered via the conversation and thoughts around that issue.
I think independent of that issue there's a small change that could prevent a hard-to-track-down problem (for some).
The issue is if toJSON() is ever called -- even when it shouldn't be -- on a reference-semantics object, like an R6 object, simply calling toJSON() will 'damage' the original object.
Here's a trivial example:
x<-R6::R6Class("Klass")$new() ## do-nothing Klass
class(x)
# [1] "Klass" "R6"x# <Klass># Public:# clone: function (deep = FALSE)jsonlite::toJSON(x) ## won't work, because no methods for Klass nor R6# Error: No method asJSON S3 class: R6
class(x) ## but now `x` has been stripped of Klass! this is (probably) unexpected# [1] "R6"x## indeed, `x` has been modified _in-place_# <R6># Public:# clone: function (deep = FALSE)
The culprit is this line, where structure() modifies reference-semantics objects in-place:
I'm not aware of an obvious way to deal with this directly via the object x, and attempting to clone the object (to then recurse on the cloned version) seems both inefficient and way of out scope for {jsonlite}.
BUT, since at this point in the dispatch code we'd be dealing with S3 objects (well, not S4 objects), and the existing code effectively replicates the NextMethod() procedure, we can conceivably look for the first non-NULL getMethod() result, like so:
...
} elseif (length(class(x)) >1) {
for (clsin class(x)[-1]) { ## this should always fail for the first el of class(), else we'd already have dispatched to its methodf<- getMethod("asJSON", cls, optional=TRUE)
if (!is.null(f)) f(x, ...) ## note that the `force` arg is no longer needed since we found our method
}
} elseif...
toJSON() errs via unclass(), even when force = TRUE is used.
A second unexpected error can happen when unclass(x) fails, even with force = TRUE, here:
# As a last resort we can force encoding using the unclassed object
return(asJSON(unclass(x), force=force, ...))
thing<-R6::R6Class("Thing")$new()
jsonlite::toJSON(thing, force=TRUE)
# Error in unclass(x) : cannot unclass an environment
This one is easy-enough to handle, with some try() or tryCatch() logic, I think.
PR
I've opened a PR that helps to deal with these cases; all tests pass and I think there's only minimal additional overhead (versus the structure() & unclass() recursive approach). I haven't added tests yet for preventing either the mutation of x or erring (when unclass(x) fails), but if this PR has any potential future of being merged, I'm obviously happy to write those test cases, too.
Side-note, is it possible these two lines are out-of-order?
structure()
modifies the object (unexpectedly)This is somewhat related to #62, but only that it was discovered via the conversation and thoughts around that issue.
I think independent of that issue there's a small change that could prevent a hard-to-track-down problem (for some).
The issue is if
toJSON()
is ever called -- even when it shouldn't be -- on a reference-semantics object, like an R6 object, simply callingtoJSON()
will 'damage' the original object.Here's a trivial example:
The culprit is this line, where
structure()
modifies reference-semantics objects in-place:jsonlite/R/asJSON.ANY.R
Line 13 in 0134cba
I'm not aware of an obvious way to deal with this directly via the object
x
, and attempting to clone the object (to then recurse on the cloned version) seems both inefficient and way of out scope for {jsonlite}.BUT, since at this point in the dispatch code we'd be dealing with S3 objects (well, not S4 objects), and the existing code effectively replicates the
NextMethod()
procedure, we can conceivably look for the first non-NULLgetMethod()
result, like so:toJSON()
errs viaunclass()
, even whenforce = TRUE
is used.A second unexpected error can happen when
unclass(x)
fails, even withforce = TRUE
, here:jsonlite/R/asJSON.ANY.R
Lines 14 to 16 in 0134cba
This one is easy-enough to handle, with some
try()
ortryCatch()
logic, I think.PR
I've opened a PR that helps to deal with these cases; all tests pass and I think there's only minimal additional overhead (versus the
structure()
&unclass()
recursive approach). I haven't added tests yet for preventing either the mutation ofx
or erring (whenunclass(x)
fails), but if this PR has any potential future of being merged, I'm obviously happy to write those test cases, too.Side-note, is it possible these two lines are out-of-order?
jsonlite/R/asJSON.ANY.R
Lines 18 to 19 in 0134cba
The text was updated successfully, but these errors were encountered: