Add stable codec interface and implementations #460
Conversation
jchadwick-buf
force-pushed
the
stable-codec
branch
from
d431aa2 to
e1b604a
Compare
| // If this function returns false, the data returned from Marshal and | ||
| // MarshalStable are considered valid text and may be used in contexts | ||
| // where text is expected. | ||
| IsBinary() bool |
There was a problem hiding this comment.
Codecs shouldn't need to expose anything about their format.
There was a problem hiding this comment.
Importantly, for Connect GET support, we need to know whether or not the URL data is/needs to be base64 encoded or not. The codec itself could handle this without exposing this detail, but that breaks transparency: there would be no way for a middlebox to know for sure how to translate a Connect GET request to an ordinary Connect Unary request or gRPC request. Unless we change the Connect GET design, this bit does need to escape the codec one way or another.
There was a problem hiding this comment.
Agreed with John here. To make Envoy and other middleboxes work, the runtime needs to know whether the encoded data is text or binary.
There was a problem hiding this comment.
I think unit tests for the codecs are appropriate now. I leaned toward end-to-end tests during connect's development b/c they were more robust to refactoring and gave me a better feel for the user experience.
Can you add a few tests? Otherwise this looks great.
|
Added tests and unexported the interface. The tests are a bit anemic at this point in time, if anyone has any ideas for how they could be expanded with more edge cases I'd like to. I'm working on stuff a bit out of order at the moment. |
| &pingv1.PingRequest{Text: "text", Number: 1}, | ||
| []byte(`{"number":"1","text":"text"}`), | ||
| ) | ||
| } |
There was a problem hiding this comment.
A few suggestions to improve test coverage:
- I always like
testing/quickto do these sorts of round-trip testing. There are a handful of examples in this repository. - Rather than directly asserting the wire format, can we create a protobuf message with a
map[string]stringand assert that repeatedly marshaling it always produces the same output?
There was a problem hiding this comment.
It was a little tricky but I think I have made tests that work how you wanted.
|
Changes were confined to |
Unfortunately, protobuf offers no story whatsoever for canonicalization of protobuf or protojson output. It seems the best we can get is to just make it deterministic within each implementation.
Related issues: