Skip to content

fix(clientgen): avoid shared JSON mapping collisions - #247

Merged
SebastienMelki merged 3 commits into
mainfrom
fix/go-client-json-mapping-ownership
Aug 6, 2026
Merged

fix(clientgen): avoid shared JSON mapping collisions#247
SebastienMelki merged 3 commits into
mainfrom
fix/go-client-json-mapping-ownership

Conversation

@SebastienMelki

Copy link
Copy Markdown
Owner

Summary

  • stop protoc-gen-go-client from emitting package-level JSON mapping files/methods owned by protoc-gen-go-http
  • keep clientgen enum annotation validation and client runtime checks for go-http-provided custom unmarshaler methods
  • add a combined go-http + go-client regression test for duplicate JSON mapping output
  • update docs and consistency tests for the new ownership boundary

Fixes #246

Verification

  • go test ./internal/clientgen ./internal/httpgen -count=1
  • make lint
  • ./scripts/run_tests.sh --fast

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

🔍 CI Pipeline Status

Lint: success
Test: success
Coverage: success
Build: success
Integration: success


📊 Coverage Report: Available in checks above
🔗 Artifacts: Test results and coverage reports uploaded

@SebastienMelki SebastienMelki left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Clean ownership fix — go-http is now the sole emitter of package-level JSON mapping files, eliminating duplicate symbol errors when both Go plugins target the same package. The client dispatches through sebufUnmarshaler/json.Marshaler interfaces at runtime, so it picks up custom marshalers when present and falls back to protojson otherwise.

What I checked

  • Deleted files (10 source + 15 golden) are genuinely duplicates of httpgen equivalents — no client-specific logic lost.
  • enum_validation.go correctly preserves the fail-fast annotation conflict check without emitting code.
  • generator.go:52-59 early return for service-less files is well-commented and correct.
  • marshalRequest / unmarshalResponse dispatch logic unchanged — interface-based forwarding to go-http-owned methods works as before.
  • Golden test expectations updated consistently — no references to deleted golden files remain.
  • Consistency tests in httpgen properly removed the TestGoGeneratorsProduceIdentical* tests that compared go-http vs go-client output (now moot), while keeping TS + OpenAPI cross-generator checks intact.
  • Combined generation test (TestCombinedGoHTTPAndGoClientGenerationDoesNotDuplicateJSONMappingFiles) is a solid regression guard.

Minor suggestions (non-blocking)

  1. Combined test coverage: The test only exercises int64_deep_nested_encoding.proto. Adding a second fixture (e.g. flatten.proto or enum_encoding.proto) would catch regressions across more annotation families.

  2. Client-only usage discoverability: If someone uses go-client without go-http, mapping annotations silently have no effect. The doc update in client-generation.md mentions this but it's easy to miss in a long sentence. A short callout box or generation-time warning could save users debugging time.

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.56%. Comparing base (9e14c24) to head (b21866f).

Files with missing lines Patch % Lines
internal/clientgen/enum_validation.go 0.00% 21 Missing ⚠️
internal/httpgen/oneof_discriminator.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
+ Coverage   28.88%   34.56%   +5.68%     
==========================================
  Files          75       67       -8     
  Lines       13157    10994    -2163     
==========================================
  Hits         3800     3800              
+ Misses       9160     6997    -2163     
  Partials      197      197              
Flag Coverage Δ
unittests 34.56% <0.00%> (+5.68%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SebastienMelki

Copy link
Copy Markdown
Owner Author

Examples review follow-up pushed in be53144.

What changed:

  • reviewed all example buf.gen.yaml files and regenerated every example with buf dep update && buf generate
  • refreshed market-data-unwrap generated output/docs so go-http-owned unwrap marshalers are used alongside go-client
  • clarified market-data-unwrap README: go-http owns custom unwrap marshalers; go-client consumes them when generated in the same package
  • fixed restful-crud Makefile install to include protoc-gen-go-client
  • fixed simple-api regeneration: clean now removes api/docs, and buf.gen.yaml enables mock generation because main.go uses NewMockUserServiceServer
  • fixed a oneof discriminator generator bug surfaced by examples: emitted fmt.Errorf strings now use %w instead of %%w; added regression coverage and updated golden
  • tidied example modules after regeneration

Verification:

  • all examples: buf dep update && buf generate
  • make lint
  • ./scripts/run_tests.sh --fast
  • go test ./... in every examples/* module with go.mod

@SebastienMelki

Copy link
Copy Markdown
Owner Author

LGTM. Clean ownership separation — all 9 JSON mapping generators removed from clientgen with no stale references, enum validation correctly retained, combined generation test verifies no duplicate files, and consistency tests properly updated. Docs reflect the new boundary.

@SebastienMelki
SebastienMelki merged commit d3bcc2a into main Aug 6, 2026
10 checks passed
@SebastienMelki
SebastienMelki deleted the fix/go-client-json-mapping-ownership branch August 6, 2026 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

codegen: protoc-gen-go-http and protoc-gen-go-client both emit *_encoding.pb.go, breaking generation when both plugins run

1 participant