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

Add Error handler from within plumb #507

Closed
wants to merge 4 commits into from

Conversation

schloerke
Copy link
Collaborator

From #506 via @jonkeane

R/plumber.R Outdated
@@ -811,6 +811,10 @@ plumber <- R6Class(
globalSettings = list(info=list()), # Global settings for this API. Primarily used for Swagger docs.

errorHandler = NULL,
setErrorHandlerInternal = function(errorhandler){
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would delete this method as there is already a public setErrorHandler method. (This can be reached using self$setErrorHandler in the evaluateBlock call above.)

Choose a reason for hiding this comment

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

Thanks, for this pointer that's exactly what I was missing when trying to access setErrorHandler earlier.

R/plumber.R Outdated
@@ -220,7 +220,7 @@ plumber <- R6Class(
srcref <- attr(e, "srcref")[[1]][c(1,3)]

evaluateBlock(srcref, private$lines, e, private$envir, private$addEndpointInternal,
private$addFilterInternal, self$mount)
private$addFilterInternal, private$setErrorHandlerInternal, self$mount)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use self$setErrorHandler

Choose a reason for hiding this comment

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

Done

@jonkeane
Copy link

Ok, I've addressed the comments and clarified the error message. Sorry for the rebase I realized I had the wrong email address associated previously.

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@schloerke
Copy link
Collaborator Author

This PR is good.

However, I am hoping to go in a different direction completely. With an #' @plumber tag mixed with a pipeable interface. (both have not been created yet)

I'm going to add it to the next release milestone for tracking, but it should be handled by a different PR.

@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jun 8, 2020
@meztez
Copy link
Collaborator

meztez commented Jun 27, 2020

Done in #568 , can be closed I think.

@schloerke
Copy link
Collaborator Author

#568 did solve this. Thinking more in it... doesn't mean this can't also exist.

Notes for later...

Since this function is a one off function, like a filter, it should be treated like a filter. (Should not be able to add routes and an error handler at the same time.)

The parseBlock content should be changed to not capture to the right of @errorhandler. The content in the test is not used (and should not be used). Within parseBlock, errorhandler <- TRUE if the regex is matched.

@@ -236,7 +250,7 @@ parseBlock <- function(lineNum, file){
#' Evaluate and activate a "block" of code found in a plumber API file.
#' @include images.R
#' @noRd
evaluateBlock <- function(srcref, file, expr, envir, addEndpoint, addFilter, mount) {
evaluateBlock <- function(srcref, file, expr, envir, addEndpoint, addFilter, setErrorHandler, mount) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need anymore since #568.

@@ -273,6 +287,9 @@ evaluateBlock <- function(srcref, file, expr, envir, addEndpoint, addFilter, mou
filter <- PlumberFilter$new(block$filter, expr, envir, block$serializer, srcref)
addFilter(filter)

} else if (!is.null(block$errorhandler)){
setErrorHandler(eval(expr))
Copy link
Collaborator

@meztez meztez Jul 2, 2020

Choose a reason for hiding this comment

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

Since the plumberRouter is now passed to evaluateBlock, use pr$setErrorHandler directly

@@ -220,7 +220,7 @@ plumber <- R6Class(
srcref <- attr(e, "srcref")[[1]][c(1,3)]

evaluateBlock(srcref, private$lines, e, private$envir, private$addEndpointInternal,
private$addFilterInternal, self$mount)
private$addFilterInternal, self$setErrorHandler, self$mount)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need since #568.

@cpsievert
Copy link
Contributor

Thanks for the PR @jonkeane.

Since we can already set a custom error handler via plumber::pr_set_error() (or the $setErrorHandler() method), we're going to pass on this short-hand plumb method since, if we do it here, then we'll need to do it for a bunch of other methods (which doesn't seem worthwhile doing). Here's an example:

handler <- function(req, res, err) {
  rlang::abort(err)
}

#* @plumber 
function(pr) {
  pr %>% pr_set_error(handler)
}

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

Successfully merging this pull request may close these issues.

None yet

5 participants