fix: Fix BetaApi annotaiton usage for REST transport and clean BetaApi for default stubs in all transports
#987
Conversation
…Api for stubs in all transports
This PR essentially does the following:
1) Cleans `@BetaApi("A restructuring of stub classes is planned, so this may break in the future")` annotaiton for Stub-related methods and Stub classes themselves, because those methods and classes have been in "beta" state like that for several years already and are de-facto GA.
2) Makes sure that all HttpJson (REST) related classes and methods on the surface of the client are marked as beta for `grpc+rest` (mixed transport) clients. This is necessary in the context of the upcoming REGAPIC rollout to indicate that the newly released REST transport funcitonality is released at Beta quality level.
.../goldens/compute/src/com/google/cloud/compute/v1small/stub/HttpJsonRegionOperationsStub.java
Show resolved
Hide resolved
| while (providerBuilderNamesIt.hasNext() && channelProviderClassesIt.hasNext()) { | ||
| List<AnnotationNode> annotations = ImmutableList.of(); |
There was a problem hiding this comment.
If I'm understanding correctly, this part is saying that we don't generate BetaApi annotation for the first getter, but generate it from the second one. I understand we may only have GRPC and HttpJson for now, but it's not easy to read and looks like a hack to me. Can we add the annotation based on something more concrete? Like override this method or part of the method in the subclass?
There was a problem hiding this comment.
This is an already present paradigm in the TransportContext - it has a bunch of transport-specific lists, where first element in transport is a primary one, all others are secondary. For now there are yes, just two transports, but this is a current architecture decision which I don't think are to be changed in scope of this PR. Overriding method is a valid thing but in that case TransportContext should not exist at all in that case (the difference between transports should be done via overriding methods in each transport). I never liked TransportContext as a design decision (and originally it all was done via overriding methods), but i is the current design.
There was a problem hiding this comment.
I just checked how TransportContext is used and I see where you are coming from. Without changing the design, I think there is still something we can do to improve it:
- Extract the hardcoded method names as constants, check if the method name matches HttpJson method names, and add BetaApi annotation accordingly. The current logic depends on the order of hardcoded method names which could be easily messed up with.
- Add some comments to explain this logic. I know it maybe all in your head and looks simple to you, but with more people starting to maintain this repo, it could benefit us tremendously.
There was a problem hiding this comment.
Added a comment.
For the hardcoded method names - changing that is beyond the scope of this PR. There are way too many inline string constants here and there, so changing a few here won't change much. Also to the best of my knowledge, having inline constants is idiomatic an preferred to having separate explicit constants if they remain private and are not reused. I think about it this way: we are generating code here, that code has lots of names, having htem as a bunch of constant scattered all over the place does not seem helping but instead making things harder because it adds another level of indirection when inspecting code. Another example: if "printf" style is used for generation then all of those method are also a bunch of strings. It templates is used, then those method names are again just string in templates.
There was a problem hiding this comment.
I agree we should just used the string literal if it is not reused, I was suggesting more like extracting all the method names and centralize them, then reuse them whenever we need it, and this is a good place for us to reuse it, instead of relying on the order of method names in the list. However, I think this refactoring effort can also be done in a separate PR, it does not have to block this PR.
There was a problem hiding this comment.
Yes, I think it is a totally valid point and would be probably a good architectural decision, but looks like it would make sense only as a separate global refactoring (which will create that centralized naming repoisitory). Before that "repository" is created, we can't do it properly in scope of this PR.
| .build()); | ||
| TypeNode betaApiType = FIXED_TYPESTORE.get("BetaApi"); | ||
|
|
||
| if (annotations.stream().noneMatch(a -> betaApiType.equals(a.type()))) { |
There was a problem hiding this comment.
I think this part makes sense, we are adding the BetaApi annotation to classes if they have beta or alpha in their package name currently, but we want to add it to any HttpJson related class even if the proto says it's not Beta anymore. Can we add a test case for it though? Like removing 'beta' from the package name here.
There was a problem hiding this comment.
Having beta in echo_grpcrest.proto tests this specific block of not applying BetaApi twice. Having the whole new set of golden files and input protos to test the small case of still adding "beta" annotation for beta apis in httpjson case (we already have it tested for grpc case as an integration test for redis/v1beta1 api) seems like not worth it (too many test files to test a very minor thing). Plus, once we have REGAPCI pushed, it will add rest transport to existing integration tests and this case will be tested automatically by integration tests.
There was a problem hiding this comment.
Add a unit test for this method should be pretty straightforward if you think adding a file is not worth it, since both the input and output of this method are just model class created in this repo.
There was a problem hiding this comment.
Composer classes are not tested by testing individual methods in them, instead they are tested by fully generating the file they are responsible for (in this case it would be HttpJson<Service>Stub.java). So to test generation of a stub class under a different package we need: new input proto file (with different package), new .golden file to match output, new method in HttpJsonServiceStubClassComposerTest class to trigger that check. It is a trivial thing to do, bu honestly I don't think it is a right thing to do, because it would bloat our testing infrastructure - we can't make the whole new set of input protos and goldens to test every tiny feature like this one.
There was a problem hiding this comment.
I think you have a very valid point and I completely agree: It's not a right thing to create a new proto and golden file for every tiny feature. So we should change the way we are testing composers, for complicated things that are hard to write unit tests, we can continue to test them using protos/goldens, but for tiny things like this we should probably start doing true unit testing - test the method directly. I have seen a lot of code missing unit tests in this repo and I think we can make it better.
| return EchoStubSettings.defaultGrpcTransportProviderBuilder(); | ||
| } | ||
|
|
||
| /** Returns a builder for the default ChannelProvider for this service. */ |
There was a problem hiding this comment.
Probably not directly related to this PR, can we update the comment for the HttpJson builder to make it more specific? Currently it's the same as the GRPC one above.
| return Builder.createDefault(); | ||
| } | ||
|
|
||
| /** Returns a new builder for this class. */ |
There was a problem hiding this comment.
Same issue here that the comment is exactly the same as the last one.
|
Kudos, SonarCloud Quality Gate passed! |
…aApi` for default stubs in all transports (#987) This PR essentially does the following: 1) Clean `@BetaApi("A restructuring of stub classes is planned, so this may break in the future")` annotaiton for Stub-related methods, because those methods and classes have been in "beta" state like that for several years already and are de-facto GA. 2) Make sure that all HttpJson (REST) related classes and methods on the surface of the client are marked as beta for `grpc+rest` (mixed transport) clients. This is necessary in the context of the upcoming REGAPIC rollout to indicate that soon to be released REST transport functionality is released at Beta quality level.
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.api:gax-bom](https://togithub.com/googleapis/gax-java) | `2.19.2` -> `2.19.4` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>googleapis/gax-java</summary> ### [`v2.19.4`](https://togithub.com/googleapis/gax-java/blob/HEAD/CHANGELOG.md#​2194-httpsgithubcomgoogleapisgax-javacomparev2193v2194-2022-10-19) [Compare Source](https://togithub.com/googleapis/gax-java/compare/v2.19.2...v2.19.4) ##### Bug Fixes - Scope the throttling metric to exclude element size calculation ([#​1835](https://togithub.com/googleapis/gax-java/issues/1835)) ([0287f83](https://togithub.com/googleapis/gax-java/commit/0287f83f9cabbb9dff6cb7e8ea3068ccc1ab12d9)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDAuNSIsInVwZGF0ZWRJblZlciI6IjMyLjI0MC41In0=-->
](https://renovatebot.com){kind=link}
This PR essentially does the following:
@BetaApi("A restructuring of stub classes is planned, so this may break in the future")annotaiton for Stub-related methods, because those methods and classes have been in "beta" state like that for several years already and are de-facto GA.grpc+rest(mixed transport) clients. This is necessary in the context of the upcoming REGAPIC rollout to indicate that soon to be released REST transport functionality is released at Beta quality level.