diff --git a/R/gitlab_api.R b/R/gitlab_api.R index 21677ea..4b62b7a 100644 --- a/R/gitlab_api.R +++ b/R/gitlab_api.R @@ -227,6 +227,11 @@ gitlab <- function(req, http_error_or_content <- function(response, handle = httr::stop_for_status, ...) { + status <- httr::status_code(response) + if (status >= 400L) { + msg <- gitlab_error_message(response, status) + stop(msg, call. = FALSE) + } if (!identical(handle(response), FALSE)) { ct <- httr::content(response, ...) nxt <- get_next_link(httr::headers(response)$link) @@ -234,6 +239,46 @@ http_error_or_content <- function(response, } } +#' Build a readable error message from a failed GitLab API response (#93). +#' +#' GitLab's REST API returns a JSON body of the form +#' `{"message": "401 Unauthorized"}` (or `error: ...`). Surface that text +#' so the caller sees *what* went wrong rather than a generic 401 / 404. +#' +#' @param response an `httr` response object. +#' @param status integer status code. +#' @return character(1) - explicit, single-line error message. +#' @noRd +gitlab_error_message <- function(response, status = httr::status_code(response)) { + body <- tryCatch( + httr::content(response, as = "parsed", encoding = "UTF-8"), + error = function(e) NULL + ) + api_msg <- NULL + if (is.list(body)) { + api_msg <- body$message %||% body$error %||% NULL + } + if (is.list(api_msg)) { + api_msg <- paste(unlist(api_msg, use.names = FALSE), collapse = " ") + } + if (is.null(api_msg) || !nzchar(api_msg)) { + api_msg <- httr::http_status(response)$message + } + + hint <- switch( + as.character(status), + "401" = " (check that your private token is set and still valid).", + "403" = " (your token does not have access to this resource - private repository or insufficient scope).", + "404" = " (the resource does not exist or your token cannot see it).", + "429" = " (GitLab rate limit reached; retry after the Retry-After header).", + "" + ) + + sprintf("GitLab API error %s: %s%s", status, api_msg, hint) +} + +`%||%` <- function(a, b) if (is.null(a)) b else a + #' @importFrom stringr str_replace_all str_split #' @noRd get_rel <- function(links) { diff --git a/R/groups.R b/R/groups.R index 151ce23..754e473 100644 --- a/R/groups.R +++ b/R/groups.R @@ -100,8 +100,8 @@ gl_get_group_id <- function(group_name, ...) { c( "Multiple groups with given name or path found,", "please use explicit name with namespace:", - matching$path_with_namespace, - paste("Picking", matching[1, "path_with_namespace"], "as default") + matching$full_path, + paste("Picking", matching[1, "full_path"], "as default") ), collapse = "\n" )) diff --git a/dev/SUIVI_ISSUES.md b/dev/SUIVI_ISSUES.md new file mode 100644 index 0000000..5031a4e --- /dev/null +++ b/dev/SUIVI_ISSUES.md @@ -0,0 +1,43 @@ +# Suivi — passe `fix/multiple-issues` + +## Issues traitées + +### #129 — `gl_get_group_id()` plante quand le groupname est dupliqué +La fonction comparait/affichait `path_with_namespace`, qui est une +colonne de l'API **Projects**, pas Groups. L'API Groups expose +`full_path`. Quand `gitlab(req = "groups", ...)` retournait plusieurs +résultats avec le même `name`, l'accès à `matching[1, "path_with_namespace"]` +levait *Column `path_with_namespace` doesn't exist*. + +- **Fix** : `path_with_namespace` -> `full_path` dans `R/groups.R` + (filtrage, message d'avertissement, picking par défaut). +- **Test** : `tests/testthat/test-gl_get_group_id_duplicates.R` + (`testthat::with_mocked_bindings()` simule `gitlab()` qui retourne + plusieurs `outillage-data` ; vérifie que la fonction warn et + retourne le premier id, et que matcher exactement par full_path + cible la bonne ligne sans warning). + +### #93 — Messages d'erreur HTTP explicites +`http_error_or_content()` déléguait à `httr::stop_for_status()` qui +sort un message générique style `Forbidden (HTTP 403)`. L'utilisateur +ne savait pas si c'était un token invalide, un dépôt inexistant ou un +dépôt privé. + +- **Fix** : court-circuit avant `stop_for_status` pour `status >= 400`. + Helper interne `gitlab_error_message()` qui : + - extrait le `message` ou `error` du corps JSON renvoyé par GitLab, + - ajoute un *hint* selon le code (401 -> token, 403 -> private/scope, + 404 -> not found, 429 -> rate limit). +- **Test** : `tests/testthat/test-http_error_messages.R` — 6 cas + (401/403/404/429/500/200) avec des `httr::response` synthétisées. + +## Issues envisagées mais non traitées + +| # | Pourquoi pas | +|---|---| +| #127 | "Install all dependencies by default" — décision de design (changement de défaut). | +| #126 | usethis pr-helper equivalents — feature large. | +| #125 | latest tag/release — feature, demande shape API. | +| #124 | junit reporter — feature, parser custom. | +| #76, #71, #69, #58, #51, #43, #33, #26, #23, #16 | features ou design, pas des bugs ; à arbitrer avec le mainteneur. | +| #42 | doc seule. | diff --git a/tests/testthat/test-gl_get_group_id_duplicates.R b/tests/testthat/test-gl_get_group_id_duplicates.R new file mode 100644 index 0000000..d37124d --- /dev/null +++ b/tests/testthat/test-gl_get_group_id_duplicates.R @@ -0,0 +1,53 @@ +# Regression for #129 - gl_get_group_id() previously crashed with +# "Column `path_with_namespace` doesn't exist" when the GitLab Groups +# API returned multiple groups sharing the same name. The Groups API +# exposes `full_path` (the Projects API exposes `path_with_namespace`) +# so the code referenced the wrong column. + +test_that("gl_get_group_id() picks the unique full_path match (#129)", { + fake_groups <- tibble::tibble( + id = c(10L, 20L, 30L), + name = c("outillage-data", "outillage-data", "infra"), + path = c("outillage-data", "outillage-data", "infra"), + full_path = c( + "platform/outillage-data", + "team-x/outillage-data", + "team-x/infra" + ) + ) + + testthat::with_mocked_bindings( + gitlab = function(req, ...) fake_groups, + .package = "gitlabr", + { + # Ambiguous name -> warning + first full_path picked. + expect_warning( + id <- gl_get_group_id("outillage-data"), + regexp = "Multiple groups" + ) + expect_equal(id, 10L) + + # Exact full_path -> picks the matching row, no warning. + id2 <- expect_no_warning( + gl_get_group_id("team-x/outillage-data") + ) + expect_equal(id2, 20L) + } + ) +}) + +test_that("gl_get_group_id() errors when no group matches (regression)", { + testthat::with_mocked_bindings( + gitlab = function(req, ...) tibble::tibble( + id = integer(), name = character(), + path = character(), full_path = character() + ), + .package = "gitlabr", + { + expect_error( + gl_get_group_id("never-exists"), + regexp = "no matching" + ) + } + ) +}) diff --git a/tests/testthat/test-http_error_messages.R b/tests/testthat/test-http_error_messages.R new file mode 100644 index 0000000..9f16648 --- /dev/null +++ b/tests/testthat/test-http_error_messages.R @@ -0,0 +1,89 @@ +# Tests for gitlabr's HTTP error mapping (#93). +# We don't talk to a real GitLab here - instead we synthesize httr +# response objects with the right status code and JSON body and let the +# helper turn them into human-readable messages. + +to_json <- function(x) { + # Tiny manual JSON encoder for the test fixtures: avoids depending on + # jsonlite (a transitive dep through httr, not in gitlabr's DESCRIPTION). + # Handles named lists of scalar string / numeric values, sufficient + # for the responses we synthesise here. + if (length(x) == 0L) return("{}") + encode_val <- function(v) { + if (is.numeric(v)) { + formatC(v, format = "d") + } else { + sprintf('"%s"', gsub('"', '\\\\"', as.character(v))) + } + } + pairs <- vapply( + names(x), + function(k) sprintf('"%s":%s', k, encode_val(x[[k]])), + character(1L) + ) + paste0("{", paste(pairs, collapse = ","), "}") +} + +fake_response <- function(status, body = NULL, content_type = "application/json") { + structure( + list( + url = "https://gitlab.example/api/v4/test", + status_code = as.integer(status), + headers = structure(list(`content-type` = content_type), class = "insensitive"), + content = charToRaw(if (is.null(body)) "" else to_json(body)) + ), + class = "response" + ) +} + +test_that("401 -> token hint (#93)", { + resp <- fake_response(401L, list(message = "401 Unauthorized")) + msg <- expect_error( + gitlabr:::http_error_or_content(resp), + regexp = "401" + ) + err <- conditionMessage(msg) + expect_match(err, "401 Unauthorized", fixed = TRUE) + expect_match(err, "private token", fixed = TRUE) +}) + +test_that("404 -> resource-not-found hint (#93)", { + resp <- fake_response(404L, list(message = "404 Project Not Found")) + err <- tryCatch(gitlabr:::http_error_or_content(resp), + error = function(e) conditionMessage(e)) + expect_match(err, "404 Project Not Found", fixed = TRUE) + expect_match(err, "does not exist", fixed = TRUE) +}) + +test_that("403 -> private/scope hint (#93)", { + resp <- fake_response(403L, list(message = "403 Forbidden")) + err <- tryCatch(gitlabr:::http_error_or_content(resp), + error = function(e) conditionMessage(e)) + expect_match(err, "403 Forbidden", fixed = TRUE) + expect_match(err, "private repository", fixed = TRUE) +}) + +test_that("429 -> rate-limit hint (#93)", { + resp <- fake_response(429L, list(message = "Too many requests")) + err <- tryCatch(gitlabr:::http_error_or_content(resp), + error = function(e) conditionMessage(e)) + expect_match(err, "429", fixed = TRUE) + expect_match(err, "rate limit", fixed = TRUE) +}) + +test_that("falls back to httr::http_status when the body has no message", { + resp <- fake_response(500L, list()) + err <- tryCatch(gitlabr:::http_error_or_content(resp), + error = function(e) conditionMessage(e)) + expect_match(err, "500", fixed = TRUE) + # We don't pin the exact wording - just that there is *some* message + # past the status code. + expect_gt(nchar(err), nchar("GitLab API error 500: ")) +}) + +test_that("2xx response still returns parsed content (regression)", { + resp <- fake_response(200L, list(id = 42L, name = "ok")) + out <- gitlabr:::http_error_or_content(resp) + expect_type(out, "list") + expect_equal(out$ct$id, 42L) +})