search

sigstore / protobuf-specs

Protocol Buffer specifications
Apache License 2.0
23 stars 29 forks source link

Drop `optional` from `PublicKey.raw_bytes` #326

Open woodruffw opened 4 months ago

woodruffw commented 4 months ago

This needs careful review/consideration from someone who understands the protobuf wire format better than I do 🙂:

  1. Are there any wire format consequences to removing this optional?
  2. If there are, do they matter given that Sigstore primarily uses the JSON envelope?

Closes #325.

kommendorkapten commented 4 months ago

I don't have a deep insight into the wire format, but I my understanding is that an optional field is is treated as a oneof, so removing it will have some effects. This can be verified by looking the generated Go code, and it's annotations:

Old: (with optional)

RawBytes []byte `protobuf:"bytes,1,opt,name=raw_bytes,json=rawBytes,proto3,oneof" json:"raw_bytes,omitempty"`

New: (without optional)

RawBytes []byte `protobuf:"bytes,1,opt,name=raw_bytes,json=rawBytes,proto3" json:"raw_bytes,omitempty"`

That said, I don't know if that affects the binary format. It may?

woodruffw commented 4 months ago

That said, I don't know if that affects the binary format. It may?

Yeah, I think oneof means a wire format change 😞

So the follow-on question: do we care 😅? Everything (?) that consumes these specs uses the JSON encoding, which isn't affected by this, so in practice it isn't a breaking change.

kommendorkapten commented 4 months ago

So the follow-on question: do we care 😅? Everything (?) that consumes these specs uses the JSON encoding, which isn't affected by this, so in practice it isn't a breaking change.

I'm with you (that we don't care). The client spec should state the the canonical format is JSON, so no clients are expected to ever deal with the binary format.

But I would still like to verify it that the language bindings for JSON does not mess this up. I don't think they should, but here I would rather test twice 😇

woodruffw commented 4 months ago

But I would still like to verify it that the language bindings for JSON does not mess this up. I don't think they should, but here I would rather test twice 😇

Makes sense! I can look into some test assets to include in this repo -- my thinking is that each language client could have a small test suite to ensure that bundles (of various versions) get loaded correctly.