fix: return the top-level GraphQL errors of eight security mutations

What does this MR do?

The review of !3063, the joint merge request that came out of #2300, asked me to split two of its changes into merge requests of their own, and this is the second, now with the integration tests the review asked for. The first is !3065. Like the rest of that work, it comes from maintaining gitlab-mcp-server, an MCP server built on this library.

When GitLab refuses one of the five security attribute mutations or the three security category mutations, it answers with HTTP 200, a top-level errors array and a null payload, and the methods only read the payload's own errors field. So a refused mutation reached the caller as a success from five of them (DestroySecurityAttribute, DestroySecurityCategory and BulkUpdateSecurityAttributes with a nil error, CreateSecurityAttributes with an empty slice, ProjectUpdateSecurityAttribute with zero counts), and as a bare ErrNotFound, without GitLab's message, from the other three (UpdateSecurityAttribute, CreateSecurityCategory and UpdateSecurityCategory). All eight now return a *GraphQLResponseError carrying GitLab's errors, as the work item and saved view methods already do.

The two commits
  1. fix: return the top-level GraphQL errors of eight security mutations. The commit reviewed in !3063 as its commit 4, cherry-picked unchanged onto v3.15.0. Each method checks the top-level errors before the payload and returns them in a *GraphQLResponseError. Each of the eight resolves what it names through authorized_find!, which raises ResourceNotAvailable, a GraphQL::ExecutionError, when the object does not exist or the caller may not act on it. authorize_resource.rb
  2. test: add integration tests for the eight security mutations. New, from the review. Two lifecycle tests drive all eight mutations against a live instance, and eight refusals, one for each of the eight, check that GitLab's error comes back as a *GraphQLResponseError; the testing section below has the detail. utils_test.go gains SkipIfNotUltimate, since security attributes are an Ultimate feature and SkipIfNotLicensed also admits Premium, and CreateTestSecurityCategory.

gitlab-mcp-server keeps the record of this defect, and of everything else it finds and contributes in the projects it depends on and in sibling projects, in upstream-bugs.md (entry 19). The server works around it by sending the same eight mutations itself and reading the top-level errors, and moves onto these methods once they do.

Is this a breaking change?

No signature changes. What changes is the error a refused mutation returns: five methods that returned success now return an error, and UpdateSecurityAttribute, CreateSecurityCategory and UpdateSecurityCategory return a *GraphQLResponseError where they returned ErrNotFound, so a caller matching ErrNotFound on a refusal from those three sees a different error. They still return ErrNotFound when GitLab answers with no error and no object.

How was this tested?

Unit tests, integration tests and lint

Unit tests. Each of the eight methods gains a _graphQLErrors test whose handler answers HTTP 200 with a top-level error and a null payload, the shape GitLab refuses with, and asserts a *GraphQLResponseError carrying GitLab's message.

Integration tests, in gitlab_test/:

  • TestSecurityCategoryLifeCycle creates, updates and destroys a category.
  • TestSecurityAttributeLifeCycle creates two attributes in a category, updates one, adds both to a project and removes one, applies one in bulk, destroys one, and finally applies the destroyed attribute in bulk, which GitLab refuses.
  • TestSecurityAttributeMutationsOnAttributeThatDoesNotExist destroys and updates an attribute that does not exist, and TestDestroySecurityCategoryThatDoesNotExist destroys a category that does not exist.
  • TestSecurityCategoryMutationsOnNamespaceThatDoesNotExist creates and updates a category in a namespace that does not exist, and TestSecurityAttributeMutationsOnNamespaceOrProjectThatDoesNotExist creates attributes in a namespace that does not exist and adds an attribute to a project that does not exist.

Each refusal asserts that the method returns a *GraphQLResponseError whose GitLab message says the resource does not exist or may not be acted on, that the error's text carries that message, and that the response was HTTP 200. The two lifecycle tests need an Ultimate license and skip elsewhere; SkipIfNotUltimate reads the license on every call rather than through the cache SkipIfNotLicensed keeps, which the two, running in parallel, would read and write without synchronization. The seven refusals of an ID that does not exist need no license: each of the seven mutations they call resolves the attribute, category, namespace or project it names through authorized_find! first, as categories/update.rb and attributes/project_update.rb show, so an ID that resolves to nothing is refused before anything a license decides is checked. The GitLab behavior the lifecycle relies on (soft deletion, the refusal of a destroyed attribute in bulk) was read from the 19.4.1 source, and the second commit's message links it.

Run against a live instance on 2026-09-30: GitLab EE 19.4.1-ee (revision 26212baacad, the gitlab/gitlab-ee:19.4.1-ee.0 image) in Docker, with an administrator's personal access token of api scope, from the branch with Go 1.27.1:

GITLAB_BASE_URL=http://127.0.0.1:18095/api/v4 GITLAB_TOKEN_TEST=<token> \
  go test -race -tags=integration -count=1 -v -run '^(TestSecurity|TestDestroySecurityCategory)' ./gitlab_test/
  • Without a license, the seven refusals of an ID that does not exist pass and the two lifecycle tests skip.
  • With an Ultimate license installed on the same instance, all six tests pass, the lifecycle tests included.
  • On v3.15.0 with only the second commit, the fix left out, against the same Ultimate instance, every refusal fails: the five methods that returned success give a nil error, the three that returned ErrNotFound give it (404 Not Found) rather than a *GraphQLResponseError, and the attribute lifecycle fails at its last step, the bulk update of the destroyed attribute. The category lifecycle, which has no refusal, passes.

The project's tests:integration job does not run them yet, nor any other test in gitlab_test/: it exports GITLAB_TOKEN, while SetupIntegrationClient reads GITLAB_TOKEN_TEST, so in the latest scheduled main pipeline every test there skipped with GITLAB_TOKEN_TEST environment variable not set (job 16821904079). !3067 (merged) proposes the one-line fix.

Lint and the rest, on the branch with Go 1.27.1:

  • go test -race ./... ./config/... in workspace mode, and go test ./... with GOWORK=off, pass, and go vet -tags=integration ./... ./config/... is clean.
  • golangci-lint run over both modules with golangci-lint 2.13.2, the version .tool-versions pins, reports 0 issues. Its configuration sets no build tags, so that run does not load the integration files; gofumpt -l reports nothing, gitlab_test included. Run with --build-tags=integration, golangci-lint reports nothing in the new code but usetesting on the context.Background() of its two cleanups, which is the pattern the existing integration tests use, since t.Context() is already cancelled when a cleanup runs.
  • No .proto file and no service interface changes, so make generate has nothing to regenerate.

Related to #2300

Edited by José M. Requena Plens

Merge request reports

Loading
Loading