aperturerobotics / protobuf-go-lite

Reflection-free Protobuf for Go.
BSD 3-Clause "New" or "Revised" License
22 stars 2 forks source link

Feature request: Allow generation of messages with `optional` fields #21

Closed gburek-fastly closed 3 weeks ago

gburek-fastly commented 1 month ago

I am working on generating https://github.com/open-telemetry/opentelemetry-proto-go definitions that work in a tinygo runtime.

I have been able to successfully use protobuf-go-lite to generate reflection-less MarshalProtoJSON and UnmarshalProtoJSON as seen in https://github.com/open-telemetry/opentelemetry-proto-go/compare/main...gburek-fastly:opentelemetry-proto-go:gburek/lite-optional-bug.

However, the generated code does not build:

opentelemetry-proto-go/lite ‹main➔› » go build ./...
# go.opentelemetry.io/proto/lite/otlp/metrics/v1
otlp/metrics/v1/metrics.pb.go:2027:12: element.MarshalProtoJSON undefined (type *HistogramDataPoint has no field or method MarshalProtoJSON)
otlp/metrics/v1/metrics.pb.go:2065:7: v.UnmarshalProtoJSON undefined (type *HistogramDataPoint has no field or method UnmarshalProtoJSON)
otlp/metrics/v1/metrics.pb.go:2098:12: element.MarshalProtoJSON undefined (type *ExponentialHistogramDataPoint has no field or method MarshalProtoJSON)
otlp/metrics/v1/metrics.pb.go:2136:7: v.UnmarshalProtoJSON undefined (type *ExponentialHistogramDataPoint has no field or method UnmarshalProtoJSON)

It appears that these messages (defined upstream in https://github.com/open-telemetry/opentelemetry-proto) have optional fields, min, max and sum:

It looks like protobuf-go-lite skips generation of these messages. Was there something about messages with optional fields that protoc-gen-go-json gets weird about?

What it would it look like to allow code generation for objects with optional fields? Could it be a feature flag?

paralin commented 1 month ago

@gburek-fastly that was mainly a stopgap to avoid the extra complexity of messages with both optional and not-optional fields. If you would like to take a look at generating those fields, I would be happy to merge a PR to add support!