Skip to content

feat(csharp): add contract model and C# generator#131

Open
elzalem wants to merge 19 commits intomainfrom
split/csharp-generator
Open

feat(csharp): add contract model and C# generator#131
elzalem wants to merge 19 commits intomainfrom
split/csharp-generator

Conversation

@elzalem
Copy link
Collaborator

@elzalem elzalem commented Mar 2, 2026

Summary

This PR extracts the shared contract traversal and C# generator work from #126.

Included here:

  • add internal/contractmodel for package/message/enum/service traversal
  • add protoc-gen-csharp-http
  • add C# golden coverage

Why

The new non-TypeScript generators need a shared contract representation. Keeping that shared model together with the first concrete consumer, the C# generator, makes this PR coherent without introducing a plumbing-only PR.

What Changed

  • add cmd/protoc-gen-csharp-http
  • add internal/contractmodel
  • add internal/csharpgen

Tests

  • env GOTOOLCHAIN=auto go test ./internal/csharpgen
  • env GOTOOLCHAIN=auto go test ./cmd/protoc-gen-csharp-http

@github-actions
Copy link

github-actions bot commented Mar 2, 2026

🔍 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

@codecov
Copy link

codecov bot commented Mar 2, 2026

Codecov Report

❌ Patch coverage is 66.29274% with 631 lines in your changes missing coverage. Please review.
✅ Project coverage is 15.39%. Comparing base (917521f) to head (a1b8a29).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/csharpgen/generator.go 63.15% 515 Missing and 48 partials ⚠️
internal/contractmodel/model.go 79.76% 61 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            main     #131       +/-   ##
==========================================
+ Coverage   3.17%   15.39%   +12.21%     
==========================================
  Files         47       49        +2     
  Lines       7485     9567     +2082     
==========================================
+ Hits         238     1473     +1235     
- Misses      7243     8035      +792     
- Partials       4       59       +55     
Flag Coverage Δ
unittests 15.39% <66.29%> (+12.21%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Owner

@SebastienMelki SebastienMelki left a comment

Choose a reason for hiding this comment

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

Code Review — PR #131

@elzalem thanks for the contribution! The shared contractmodel abstraction and C# generator are a solid start. I've gone through the code in detail and have feedback across several areas.


internal/contractmodel/model.go

1. Nested message naming uses __ separator — leaks into generated output

walkMessageSymbols joins nested message names with __ (e.g., Item__Item_Details). This produces ugly class/type names in every consumer (C#: Item__Item_Details, Python: Item__Item_Details). In C# specifically, nested classes (Item.Details) or flattened names (ItemDetails) would be far more idiomatic. Since this is the shared model, the naming strategy here cascades to all downstream generators — it should be thought through carefully.

2. Limited well-known type handling

Only google.protobuf.Struct and google.protobuf.Timestamp are handled. Missing: Any, Duration, Value, ListValue, Empty, FieldMask, wrappers (StringValue, Int32Value, etc.). These are commonly used in real protos and will produce incorrect output or object/Any fallbacks silently.

3. No handling for optional fields or oneof

The Field struct has no concept of optionality (proto3 optional keyword) or oneof grouping. These are semantically important — optional produces hasField semantics, and oneof means mutually exclusive fields. Both affect generated code shape.

4. Package exposes Files []*protogen.File

This leaks protogen types into the abstraction layer. If the goal of contractmodel is to provide a clean, generator-agnostic model, it should not expose raw protogen types. Consumers should work purely through the model's own types.

5. No unit tests for contractmodel

This package is the foundation for all new generators but has zero unit tests of its own. It's only tested indirectly via golden tests that exercise one simple proto. Functions like Packages(), buildSymbols(), resolveType(), walkMessageSymbols() need direct unit tests with targeted edge cases (deeply nested messages, map<K, Enum>, cross-file imports, etc.).

6. sebuf annotation support missing

The existing generators support unwrap, int64_encoding, enum_encoding, nullable, empty_behavior, timestamp_format, bytes_encoding, oneof_config, flatten, etc. The contract model doesn't carry any of this annotation metadata, so consumers can't generate code that respects these annotations. This is a significant gap for a shared model in this project.


internal/csharpgen/generator.go

7. pascalCase() doesn't properly handle ALL_CAPS proto names

pascalCase("STATE_UNSPECIFIED") produces STATEUNSPECIFIED (each segment keeps its original casing after the first letter). The golden file confirms this. The expected C# convention is StateUnspecified. Fix: lowercase each segment before capitalizing the first letter.

8. Enums generated as static class with string constants

C# has proper enum types. Generating public static class with public const string is non-idiomatic and loses type safety. C# developers expect real enums (or at least smart enums). The current approach makes it impossible to use enums in switch statements, get compiler exhaustiveness checks, etc.

9. No handling of sebuf annotations

The generator doesn't read or act on any of the project's custom annotations (int64_encoding, enum_encoding, nullable, timestamp_format, bytes_encoding, oneof_config, flatten, etc.). For a generator in this project, these are core requirements — without them the generated C# code won't correctly serialize/deserialize messages that use these annotations.

10. ServiceContracts is barely useful

The generated ServiceContracts class only emits service name constants. No methods, no route info, no HTTP verb/path configuration, no request/response type associations. For an HTTP client generator (protoc-gen-csharp-http), this should at minimum include HTTP method + path + types per RPC.


internal/csharpgen/golden_test.go

11. Single minimal test proto

The test proto (contracts.proto) only covers: basic scalar fields, repeated string, google.protobuf.Struct, nested enums, nested messages, one service with one RPC. Missing coverage:

  • optional fields
  • oneof fields
  • Maps with non-string keys, enum values, message values
  • Multiple services
  • Well-known types (Timestamp, Duration, Any, etc.)
  • Deeply nested messages (3+ levels)
  • Cross-file imports
  • sebuf annotations (unwrap, int64_encoding, etc.)
  • Empty messages
  • Streaming RPCs (should be skipped or handled gracefully)

12. diffStrings() is duplicated

This helper is copy-pasted between csharpgen and pyclientgen test files. Should be in a shared test utility.


Missing Requirements

Beyond the code issues above, this PR needs several additions before it's ready to merge:

  1. Documentation: Both top-level README/CLAUDE.md updates and generator-specific documentation explaining the C# generator's capabilities, options, and limitations.

  2. Examples: An example in the examples/ directory that demonstrates all supported use cases — covering the various proto features and annotations the generator handles (similar to examples/ts-client-demo/ or examples/ts-fullstack-demo/).

  3. Extensive testing: The current single golden test with one basic proto is insufficient. We need:

    • Unit tests for contractmodel (edge cases for type resolution, nested message naming, map handling, well-known types)
    • Unit tests for csharpgen functions (pascalCase, csharpType, csharpScalar, jsonAttribute)
    • Multiple golden test protos covering all supported type combinations and edge cases
    • Tests for both Newtonsoft and System.Text.Json output paths
  4. Makefile integration: The new plugin should be discovered by the Makefile automatically (it likely already is via cmd/ auto-discovery, but verify).

@elzalem elzalem requested a review from SebastienMelki March 3, 2026 15:53
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.

2 participants