Barbara/error hiding #1156
Barbara/error hiding #1156
Conversation
…y.sanitize.errors = FALSE) to override this behavior)
|
The default behavior is to sanitize, even when running Shiny apps locally? That feels like going overboard. I can understand defaulting to sanitizing in Shiny Server. |
|
How attached are you to the function name |
| error = function(cond) { | ||
| # Don't print shiny.silent.error (i.e. validation errors) | ||
| if (inherits(cond, "shiny.silent.error")) return() | ||
| if (isTRUE(getOption("show.error.messages"))) { |
There was a problem hiding this comment.
What happens if show.error.messages is FALSE?
There was a problem hiding this comment.
I inherited this check from the shiny.R code (before I added to it): if (isTRUE(getOption("show.error.messages"))) printError(cond). show.error.messages is a general R error handling option: from my understanding, if it is FALSE, no errors (strictly speaking, nothing produced by stop) is printed. I don't think it's really important whether we keep this or not because I don't think this is a widely used option. It doesn't hurt though, and keeps things consistent.
|
…ode. Fixed downloadHandler's bug of not responding to safeError.
|
Update:
|
| cond <- structure( | ||
| list(message = error), | ||
| class = c("shiny.custom.error", errorClass, "error", "condition") | ||
| ) |
There was a problem hiding this comment.
I was actually picturing more like this (not tested):
if (inherits(error, "character")) {
error <- simpleError(error)
}
if (!inherits(error, "error")) {
stop("The class of the `error` parameter must be either 'error' or 'character'")
}
if (!("shiny.custom.error" %in% class(error))
class(error) <- c("shiny.custom.error", class(error))
errorI'd like to use simpleError to convert the error string to an error obj, as it relies less on the internal implementation details of errors. And for the case where error is an error obj already, we can leave it alone as much as possible and just add the one class name.
There was a problem hiding this comment.
Ok, that makes sense. I've changed to that, with a small difference: I don't think this check is necessary: if (!("shiny.custom.error" %in% class(error)). error will never have the class shiny.custom.error a priori -- we'll always have to add it (right?) Also, I added the user-provided errorClass to class(error).
|
Looks great other than my one last comment! |
| stop("The class of the `error` parameter must be either 'error' or 'character'") | ||
| } | ||
| return(cond) | ||
| class(error) <- c("shiny.custom.error", errorClass, class(error)) |
There was a problem hiding this comment.
I didn't notice errorClass before--what's the intended usage of this?
There was a problem hiding this comment.
Cause I think it might make more sense to just say, if you want to provide a custom error class, just give us a custom error object.
There was a problem hiding this comment.
Hmmm... Initially, I just took errorClass from stopWithCondition that has an argument just like that. I then thought it was worth keeping around to allow app authors to style their apps however they want (yes, we do give them the "shiny.custom.error" class, but maybe one class is not enough). I doubt this will be used a lot, but it seemed unobtrusive enough that it was worth keeping it around to give app authors more control over their errors.
|
Final updates:
|
A PR to address this issue. I think the coding for this is more or less finished. Planning on testing it a bit more, but aside from that, this should be good to merge soon.
An overview:
By default, all errors are now sanitized automatically. This means that most error messages will be replaced with a generic error message: "An error has occurred. Check your logs or contact the app author for clarification." We can disable this feature and go back to the way things were up until now by adding
options(shiny.sanitize.errors = FALSE). However, if we want to sanitize most errors but there is one or two particular errors that we think would be really useful for the user to see (and which don't leak any sensitive information), then we can use the newcustomStop()function instead of the regularstop()function (even if we keep the default ofoptions(shiny.sanitize.errors = TRUE)).Possible issues
downloadHandler()andui.R. The aesthetics are still very different though -- these two open a new window to display the error, rather than producing an in-place, red error message. This can be something for us to prettify at some point in the future...runtime: shinyRmd documents. However, for that case, we're still not completely safe because sensitive information may still leak through to the end user via warnings and/or messages. This is something we'll need to think about. [The silver lining here is that if a warning or message occurs inside of areactiveor arenderXXXfunction, they do not show up in the document, only in the R Markdown console.]