Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix: REST: Make make LRO stub accept APIs of different versions #1622

Merged
merged 3 commits into from
Feb 9, 2022

Conversation

vam-google
Copy link
Contributor

Prior to this fix it worked only with v1 APIs. HttpJsonOperationsStub is shared by multiple different services similarly to GrpcOperationsStub. Unlike grpc version, the verison of the actual API that this stub is used by is supposed to be encoded in the http url, so the HttpJsonOperationsStub must adjust to the actual version of the API it is being used from. Similarly we also need to make the url patterns less restrictive, to accept operaion names in different formats (this is what caused the {name=} pattern changes).

The solution is hacky, but simple and should work for all known APIs. It is based on the fact that even though the HttpJsonOperationsStub is shared by all clients (i.e. is in gax), its HttpJsonStubCallbackFactory argument, on the other hand, is a generated one (i.e. is in a gapic client for a specific API) and its package corresponds to the API version, which is a convention for all google APIs (i.e. version in the proto package corresponds to the version of the API).

Doing this the other way would require a lot of work (parsing yaml, generating operations stub on the fly etc), and we may come back to it if even needed (most probably never).

Prior to this fix it worked only with `v1` APIs. `HttpJsonOperationsStub` is shared by multiple different services similarly to `GrpcOperationsStub`. Unlike grpc version, the verison of the actual API that this stub is used by is supposed to be encoded in the http url, so the `HttpJsonOperationsStub` must adjust to the actual version of the API it is being used from. Similarly we also need to make the url patterns less restrictive, to accept operaion names in different formats (this is what caused the `{name=}` pattern changes).

The solution is hacky, but simple and should work for all known APIs. It is based on the fact that even though the `HttpJsonOperationsStub` is shared by all clients (i.e. is in `gax`), its `HttpJsonStubCallbackFactory` argument, on the other hand, is a generated one (i.e. is in a gapic client for a specific API) and its package corresponds to the API version, which is a convention for all google APIs (i.e. version in the proto package corresponds to the version of the API).

Doing this the other way would require a lot of work (parsing yaml, generating operations stub on the fly etc), and we may come back to it if even needed (most probably never).
@vam-google vam-google requested review from a team as code owners February 9, 2022 04:43
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hacky indeed, but I'll take your word for that the other way is worse.
Please take a look at the potential security issue with the regex reported by SonarCloud. Anything to be concerned about here?

@vam-google
Copy link
Contributor Author

@meltsufin The codesmells and security warnings are all basically about usage of regexps (reading descriptions seems like they will be triggered for any code using the regexps no matter what). Here they are definitely not a concern, because the set of potential strings to parse by the regexp is very limited and restricted, since they are always a package name of a gapic-generated class, so any craziness in that package name is restricted and validated by java compiler before even getting to the regexp part.

@meltsufin
Copy link
Member

That's what I thought. Thanks for the clarification. You can mark that issue resolved with the explanation that you provided above so that it goes away.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 9, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

55.6% 55.6% Coverage
0.0% 0.0% Duplication

@vam-google vam-google merged commit 3ae8d85 into googleapis:main Feb 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants