Honor json:"-" on embedded fields during initial field walk by chrisjtwomey · Pull Request #113 · Teamwork/kommentaar

chrisjtwomey · 2026-05-18T10:29:06Z

The human is talking in this part:

Hey lads, I don't really know kommentaar that well to delve in myself so I got the robots to do it for me. I have come across what I think is a bug but maybe I'm missing something intentional in kommentaars design (go panic aside). Either way, would be good to get eyes on this and tell me if we've got a genuine fix here.

We came across this in a private repo, specifically in a case like this:

type PaymentMethod struct {
    *stripe.PaymentMethod `json:"-"`
    ...
}

Where that struct is returned in a handler that we annotate for kommentaar

Running kommentaar against our repo with this would result in a panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x44d0a4]

goroutine 1 [running]:
github.com/teamwork/kommentaar/docparse.resolveType(0x15b34fed37a0, {0x5cb293, 0x4}, 0x0, 0x15b35017fa83?, {0x0, 0x0}, {0x15b350507b01?, 0x8000000000?})
	/go/src/github.com/teamwork/kommentaar/docparse/find.go:764 +0x34
github.com/teamwork/kommentaar/docparse.GetReference(0x15b34fed37a0, {0x5cb293, 0x4}, 0x0, {0x15b35017fa80, 0x10}, {0x15b350005180, 0x46})
	/go/src/github.com/teamwork/kommentaar/docparse/find.go:446 +0xdb0
github.com/teamwork/kommentaar/docparse.resolveType(0x15b34fed37a0, {0x5cb293, 0x4}, 0x0, 0x15b350236820?, {0x15b350005180, 0x46}, {0x15b3500b9126?, 0x2?})
	/go/src/github.com/teamwork/kommentaar/docparse/find.go:788 +0x16c
github.com/teamwork/kommentaar/docparse.findNested(0x15b34fed37a0, {0x5cb293, 0x4}, 0x0, 0x564880?, {0x15b350005180, 0x46}, {0x15b3504891c8?, 0x15b350424090?})
	/go/src/github.com/teamwork/kommentaar/docparse/find.go:753 +0x628
github.com/teamwork/kommentaar/docparse.GetReference(0x15b34fed37a0, {0x5cb293, 0x4}, 0x0, {0x15b3501f0099, 0x16}, {0x15b34fec4140, 0x46})
	/go/src/github.com/teamwork/kommentaar/docparse/find.go:526 +0x1390
github.com/teamwork/kommentaar/docparse.parseRefValue(0x15b34fed37a0, {0x5cb293, 0x4}, {0x15b3501f0099, 0x16}, {0x15b34fec4140, 0x46})
	/go/src/github.com/teamwork/kommentaar/docparse/docparse.go:508 +0xb8
github.com/teamwork/kommentaar/docparse.ParseResponse(0x15b34fed37a0, {0x15b34fec4140, 0x46}, {0x15b3501f008b, 0x24})
	/go/src/github.com/teamwork/kommentaar/docparse/docparse.go:435 +0x128
github.com/teamwork/kommentaar/docparse.parseComment(0x15b34fed37a0, {0x15b3501f0000, 0xe0}, {0x0?, 0x0?}, {0x15b34fec4140, 0x46})
	/go/src/github.com/teamwork/kommentaar/docparse/docparse.go:329 +0x768
github.com/teamwork/kommentaar/docparse.FindComments({0x60ba48, 0x15b34fdea068}, 0x15b34fed37a0)
	/go/src/github.com/teamwork/kommentaar/docparse/find.go:50 +0x318
main.start()
	/go/src/github.com/teamwork/kommentaar/main.go:91 +0x4ac
main.main()
	/go/src/github.com/teamwork/kommentaar/main.go:23 +0x48
---exit: 0---

We did a workaround for the moment by stripping all embedded fields with json:"-" before kommentaar runs but its probably better its fixed here instead.

Robot talking:

Summary

GetReference walks struct fields in two passes. The first pass resolves embedded types (so they can be merged later); the second walks nested types for non-embed fields. The second pass already skips fields tagged json:"-", but the first pass does not — so an embed marked json:"-" was still resolved despite being excluded from the schema.

For an embedded qualified pointer type (e.g. *pkg.Type with json:"-"), the first-pass StarExpr branch additionally asserts t.X.(*ast.Ident), which fails silently for *ast.SelectorExpr and passes nil to resolveType — panicking on typ.Obj.

Reproducing the bug

A common Go pattern that hits this: embed a third-party type for method promotion, hide it from JSON output. To reproduce end-to-end:

1. Create a minimal package:

mkdir -p /tmp/repro/contacts
cd /tmp/repro

cat > go.mod <<'GOMOD'
module repro
go 1.23
GOMOD

cat > contacts/contacts.go <<'GO'
package contacts

import "net/mail"

// Contact wraps mail.Address with a display name.
type Contact struct {
    *mail.Address `json:"-"`

    DisplayName string `json:"displayName"`
}

// GET /contacts
//
// Response 200: Contact
func ListContacts() {}
GO

cat > kommentaar.conf <<'CONF'
title Repro
version 1.0
struct-tag json
CONF

2. Run kommentaar against master:

git -C /path/to/kommentaar checkout master
go -C /path/to/kommentaar build -o /tmp/kommentaar-master .

cd /tmp/repro
/tmp/kommentaar-master -config kommentaar.conf -output openapi2-yaml ./contacts

Expected on master:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18]
  docparse.resolveType   find.go:764
  docparse.GetReference  find.go:446
  docparse.parseRefValue docparse.go:508
  ...

3. Run against this branch:

git -C /path/to/kommentaar checkout fix-embed-json-dash
go -C /path/to/kommentaar build -o /tmp/kommentaar-fixed .

cd /tmp/repro
/tmp/kommentaar-fixed -config kommentaar.conf -output openapi2-yaml ./contacts

Expected on this branch (exit 0, valid swagger with the embed correctly excluded):

definitions:
  contacts.Contact:
    title: Contact
    description: Contact wraps mail.Address with a display name.
    type: object
    properties:
      displayName:
        type: string

What changed

docparse/find.go: skip json:"-" embeds in the first field-walk pass, mirroring the existing skip in the nested-walk pass below. This also sidesteps the unsafe *ast.Ident assertion for the most common crash case.
docparse/test.go: add testEmbedJSONDash fixture — *mail.Address \json:"-"`` embedded alongside a regular field.
docparse/docparse_test.go: add TestGetReference_EmbedJSONDash covering the panic regression and asserting mail.Address is not pulled into prog.References.

To just run the new unit test:

go test -run TestGetReference_EmbedJSONDash ./docparse/

Not addressed here

The t.X.(*ast.Ident) assertion in the first pass is still unsafe — embedding a *pkg.Type without json:"-" will still nil-panic. Worth a separate fix; kept out of this PR to stay minimal.

🤖 Generated with Claude Code

The first field-walk pass in GetReference resolved embedded types before checking their struct tag, so an embed marked `json:"-"` was still pulled into the type graph even though it would be omitted from the schema. With a qualified pointer embed (e.g. `*pkg.Type` `json:"-"`) the inner type assertion to `*ast.Ident` also fails silently, passing nil to resolveType and panicking on `typ.Obj`. The nested-walk loop further down already skips `json:"-"`; apply the same check in the first pass so the embed is never resolved and the schema-side filter remains the single source of truth for tag handling.

coveralls · 2026-05-18T10:50:13Z

Coverage Report for CI Build 26028825186

Coverage increased (+0.3%) to 53.695%

Details

Coverage increased (+0.3%) from the base build.
Patch coverage: 2 of 2 lines across 1 file are fully covered (100%).
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	2233
Covered Lines:	1199
Line Coverage:	53.69%
Coverage Strength:	45.49 hits per line

💛 - Coveralls

rafaeljusto · 2026-05-18T11:10:00Z

type PaymentMethod struct {
    *stripe.PaymentMethod `json:"-"`
    ...
}

Interesting, I wasn't aware you could add JSON tags in embedded references 🤔 But it sounds reasonable since you don't have to change the Stripe SDK. Have you tried the // {omitdoc} thing?

https://github.com/Teamwork/kommentaar/blob/master/doc/syntax.markdown#parameter-properties

Like:

type PaymentMethod struct {
    // {omitdoc}
    *stripe.PaymentMethod
    ...
}

chrisjtwomey · 2026-05-18T11:23:17Z

// {omitdoc}

That also did the trick! Still - I think the fix is still valid as atm one could forget the omidoc and kommentaar will panic 🤔

rafaeljusto · 2026-05-18T11:28:09Z

Yep, it's a valid scenario yeah 👍

Embedded struct fields are usually marshaled as if their inner exported fields were fields in the outer struct, subject to the usual Go visibility rules amended as described in the next paragraph. An anonymous struct field with a name given in its JSON tag is treated as having that name, rather than being anonymous. An anonymous struct field of interface type is treated the same as having that type as its name, rather than being anonymous.

https://pkg.go.dev/encoding/json#Marshal

chrisjtwomey added 2 commits May 18, 2026 11:17

Fix linter issue

c34fa19

chrisjtwomey requested a review from rafaeljusto May 18, 2026 11:02

rafaeljusto approved these changes May 18, 2026

View reviewed changes

chrisjtwomey merged commit f4b6904 into master May 18, 2026
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor json:"-" on embedded fields during initial field walk#113

Honor json:"-" on embedded fields during initial field walk#113
chrisjtwomey merged 2 commits into
masterfrom
fix-embed-json-dash

chrisjtwomey commented May 18, 2026 •

edited

Loading

Uh oh!

coveralls commented May 18, 2026

Uh oh!

rafaeljusto commented May 18, 2026

Uh oh!

chrisjtwomey commented May 18, 2026

Uh oh!

rafaeljusto commented May 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

chrisjtwomey commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Reproducing the bug

What changed

Not addressed here

Uh oh!

coveralls commented May 18, 2026

Coverage Report for CI Build 26028825186

Coverage increased (+0.3%) to 53.695%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

rafaeljusto commented May 18, 2026

Uh oh!

chrisjtwomey commented May 18, 2026

Uh oh!

rafaeljusto commented May 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisjtwomey commented May 18, 2026 •

edited

Loading