Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop optional from PublicKey.raw_bytes #326

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

woodruffw
Copy link
Member

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.

Signed-off-by: William Woodruff <[email protected]>
Comment on lines -158 to -162
{
"required": [
"raw_bytes"
]
},
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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.

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.

Drop optional from common.PublicKey.raw_bytes
3 participants