fix: 2 gitlabr issues — gl_get_group_id duplicates (#129), explicit HTTP errors (#93)

R/gitlab_api.R

@@ -227,13 +227,58 @@ 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)
     list(ct = ct, nxt = nxt)
   }
 }
 
+#' 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) {

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"
     ))

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. |

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"
+      )
+    }
+  )
+})

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)
+})