From 94d6a11587a32655afad90a01a985b3c138e7c51 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 22 Aug 2024 16:33:09 +0100 Subject: [PATCH] Improve handling of non-JSON errors. When getting a non-200 HTTP response, orderly tries to decode the body as JSON in order to extract the error message. We do however occasionally get errors that aren't JSON formatted, in particular if the error is generated by nginx rather than Packit. This situation can arise if the Packit server is down (a 503 error), or if nginx rejects the request due to the body being too large (a 413 error). We can use the content type header of the response to determine whether it is safe to decode the body as JSON. If the response is not of the expected mime-type, the body is ignored and a generic error based on the status code alone is returned. --- R/outpack_http_client.R | 18 +++++++++++------- tests/testthat/helper-outpack-http.R | 5 ++++- tests/testthat/test-outpack-http-client.R | 13 +++++++++++-- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/R/outpack_http_client.R b/R/outpack_http_client.R index d5fe4630..f21ea84d 100644 --- a/R/outpack_http_client.R +++ b/R/outpack_http_client.R @@ -64,13 +64,17 @@ http_client_handle_error <- function(response) { ## for that and then reauthenticate; that requires that a callback ## is passed through here too. code <- httr::status_code(response) - if (code >= 400) { - txt <- httr::content(response, "text", encoding = "UTF-8") - res <- from_json(txt) - ## I am seeing Packit returning an element 'error' not a list of - ## errors - errors <- if ("error" %in% names(res)) list(res$error) else res$errors - stop(http_client_error(errors[[1]]$detail, code, errors)) + if (httr::http_error(code)) { + if (httr::http_type(response) == "application/json") { + txt <- httr::content(response, "text", encoding = "UTF-8") + res <- from_json(txt) + ## I am seeing Packit returning an element 'error' not a list of + ## errors + errors <- if ("error" %in% names(res)) list(res$error) else res$errors + stop(http_client_error(errors[[1]]$detail, code, errors)) + } else { + stop(http_client_error(httr::http_status(code)$message, code, NULL)) + } } response } diff --git a/tests/testthat/helper-outpack-http.R b/tests/testthat/helper-outpack-http.R index 918b50e5..e4f61820 100644 --- a/tests/testthat/helper-outpack-http.R +++ b/tests/testthat/helper-outpack-http.R @@ -2,7 +2,7 @@ mock_headers <- function(...) { structure(list(...), class = c("insensitive", "list")) } -mock_response <- function(content, status = 200, wrap = TRUE, download = NULL) { +mock_response <- function(content, status = 200L, wrap = TRUE, download = NULL) { headers <- mock_headers() if (!is.null(download)) { headers <- mock_headers("content-type" = "application/octet-stream") @@ -15,6 +15,9 @@ mock_response <- function(content, status = 200, wrap = TRUE, download = NULL) { } class(content) <- NULL content <- c(writeBin(content, raw()), as.raw(0L)) + } else if (inherits(content, "character")) { + headers <- mock_headers("content-type" = "text/plain") + content <- c(writeBin(content, raw()), as.raw(0L)) } else { stop("Unhandled mock response type") } diff --git a/tests/testthat/test-outpack-http-client.R b/tests/testthat/test-outpack-http-client.R index a09c256f..d2e5e4de 100644 --- a/tests/testthat/test-outpack-http-client.R +++ b/tests/testthat/test-outpack-http-client.R @@ -53,7 +53,7 @@ test_that("handle errors", { '{"status":"failure",', '"errors":[{"error":"NOT_FOUND","detail":"Resource not found"}],', '"data":null}') - r <- mock_response(json_string(str), status = 404, wrap = FALSE) + r <- mock_response(json_string(str), status = 404L, wrap = FALSE) err <- expect_error(http_client_handle_error(r), "Resource not found") expect_s3_class(err, "outpack_http_client_error") @@ -68,7 +68,7 @@ test_that("handle errors from packit", { '{"status":"failure",', '"error":{"error":"NOT_FOUND","detail":"Resource not found"},', '"data":null}') - r <- mock_response(json_string(str), status = 404, wrap = FALSE) + r <- mock_response(json_string(str), status = 404L, wrap = FALSE) err <- expect_error(http_client_handle_error(r), "Resource not found") expect_s3_class(err, "outpack_http_client_error") @@ -78,6 +78,15 @@ test_that("handle errors from packit", { }) +test_that("handle plain text errors", { + r <- mock_response("foobar", status = 503L, wrap = FALSE) + err <- expect_error(http_client_handle_error(r), + "Server error: \\(503\\) Service Unavailable") + expect_s3_class(err, "outpack_http_client_error") + expect_equal(err$code, 503) +}) + + test_that("can construct sensible download options", { path <- temp_file() res <- http_client_download_options(path)