fix(issues): avoid panic unmarshalling an Issue with no id

José M. Requena Plensrequested to merge
jmrp/client-go:fix/issue-unmarshal-nil-id into main

What does this MR do?

Issue.UnmarshalJSON inspects the raw id value with reflection:

if reflect.TypeOf(raw["id"]).Kind() == reflect.String {

When raw["id"] is a nil any, reflect.TypeOf returns a nil *rtype and calling Kind() on it panics with a nil pointer dereference. That happens for four inputs:

  • an object with no id key
  • an explicit {"id": null}
  • an empty object {}
  • a null document (which leaves raw a nil map)

A panic inside an UnmarshalJSON implementation is not recoverable by the caller in any reasonable way — it unwinds through encoding/json and Client.Do and takes the process down, where an error return would have been handled normally.

This replaces the reflection with the comma-ok type assertion the sibling unmarshallers in this package already use — MergeRequest.UnmarshalJSON reads raw["labels"].([]any) and Label.UnmarshalJSON reads raw["title"].(string) — which is nil-safe by construction and expresses the same intent more directly:

if _, ok := raw["id"].(string); ok {

Issue.UnmarshalJSON was the only hand-written unmarshaller in the module using the reflection form; the one other non-generated reflect.TypeOf call (request_handler.go) compares types rather than calling a method on the result, so it is unaffected.

Is this a breaking change?

No. Behaviour is identical for every payload the API actually returns: a numeric id is still preserved, and a string id is still moved to external_id. The only path that changes is the one that previously panicked. Targeting main rather than release-client-3.0 for that reason.

How was this tested?

Added TestIssueUnmarshalJSON, a table-driven test covering the two existing behaviours (numeric id preserved, string id moved to external_id) plus the four inputs above. Verified it fails against the current code and passes with the fix:

--- FAIL: TestIssueUnmarshalJSON/explicit_null_id
    Error: func (assert.PanicTestFunc)(...) should not panic
    Panic value: runtime error: invalid memory address or nil pointer dereference

Full package suite passes, and golangci-lint run ./... reports the same five pre-existing findings before and after the change (none in the touched files).

How was it found

Surfaced while building resource subscriptions in gitlab-mcp-server, an MCP server built on this client. A test fed a synthetic {} body through every resource handler to verify URI routing, and GetIssue panicked rather than returning an error. Real GitLab responses always carry id, so this is a robustness fix rather than a report of a production outage — but it does make the library unsafe to point at any mock, fixture or proxy that returns a minimal body.

Merge request reports