-
Notifications
You must be signed in to change notification settings - Fork 29
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
base: main
Are you sure you want to change the base?
Drop optional
from PublicKey.raw_bytes
#326
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
{ | ||
"required": [ | ||
"raw_bytes" | ||
] | ||
}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
I don't have a deep insight into the wire format, but I my understanding is that an 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? |
Yeah, I think 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 😇 |
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. |
This needs careful review/consideration from someone who understands the protobuf wire format better than I do 🙂:
optional
?Closes #325.