feat(csharp): add contract model and C# generator#131
Conversation
🔍 CI Pipeline Status✅ Lint: success 📊 Coverage Report: Available in checks above |
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SebastienMelki
left a comment
There was a problem hiding this comment.
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:
optionalfieldsoneoffields- 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:
-
Documentation: Both top-level README/CLAUDE.md updates and generator-specific documentation explaining the C# generator's capabilities, options, and limitations.
-
Examples: An example in the
examples/directory that demonstrates all supported use cases — covering the various proto features and annotations the generator handles (similar toexamples/ts-client-demo/orexamples/ts-fullstack-demo/). -
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
csharpgenfunctions (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
- Unit tests for
-
Makefile integration: The new plugin should be discovered by the Makefile automatically (it likely already is via cmd/ auto-discovery, but verify).
Summary
This PR extracts the shared contract traversal and C# generator work from #126.
Included here:
internal/contractmodelfor package/message/enum/service traversalprotoc-gen-csharp-httpWhy
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
cmd/protoc-gen-csharp-httpinternal/contractmodelinternal/csharpgenTests
env GOTOOLCHAIN=auto go test ./internal/csharpgenenv GOTOOLCHAIN=auto go test ./cmd/protoc-gen-csharp-http