Read RFC 7807 title and detail through their pointers in FormatErrorMessage - #182
Read RFC 7807 title and detail through their pointers in FormatErrorMessage#182midwell wants to merge 1 commit into
Conversation
…essage
The generated models declare the RFC 7807 members as `*string` with `omitempty`,
and `FormatErrorMessage` formatted the field's `Interface()` with `%s`. On a
pointer that renders the address, so the text the function exists to surface is
replaced by it in `GenericOpenAPIError.RawError` — which is what callers log and
what the SBI error paths propagate.
Both states were wrong, not just the populated one:
403 Forbidden %!s(*string=0x14000123456) (%!s(*string=0x14000123460))
403 Forbidden %!s(*string=<nil>) (%!s(*string=<nil>))
The second is the more common: every member is `omitempty`, so a model carrying
only a cause has both nil, and the old code printed the nils rather than nothing.
The check it used, `field != (reflect.Value{})`, also cannot distinguish a field
that is absent from one that is present and unset — only the first of those three
states was handled.
`problemDetailsString` follows a single level of pointer indirection and returns
the empty string for all three "nothing to say" states: no such field, a nil
field, or an empty one. The message is then assembled from the members that have
something in them, so an unset detail no longer contributes an empty `()`.
Non-pointer `string` members keep their existing output. The one deliberate
behaviour change is that a member with nothing in it is now omitted instead of
rendered, which is the point of the fix.
It also stops two panics on the unconditional `Elem()`: an untyped nil ("on zero
Value") and a model passed by value ("on struct Value"). Neither is reachable
from the generated API methods, which all pass `&v`, so this is hygiene for an
exported reflective helper rather than a live defect — and it costs nothing,
because reading through a pointer member needs the `Kind` checks in any case. A
typed nil pointer was already safe and still is.
No test is added here. This package's `*_test.go` names are the generator's own
output slots, so a hand-written test in it is silently overwritten rather than
merely deleted; assertions for this behaviour belong with the consumer. The fix
was nonetheless mutation-verified before the test was dropped: against the
previous implementation the pointer case reports the address in the message,
five of seven empty-state cases fail, and an untyped nil and a by-value model
both panic.
Note for whoever regenerates: the buggy body is verbatim stock output from the
generator's `go/client.mustache`, so a regeneration reinstates it unless that
template is fixed too. This file has already diverged from stock in six ways -
exported `RawBody`/`RawError`/`RawModel` instead of unexported fields, `any` for
`interface{}`, an exported `FormatErrorMessage`, and `AsGenericOpenAPIError` and
the generic `ErrorModel[T]`, neither of which exists in the template at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GyNr6vp6JaxxzPyVcuHXTf
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
50e3988 to
b3458da
Compare
|
Closing this in favour of a fix to the template it is generated from.
Fixed at the source instead: OpenAPITools/openapi-generator#24853, which also closes their No test was added here either way: |
The defect
The generated models declare the RFC 7807 members as
*stringwithomitempty, andFormatErrorMessageformatted the field'sInterface()with%s. On a pointer that renders theaddress, so the text the function exists to surface is replaced by it in
GenericOpenAPIError.RawError— which is what callers log and what the SBI error paths propagate.Both states were wrong, not only the populated one:
The second is the more common of the two. Every member is
omitempty, so a model carrying only acause has both nil, and the old code printed the nils rather than nothing. The check it used,
field != (reflect.Value{}), also cannot tell a field that is absent from one that is presentand unset — of the three states a member can be in, only one was handled.
Observed rather than inferred: the first line above is from a live SMF rejection reaching an AMF,
which logged
S-NSSAI[sst: 1, sd: %!s(*string=0xc00103d2c0)].The fix
problemDetailsStringfollows a single level of pointer indirection and returns the empty stringfor all three "nothing to say" states — no such field, a nil field, an empty one. The message is
then assembled from the members that have something in them, so an unset detail no longer
contributes an empty
().Non-pointer
stringmembers keep their existing output. The one deliberate behaviour change isthat a member with nothing in it is omitted rather than rendered, which is the point of the fix.
It also removes two panics on the unconditional
Elem(): an untyped nil ("on zero Value") and amodel passed by value ("on struct Value"). Neither is reachable from the generated API methods —
all 4633 call sites pass
&v— so that part is hygiene for an exported reflective helper rather than alive defect, and it costs nothing, because reading through a pointer member needs the
Kindchecksin any case. A typed nil pointer was already safe and still is.
Deliberately not done: returning only the status. That removes the symptom by discarding the
detail, which is the information the function exists to carry.
No test in this package
Deliberately none, following the review of #176:
*_test.gonames here are the generator's ownoutput slots, so a hand-written test in this package is silently overwritten by a regeneration
rather than merely deleted. Assertions for this behaviour belong with the consumer.
The fix was mutation-verified before that test was dropped, against the previous implementation:
*stringmembersNote for whoever regenerates
The buggy body is verbatim stock output from the generator's
go/client.mustache(checkedagainst 7.24.0), so a regeneration reinstates it unless that template is fixed as well. The
upstream template carries the same two faults for every Go client it generates.
This file has already diverged from stock in six ways, so it is in practice hand-maintained:
exported
RawBody/RawError/RawModelin place of the unexportedbody/error/model,anyfor
interface{}, an exportedFormatErrorMessage, andAsGenericOpenAPIErrorplus the genericErrorModel[T], neither of which exists in the template at all.Verification
go test -race ./... -count=1,staticcheckv0.8.1,golangci-lintv2.13.2 (in Docker, with therepo config),
gciv0.14.0,gofmt,gofumptandreuse lintare all clean. The commit wasextracted into a clean tree with
git archiveand rebuilt and re-checked there, not only in theworking tree.
VERSIONdrops-devto 2.2.3, following #176, so the fix is consumable by the network functionson merge rather than waiting for a separate release PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01GyNr6vp6JaxxzPyVcuHXTf