Drop optional from PublicKey.raw_bytes

[woodruffw]

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.

[woodruffw]

Flagging: I don't fully understand this codegen change -- raw_bytes was originally marked explicitly with optional, so there absolutely should never have been a required variant to begin with.

[haydentherapper]

That is strange, what happens if you generate jsonschema from scratch before this change? Is required still present? Just wondering if this is stale or something.

[woodruffw]

Hmm, if I re-generate before this change the required comes back. My only guess here is that because it's wrapped within a oneOf, it has no effect (since it's still optional under the other oneOf variants).

[kommendorkapten]

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]

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]

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]

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.