From 7ec8a68b761bf80084d82f5d937aab903ce6b346 Mon Sep 17 00:00:00 2001 From: Greg Freedman Ellis Date: Thu, 26 Oct 2023 14:56:57 -0500 Subject: [PATCH 1/5] [185973710]: rework error handling so unexpected automation error details get surfaced --- R/api.R | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/R/api.R b/R/api.R index 3b8b23983..cb79bf85b 100644 --- a/R/api.R +++ b/R/api.R @@ -191,22 +191,33 @@ handleAPIfailure <- function(code, response) { return(crGET(response$url)) } msg <- http_status(response)$message + if (code == 409 && grepl("current editor", msg)) { + halt( + "You are not the current editor of this dataset. `unlock()` ", + "it and try again." + ) + } + + msg2 <- NULL if (code == 404) { # Add the URL that was "not found" (there isn't going to be any # useful response content message) msg2 <- response$url } else { - msg2 <- try(content(response)$message, silent = TRUE) + err_content <- try(content(response), silent = TRUE) + if (!is.error(err_content) && is.list(err_content)) { + if (!is.null(err_content$message)) { + msg2 <- err_content$message + } else if (is.shoji.like(err_content)) { + message_desc <- try(err_content$value$message$description, silent = TRUE) + if (!is.error(message_desc)) msg2 <- message_desc + } + } } - if (!is.error(msg2)) { + + if (!is.null(msg2)) { msg <- paste(msg, msg2, sep = ": ") } - if (code == 409 && grepl("current editor", msg)) { - halt( - "You are not the current editor of this dataset. `unlock()` ", - "it and try again." - ) - } halt(msg) } From 300980c86738e26ff35b93bda511564b8107883d Mon Sep 17 00:00:00 2001 From: Greg Freedman Ellis Date: Mon, 30 Oct 2023 13:40:07 -0500 Subject: [PATCH 2/5] [185973710]: fixes for receiving CA scripts --- R/api.R | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/R/api.R b/R/api.R index cb79bf85b..9dd38df48 100644 --- a/R/api.R +++ b/R/api.R @@ -191,13 +191,6 @@ handleAPIfailure <- function(code, response) { return(crGET(response$url)) } msg <- http_status(response)$message - if (code == 409 && grepl("current editor", msg)) { - halt( - "You are not the current editor of this dataset. `unlock()` ", - "it and try again." - ) - } - msg2 <- NULL if (code == 404) { # Add the URL that was "not found" (there isn't going to be any @@ -208,9 +201,12 @@ handleAPIfailure <- function(code, response) { if (!is.error(err_content) && is.list(err_content)) { if (!is.null(err_content$message)) { msg2 <- err_content$message - } else if (is.shoji.like(err_content)) { - message_desc <- try(err_content$value$message$description, silent = TRUE) - if (!is.error(message_desc)) msg2 <- message_desc + } else if ( + !is.null(err_content$element) && + err_content$element == "crunch:error" && + !is.null(err_content$description) + ) { + msg2 <- err_content$description } } } @@ -218,6 +214,14 @@ handleAPIfailure <- function(code, response) { if (!is.null(msg2)) { msg <- paste(msg, msg2, sep = ": ") } + + if (code == 409 && grepl("current editor", msg)) { + halt( + "You are not the current editor of this dataset. `unlock()` ", + "it and try again." + ) + } + halt(msg) } From ec626facb2cba9f7321878b23aec0be6f4640e9b Mon Sep 17 00:00:00 2001 From: Greg Freedman Ellis Date: Mon, 30 Oct 2023 14:22:11 -0500 Subject: [PATCH 3/5] [185973710]: POST/PATCH/PUT auto-detect JSON content type --- R/api.R | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/R/api.R b/R/api.R index 9dd38df48..e0b28c265 100644 --- a/R/api.R +++ b/R/api.R @@ -39,7 +39,7 @@ crunchAPI <- function( #' These methods let you communicate with the Crunch API, for more background #' see [Crunch Internals](https://crunch.io/r/crunch/articles/crunch-internals.html). #' -#' @param ... see [`crunchAPI`] for details. `url` is the first +#' @param url,config,body,... see [`crunchAPI`] for details. `url` is the first #' named argument and is required; `body` is also required for PUT, #' PATCH, and POST. #' @return Depends on the response status of the HTTP request and any custom @@ -50,17 +50,31 @@ crunchAPI <- function( crGET <- function(...) crunchAPI("GET", ...) #' @rdname http-methods #' @export -crPUT <- function(...) crunchAPI("PUT", ...) +crPUT <- function(url, config = list(), ..., body) { + crAutoDetectBodyContentType("PUT", url, config = config, ..., body = body) +} #' @rdname http-methods #' @export -crPATCH <- function(...) crunchAPI("PATCH", ...) +crPATCH <- function(url, config = list(), ..., body) crAutoDetectBodyContentType("PATCH", url, config = config, ..., body = body) #' @rdname http-methods #' @export -crPOST <- function(...) crunchAPI("POST", ...) +crPOST <- function(url, config = list(), ..., body) crAutoDetectBodyContentType("POST", url, config = config, ..., body = body) #' @rdname http-methods #' @export crDELETE <- function(...) crunchAPI("DELETE", ...) +# Helper to auto-detect json class in body to set content type +crAutoDetectBodyContentType <- function(httr.verb, url, config = list(), ..., body) { + if (!missing(body)) { + if (inherits(body, "json")) { + config <- c(add_headers(`Content-Type` = "application/json"), config) + } + crunchAPI(httr.verb, url, config, ..., body = body) + } else { + crunchAPI(httr.verb, url, config, ...) + } +} + #' Do the right thing with the HTTP response #' @param response an httr response object #' @param special.statuses an optional named list of functions by status code. From b3ee24405dffc2326b1637fe5095b16ff18d1883 Mon Sep 17 00:00:00 2001 From: Greg Freedman Ellis Date: Mon, 30 Oct 2023 14:48:26 -0500 Subject: [PATCH 4/5] [185973710]: oops re-roxygen --- man/http-methods.Rd | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/man/http-methods.Rd b/man/http-methods.Rd index 051819da4..e16ca425b 100644 --- a/man/http-methods.Rd +++ b/man/http-methods.Rd @@ -11,16 +11,16 @@ \usage{ crGET(...) -crPUT(...) +crPUT(url, config = list(), ..., body) -crPATCH(...) +crPATCH(url, config = list(), ..., body) -crPOST(...) +crPOST(url, config = list(), ..., body) crDELETE(...) } \arguments{ -\item{...}{see \code{\link{crunchAPI}} for details. \code{url} is the first +\item{url, config, body, ...}{see \code{\link{crunchAPI}} for details. \code{url} is the first named argument and is required; \code{body} is also required for PUT, PATCH, and POST.} } From ed2b0e2104421fadeb2730fd7c4c35a3438f2d10 Mon Sep 17 00:00:00 2001 From: Greg Freedman Ellis Date: Thu, 2 Nov 2023 13:49:50 -0500 Subject: [PATCH 5/5] [185973710]: review feedback --- R/api.R | 26 +++++++++++++++----------- man/http-methods.Rd | 4 ++-- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/R/api.R b/R/api.R index e0b28c265..2c2e7ecbf 100644 --- a/R/api.R +++ b/R/api.R @@ -47,7 +47,7 @@ crunchAPI <- function( #' @importFrom httpcache GET PUT PATCH POST DELETE #' @name http-methods #' @export -crGET <- function(...) crunchAPI("GET", ...) +crGET <- function(url, config = list(), ...) crunchAPI("GET", url, config = config, ...) #' @rdname http-methods #' @export crPUT <- function(url, config = list(), ..., body) { @@ -55,16 +55,21 @@ crPUT <- function(url, config = list(), ..., body) { } #' @rdname http-methods #' @export -crPATCH <- function(url, config = list(), ..., body) crAutoDetectBodyContentType("PATCH", url, config = config, ..., body = body) +crPATCH <- function(url, config = list(), ..., body) { + crAutoDetectBodyContentType("PATCH", url, config = config, ..., body = body) +} #' @rdname http-methods #' @export -crPOST <- function(url, config = list(), ..., body) crAutoDetectBodyContentType("POST", url, config = config, ..., body = body) +crPOST <- function(url, config = list(), ..., body) { + crAutoDetectBodyContentType("POST", url, config = config, ..., body = body) +} #' @rdname http-methods #' @export -crDELETE <- function(...) crunchAPI("DELETE", ...) +crDELETE <- function(url, config = list(), ...) crunchAPI("DELETE", url, config = config, ...) # Helper to auto-detect json class in body to set content type crAutoDetectBodyContentType <- function(httr.verb, url, config = list(), ..., body) { + ignore <- list(...) if (!missing(body)) { if (inherits(body, "json")) { config <- c(add_headers(`Content-Type` = "application/json"), config) @@ -212,14 +217,13 @@ handleAPIfailure <- function(code, response) { msg2 <- response$url } else { err_content <- try(content(response), silent = TRUE) - if (!is.error(err_content) && is.list(err_content)) { - if (!is.null(err_content$message)) { + if (is.list(err_content)) { + # Most API errors have info in message + # But some are starting to wrap in a "crunch:error" with a description (and other keys, + # but we adapt to those on a case-by-case basis, like crunchAutomationErrorHandler) + if (is.character(err_content$message) && length(err_content$message) == 1) { msg2 <- err_content$message - } else if ( - !is.null(err_content$element) && - err_content$element == "crunch:error" && - !is.null(err_content$description) - ) { + } else if (is.character(err_content$description) && length(err_content$description) == 1) { msg2 <- err_content$description } } diff --git a/man/http-methods.Rd b/man/http-methods.Rd index e16ca425b..16727643a 100644 --- a/man/http-methods.Rd +++ b/man/http-methods.Rd @@ -9,7 +9,7 @@ \alias{crDELETE} \title{HTTP methods for communicating with the Crunch API} \usage{ -crGET(...) +crGET(url, config = list(), ...) crPUT(url, config = list(), ..., body) @@ -17,7 +17,7 @@ crPATCH(url, config = list(), ..., body) crPOST(url, config = list(), ..., body) -crDELETE(...) +crDELETE(url, config = list(), ...) } \arguments{ \item{url, config, body, ...}{see \code{\link{crunchAPI}} for details. \code{url} is the first