binary: Move Responders and Protocol into package #516
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## streamdev #516 +/- ##
==========================================
Coverage 68.83% 68.83%
==========================================
Files 130 131 +1
Lines 22767 22769 +2
==========================================
+ Hits 15671 15673 +2
Misses 4071 4071
Partials 3025 3025
Continue to review full report at Codecov.
|
witriew
approved these changes
Jul 8, 2021
In #512 it came up that the `*Responder` types in the protocol package belong in the protocol/binary package because they're binary-specific. We found that we cannot move the types into the `binary` package because `EnvelopeAgnosticProtocol.DecodeRequest` returns a `protocol.Responder`, which causes an import cycle between protocol and protocol/binary. This change moves the implementation of the binary protocol, all its methods, and all responder implementations into a binary/protocol.go file, including the DecodeRequest method and exposes the raw Protocol type so that we're not limited by an interface. To get around the import cycle, we redeclare the Responder interface in the binary package, and adapt the type from a binary.Protocol into a protocol.Protocol using an unexported type. Per apidiff, the net effect of these changes is: ``` --- go.uber.org/thriftrw/protocol/binary --- Compatible changes: - Default: added - EnvelopeV0Responder: added - EnvelopeV1Responder: added - NoEnvelopeResponder: added - Protocol: added - Responder: added ``` So nothing changes in the top-level `protocol` package (besides some deprecations) and the `binary` package exports a bunch of new types. Caveat: I would have preferred not to export the `*Responder` types and return a `Responder` struct instead of the interface, but unfortunately, ur hands are tied because of exporting the responder implementations originally. So `binary.Protocol.ReadRequest` has to be able to return one of the three Responder types, which is what necessitates the interface.
Update references made to protocol.Binary to reference binary.Default.
Co-authored-by: Wit Riewrangboonya <wit@uber.com>
|
@usmyth rebased on streamdev and merged. There were enough conflicts that cherry-picking would have been annoying. |
abhinav
added a commit
that referenced
this pull request
Jul 12, 2021
In #516, we tried to move binary-specific concerns into the binary subpackage. Unfortunately, this has the effect of declaring two different `Responder` interfaces: protocol/ envelope.go // type Responder binary/ protocol.go // type Responder Although these two interfaces are equivalent, and any type that implements one implements the other, this introduced a problem: an interface that has a method that *returns* one of these interfaces does not automatically support the other interface. That is, `*binary.Protocol`, which returns a `binary.Responder` does not implement `protocol.EnvelopeAgnosticProtocol`, which returns a `protocol.Responder`. // compilation error! var p protocol.EnvelopeAgnosticProtocol = binary.Default To address this, we introduced an adapter in the `protocol` package that adapts a `*binary.Default` into a `protocol.EnvelopeAgnosticProtocol` by altering the return type of the `DecodeRequest` method. This isn't great, especially because this means that we either need this adapter everywhere, or we need to remember to use `EnvelopeAgnosticBinary` in different places. In particular, it breaks code like the following: var proto protocol.Protocol = binary.Default if p, ok := protocol.(protocol.EnvelopeAgnosticProtocol); ok { p.DecodeRequest(..) } Because `binary.Default` (of type `*binary.Protocol`) does not implement that interface. More importantly, this breaks the following code as well, which previously worked just fine: var proto protocol.Protocol = protocol.Binary if p, ok := protocol.(protocol.EnvelopeAgnosticProtocol); ok { p.DecodeRequest(..) } To address these problems, declare the `Responder` interface in a new subpackage of `protocol`, and alias the `protocol.Responder` to that interface. With this, `*binary.Protocol` now implements `EnvelopeAgnosticProtocol`
abhinav
added a commit
that referenced
this pull request
Jul 13, 2021
In #516, we tried to move binary-specific concerns into the binary subpackage. Unfortunately, this has the effect of declaring two different `Responder` interfaces: protocol/ envelope.go // type Responder binary/ protocol.go // type Responder Although these two interfaces are equivalent, and any type that implements one implements the other, this introduced a problem: an interface that has a method that *returns* one of these interfaces does not automatically support the other interface. That is, `*binary.Protocol`, which returns a `binary.Responder` does not implement `protocol.EnvelopeAgnosticProtocol`, which returns a `protocol.Responder`. // compilation error! var p protocol.EnvelopeAgnosticProtocol = binary.Default To address this, we introduced an adapter in the `protocol` package that adapts a `*binary.Default` into a `protocol.EnvelopeAgnosticProtocol` by altering the return type of the `DecodeRequest` method. This isn't great, especially because this means that we either need this adapter everywhere, or we need to remember to use `EnvelopeAgnosticBinary` in different places. In particular, it breaks code like the following: var proto protocol.Protocol = binary.Default if p, ok := protocol.(protocol.EnvelopeAgnosticProtocol); ok { p.DecodeRequest(..) } Because `binary.Default` (of type `*binary.Protocol`) does not implement that interface. More importantly, this breaks the following code as well, which previously worked just fine: var proto protocol.Protocol = protocol.Binary if p, ok := protocol.(protocol.EnvelopeAgnosticProtocol); ok { p.DecodeRequest(..) } To address these problems, declare the `Responder` interface in a new subpackage of `protocol`, and alias the `protocol.Responder` to that interface. With this, `*binary.Protocol` now implements `EnvelopeAgnosticProtocol`
Merged
abhinav
added a commit
that referenced
this pull request
Aug 30, 2021
# Commits The following comments are included in this release. Some of these cherry-picked and released in v1.28.0, but they appear again in the list above. - protocol: Add streaming interfaces (#485) - Move Stream-based interfaces into their own package - Make Streaming interfaces private to allow for safe experimentation (#488) - idl: Return structured ParseError from idl.Parse() (#492) - Add CHANGELOG entry for #492 (#494) - Support "<" in the templating language (#499) - idl: add a Position struct to wrap reported lines (#497) - Add streamwriter implementation (#490) - Add a "StreamReader" which implements "stream.Reader" - Use the "stream.Reader" in the "binary.Reader" - Add code generation for all wire types for stream encoding (#500) - Generate "Decode" for "enums" that will directly decode (#495) - Provide "decode" code generation for the streaming variants for all other types (#496) - idl: record document positions on constant nodes (#503) - ast: move idl.Position to the ast package (#504) - idl: replace internal.Position with ast.Position (#505) - Expose stream protocol method to close Writer (#506) - idl: add column numbers to parse error positions (#507) - idl: record full positions for constants (#508) - Mark assertParseCases() as a test helper (#509) - protocol/stream: Define enveloping interfaces (#511) - protocol/stream: Declare interface for encoding envelopes (#513) - binary/StreamWriter: Borrow => New; unexport Return (#515) - stream: add Close method, pool binary reader (#514) - binary/reader: Return to pool after ReadValue (#517) - binary/reader: Skip fixed width collections faster (#518) - binary/stream/reader: Fast-path offsetReader skips (#519) - binary: Move Responders and Protocol into package (#516) - benchmark: Refactor into a suite (#520) - Upgrade to Ragel version 6.10 (from 6.9) (#523) - Responder: Deduplicate interface (#524) - gen/quick_test: Add missing types (#525) - enum/json: Support rejecting unknown values (#502) - Back to development - Upgrade to golang.org/x/tools version 0.1.5 (#529) - ast: add column values to the AST nodes (#522) - stream: Implement Request and Response handling with Enveloping (#526) - offsetReader: Implement io.Seeker - binary/ReadRequest: Use io.Seeker if available - StreamReader: Use Seeker instead of offsetReader - protocol/stream: Unembed stream.Protocol from stream.RequestReader (#532) - thrifttest: Add mocks for streaming interfaces (#527) - streaming: Unembed iface.Private in streaming-based interfaces (#533) - Regenerate files for tests after merging `streamdev` - ast: formally declare CppInclude as a Node (#536) - ast: add Annotations(Node) []*Annotations (#537) - Preparing release v1.29.0 # API changes I ran apidiff on all packages in v1.28.0 and compared it with this release. Removing changes to gen/internal/tests, the result is: ``` --- go.uber.org/thriftrw/ast --- Compatible changes: - Annotation.Column: added - Annotations: added - BaseType.Column: added - Constant.Column: added - ConstantList.Column: added - ConstantMap.Column: added - ConstantMapItem.Column: added - ConstantReference.Column: added - CppInclude.Column: added - DefinitionInfo.Column: added - Enum.Column: added - EnumItem.Column: added - Field.Column: added - Function.Column: added - Include.Column: added - ListType.Column: added - MapType.Column: added - Namespace.Column: added - Position.Column: added - Position.String: added - Service.Column: added - ServiceReference.Column: added - SetType.Column: added - Struct.Column: added - TypeReference.Column: added - Typedef.Column: added --- go.uber.org/thriftrw/envelope/stream --- NEW PACKAGE --- go.uber.org/thriftrw/gen --- Compatible changes: - StreamGenerator: added --- go.uber.org/thriftrw/internal/envelope/exception --- Compatible changes: - (*ExceptionType).Decode: added - (*TApplicationException).Decode: added - (*TApplicationException).Encode: added - ExceptionType.Encode: added --- go.uber.org/thriftrw/plugin/api --- Compatible changes: - (*Argument).Decode: added - (*Argument).Encode: added - (*Feature).Decode: added - (*Function).Decode: added - (*Function).Encode: added - (*GenerateServiceRequest).Decode: added - (*GenerateServiceRequest).Encode: added - (*GenerateServiceResponse).Decode: added - (*GenerateServiceResponse).Encode: added - (*HandshakeRequest).Decode: added - (*HandshakeRequest).Encode: added - (*HandshakeResponse).Decode: added - (*HandshakeResponse).Encode: added - (*Module).Decode: added - (*Module).Encode: added - (*ModuleID).Decode: added - (*Plugin_Goodbye_Args).Decode: added - (*Plugin_Goodbye_Args).Encode: added - (*Plugin_Goodbye_Result).Decode: added - (*Plugin_Goodbye_Result).Encode: added - (*Plugin_Handshake_Args).Decode: added - (*Plugin_Handshake_Args).Encode: added - (*Plugin_Handshake_Result).Decode: added - (*Plugin_Handshake_Result).Encode: added - (*Service).Decode: added - (*Service).Encode: added - (*ServiceGenerator_Generate_Args).Decode: added - (*ServiceGenerator_Generate_Args).Encode: added - (*ServiceGenerator_Generate_Result).Decode: added - (*ServiceGenerator_Generate_Result).Encode: added - (*ServiceID).Decode: added - (*SimpleType).Decode: added - (*Type).Decode: added - (*Type).Encode: added - (*TypePair).Decode: added - (*TypePair).Encode: added - (*TypeReference).Decode: added - (*TypeReference).Encode: added - Feature.Encode: added - ModuleID.Encode: added - ServiceID.Encode: added - SimpleType.Encode: added --- go.uber.org/thriftrw/protocol --- Compatible changes: - BinaryStreamer: added --- go.uber.org/thriftrw/protocol/binary --- Compatible changes: - Default: added - EnvelopeV0Responder: added - EnvelopeV1Responder: added - NewStreamReader: added - NewStreamWriter: added - NoEnvelopeResponder: added - Protocol: added - Responder: added - StreamReader: added - StreamWriter: added --- go.uber.org/thriftrw/protocol/envelope --- NEW PACKAGE --- go.uber.org/thriftrw/protocol/stream --- NEW PACKAGE --- go.uber.org/thriftrw/thrifttest/streamtest --- NEW PACKAGE --- go.uber.org/thriftrw/version --- Incompatible changes: - Version: value changed from "1.28.0" to "1.29.0" ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #512 it came up that the
*Respondertypes in the protocol packagebelong in the protocol/binary package because they're binary-specific.
We found that we cannot move the types into the
binarypackage becauseEnvelopeAgnosticProtocol.DecodeRequestreturns aprotocol.Responder,which causes an import cycle between protocol and protocol/binary.
This change moves the implementation of the binary protocol, all its
methods, and all responder implementations into a binary/protocol.go
file, including the DecodeRequest method and exposes the raw Protocol
type so that we're not limited by an interface.
To get around the import cycle, we redeclare the Responder interface in
the binary package, and adapt the type from a binary.Protocol into a
protocol.Protocol using an unexported type.
Per apidiff, the net effect of these changes is:
So nothing changes in the top-level
protocolpackage (besides somedeprecations) and the
binarypackage exports a bunch of new types.Caveat: I would have preferred not to export the
*Respondertypes andreturn a
Responderstruct instead of the interface, but unfortunately,ur hands are tied because of exporting the responder implementations
originally. So
binary.Protocol.ReadRequesthas to be able to returnone of the three Responder types, which is what necessitates the
interface.