feat(client): add option to treat nil options pointers as no options

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 first: the commit reviewed there as commit 2, cherry-picked unchanged onto v3.15.0, with one commit on top answering the review here. The second is !3066. Like the rest of that work, it comes from maintaining gitlab-mcp-server, an MCP server built on this library.

A nil *Options held in an any is not nil, so NewRequestToURL sends the four bytes null as the body of a POST, PUT or PATCH given one, and replaces a query already on the URL for any other method. CancelJob and PublishAllDraftNotes reach it on their own, through their WithOptions siblings. The client option WithNilOptionsOmitted() makes such a pointer mean no options, as an untyped nil does. Nobody can know whether a caller relies on the null, so, as the earlier review asked, the option is off by default and no request changes unless a caller opts in. #2301 tracks making it the default in 4.0, where the option will have no effect, and removing the option in 5.0.

The change
  • client_options.go: WithNilOptionsOmitted(), modelled on WithOnlyIdempotentRetries. Its comment says the behavior is expected to become the default in the next major release, where the option will have no effect, and the option to be removed in the one after.
  • gitlab.go: an unexported omitNilOptions field on Client, and isNonNilOptions, which reports whether opt carries options, that is, whether it is neither nil nor a nil pointer. NewRequestToURL consults it only when the option is set, in the body branch and in the query branch alike, so without the option both branches decide exactly as before.
  • The commit message carries the evidence: the Go FAQ entry on nil values in interfaces, and the two routes the SDK sends null to on its own.

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 51). The server works around it by always passing a non-nil options value to the two WithOptions siblings it calls.

Is this a breaking change?

No. The option is off by default, so every request is what it was before, and the tests pin that: TestPublishAllDraftNotes still asserts the null body. The change adds one exported function, WithNilOptionsOmitted, and two unexported names, the omitNilOptions field and the isNonNilOptions helper, and no service interface changes, so no mock is regenerated.

How was this tested?

Tests and lint

The tests pin both sides of the option:

  • TestNewRequest_NilOptionsPointer_Body: a POST given a nil options pointer sends null by default, and no body with WithNilOptionsOmitted().
  • TestNewRequestToURL_NilOptionsPointer_Query: a GET given one clears a query already on the URL by default, and keeps it with the option.
  • TestNewRequestToURL_NilOptionsOmitted_OtherOptions: with the option, an untyped nil still sends no body, and real options are still sent as the body or as the query.
  • TestJobsService_CancelJob_NilOptionsOmitted and TestPublishAllDraftNotes_NilOptionsOmitted: the two methods that delegate with a nil pointer send no body with the option, and TestJobsService_CancelJobWithOptions now asserts the body it sends when options are given.

On the branch, with Go 1.27.1:

  • go test -race ./... ./config/... in workspace mode, and go test ./... with GOWORK=off, pass.
  • golangci-lint run over both modules with golangci-lint 2.13.2, the version .tool-versions pins, reports 0 issues, and gofumpt -l reports nothing.
  • 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