From 946dc767910aaa47ec27fe590e55cdc041f01819 Mon Sep 17 00:00:00 2001 From: Vincent Guyader Date: Sat, 25 Apr 2026 23:18:19 +0200 Subject: [PATCH 1/3] =?UTF-8?q?fix:=202=20issues=20=E2=80=94=20gl=5Fget=5F?= =?UTF-8?q?group=5Fid=20duplicates=20(#129),=20explicit=20HTTP=20errors=20?= =?UTF-8?q?(#93)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #129 — gl_get_group_id() 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 previous code used path_with_namespace, which only exists on the Projects API. Switch the column references in groups.R. #93 — http_error_or_content() now intercepts any HTTP >= 400 response and surfaces an explicit message: the GitLab API's body 'message' / 'error' field plus a short hint based on the status (401 -> token, 403 -> private/scope, 404 -> not found, 429 -> rate limit). 2xx responses behave exactly as before. Tests: - tests/testthat/test-gl_get_group_id_duplicates.R: testthat with_mocked_bindings on gitlab() simulates duplicates and verifies warn/pick + exact full_path match. - tests/testthat/test-http_error_messages.R: 6 synthesized httr responses (401/403/404/429/500/200) check the produced message and confirm 200 still returns parsed content. --- R/gitlab_api.R | 45 ++++++++++++ R/groups.R | 4 +- dev/SUIVI_ISSUES.md | 43 ++++++++++++ .../test-gl_get_group_id_duplicates.R | 53 +++++++++++++++ tests/testthat/test-http_error_messages.R | 68 +++++++++++++++++++ 5 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 dev/SUIVI_ISSUES.md create mode 100644 tests/testthat/test-gl_get_group_id_duplicates.R create mode 100644 tests/testthat/test-http_error_messages.R diff --git a/R/gitlab_api.R b/R/gitlab_api.R index 21677ea..1495fc8 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..0b323f5 --- /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..e225519 --- /dev/null +++ b/tests/testthat/test-http_error_messages.R @@ -0,0 +1,68 @@ +# 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. + +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 jsonlite::toJSON(body, auto_unbox = TRUE)) + ), + 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) +}) From 1db4510269165854e1b62ef74417f8d2287354ec Mon Sep 17 00:00:00 2001 From: Vincent Guyader Date: Sat, 25 Apr 2026 23:24:54 +0200 Subject: [PATCH 2/3] fix: drop em-dashes + jsonlite usage from new tests R CMD check's --as-cran flagged the new test files: em-dashes (-) made the 'non-ASCII characters in code' NOTE go up; jsonlite::toJSON was used in test fixtures although jsonlite isn't declared in DESCRIPTION (only a transitive dep through httr). - Em-dashes -> regular hyphens. - Tiny inline JSON encoder (handles strings + integers, sufficient for the test fixtures we synthesise) replaces jsonlite::toJSON. --- .../test-gl_get_group_id_duplicates.R | 2 +- tests/testthat/test-http_error_messages.R | 27 ++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/testthat/test-gl_get_group_id_duplicates.R b/tests/testthat/test-gl_get_group_id_duplicates.R index 0b323f5..d37124d 100644 --- a/tests/testthat/test-gl_get_group_id_duplicates.R +++ b/tests/testthat/test-gl_get_group_id_duplicates.R @@ -1,4 +1,4 @@ -# Regression for #129 — gl_get_group_id() previously crashed with +# 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`) diff --git a/tests/testthat/test-http_error_messages.R b/tests/testthat/test-http_error_messages.R index e225519..9f16648 100644 --- a/tests/testthat/test-http_error_messages.R +++ b/tests/testthat/test-http_error_messages.R @@ -1,15 +1,36 @@ # Tests for gitlabr's HTTP error mapping (#93). -# We don't talk to a real GitLab here — instead we synthesize httr +# 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 jsonlite::toJSON(body, auto_unbox = TRUE)) + content = charToRaw(if (is.null(body)) "" else to_json(body)) ), class = "response" ) @@ -55,7 +76,7 @@ test_that("falls back to httr::http_status when the body has no message", { 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 + # 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: ")) }) From 27933c4622a5e0c74473f7b4bc910b8ebb74cdc4 Mon Sep 17 00:00:00 2001 From: Vincent Guyader Date: Sun, 26 Apr 2026 20:07:43 +0200 Subject: [PATCH 3/3] fix(http_error_or_content): drop em-dash from R source (self-review) Self-review: the 403 hint string contained an em-dash, which R CMD check NOTEs as 'non-ASCII characters in code'. Plain ASCII hyphen keeps the error message readable and the source CRAN-clean. --- R/gitlab_api.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/gitlab_api.R b/R/gitlab_api.R index 1495fc8..4b62b7a 100644 --- a/R/gitlab_api.R +++ b/R/gitlab_api.R @@ -247,7 +247,7 @@ http_error_or_content <- function(response, #' #' @param response an `httr` response object. #' @param status integer status code. -#' @return character(1) — explicit, single-line error message. +#' @return character(1) - explicit, single-line error message. #' @noRd gitlab_error_message <- function(response, status = httr::status_code(response)) { body <- tryCatch( @@ -268,7 +268,7 @@ gitlab_error_message <- function(response, status = httr::status_code(response)) 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).", + "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).", ""