feat: add interface abstraction primitives #577
Conversation
software-dov
requested review from
miraleung,
vchudnov-g,
codyoss,
noahdietz and
vam-google
google-cla
bot
added
the
cla: yes
|
Is this truly ready for review? Should we make this a draft PR for the time being? |
|
Yes, this is complete module the test failures. The TODOs are caveat notes about subsequent PRs. |
Ok! You can also download the linting report from the failing GitHub Actions. It includes govet, golint and gofmt. |
That is incredibly useful. Wow, I wish I'd known this when doing the python m-gen CI conversion. Thanks! |
|
Would you mind opening a draft PR on gapic-showcase with the client regenerated using your changes? Like googleapis/gapic-showcase#649. This will be useful for reviewing the changes in a "real" client. In gapic-generator-go, run |
|
Done. showcase draft PR is here: https://github.com/googleapis/gapic-showcase/pull/652/files?file-filters%5B%5D=.go |
Nomenclature within this commit: * The current generated client is referred to as a 'transport'. Similarly, the WIP rest+json client will also be a 'transport'. * The new internal-only interface is the 'transport interface'. * The new client type whose only responsibility is to embed a transport interface is a 'new-style client'. Two new types are defined: an interface intended to abstract out the mechanism of network communication and serialization (the transport interface), and a wrapper struct that embeds the interface. The transport interface cannot be user visible because methods will be added to it if the API defines new methods. This is a non-breaking change in the API but is technically a breaking change to the user; if they defined a type that previously satisfied the contract of the interface, the type would no longer satisfy the interface. The workaround is to make the transport interface not user visible and to embed the interface in a struct. The transports and transport interface are generated concurrently, so there is no risk of breaking the interface contract. Adding methods to the new-style client is a non-breaking change becasuse it is a struct. This is similar to the PIMPL idiom in C++. TODO: * Add factory function(s) for the new-style client to take an injected transport + auth. * Tweak documentation, samples, and integration tests to prefer new-style clients instead of directly using transports. * Write out rest+json transport. Includes moderately substantial refactors as implementation details.
software-dov
force-pushed
the
pimpl-interface
branch
from
a67df39 to
07ae859
Compare
There was a problem hiding this comment.
Once the Operation helpers are added to the generated interface, this should be backwards compat and lgtm. I want @tbpg or @codyoss to sign off. If that happens 4/23, i will be OOO and @vchudnov-g is my proxy :)
There was a problem hiding this comment.
This LGTM, thanks @software-dov for all of your hard work on this! I will let someone from your team have final signoff. I think there is still on unresolved comment on the draft PR.
|
@noahdietz has pointed out that changing the name of the default client options thunk will break Spanner: https://github.com/googleapis/google-cloud-go/blob/master/spanner/apiv1/spanner_client_options.go#L24 |
There was a problem hiding this comment.
changing the name of the default client options thunk will break Spanner: https://github.com/googleapis/google-cloud-go/blob/master/spanner/apiv1/spanner_client_options.go#L24
This is a manageable change, though; when we regenerate Spanner, the build will fail, and we will just fix that when it happens. This comment is just a reminder of that eventuality.
Thanks @software-dov I think this is reasonable especially because I agree that the default client option helper(s) should probably be transport-specific.
Nomenclature within this commit:
Similarly, the WIP rest+json client will also be a 'transport'.
transport interface is a 'new-style client'.
Two new types are defined: an interface intended to abstract out the
mechanism of network communication and serialization (the transport interface),
and a wrapper struct that embeds the interface.
The transport interface cannot be user visible because methods will be
added to it if the API defines new methods. This is a non-breaking
change in the API but is technically a breaking change to the user; if
they defined a type that previously satisfied the contract of the
interface, the type would no longer satisfy the interface.
The workaround is to make the transport interface not user visible and
to embed the interface in a struct. The transports and transport
interface are generated concurrently, so there is no risk of breaking
the interface contract. Adding methods to the new-style client is a
non-breaking change becasuse it is a struct.
This is similar to the PIMPL idiom in C++.
TODO:
transport + auth.
new-style clients instead of directly using transports.
Includes moderately substantial refactors as implementation details.