feat: REST LRO implementation

[vam-google]

This includes both DIREGAPIC and AIP-151 LRO implementations.

This PR merges changes from diregapic-lro branch to master.

[google-cla]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[codecov]

Codecov Report

Merging #859 (84c04c9) into main (aba0ec0) will decrease coverage by 2.45%.
The diff coverage is 36.46%.

@@            Coverage Diff             @@
##             main     #859      +/-   ##
==========================================
- Coverage   90.03%   87.58%   -2.46%     
==========================================
  Files         151      152       +1     
  Lines       15339    15983     +644     
  Branches     1101     1160      +59     
==========================================
+ Hits        13811    13999     +188     
- Misses       1213     1637     +424     
- Partials      315      347      +32     

Impacted Files	Coverage Δ
...ic/composer/grpc/GrpcServiceStubClassComposer.java	97.76% <ø> (ø)
...rator/gapic/composer/grpcrest/GrpcRestContext.java	0.00% <0.00%> (ø)
...e/api/generator/gapic/model/OperationResponse.java	0.00% <0.00%> (ø)
...common/AbstractServiceClientTestClassComposer.java	86.01% <4.54%> (-3.61%)	⬇️
...omposer/rest/HttpJsonServiceStubClassComposer.java	52.32% <12.26%> (-43.00%)	⬇️
...ic/composer/defaultvalue/DefaultValueComposer.java	82.40% <13.63%> (-5.10%)	⬇️
...poser/common/AbstractServiceStubClassComposer.java	93.75% <38.46%> (-5.53%)	⬇️
...google/api/generator/gapic/protoparser/Parser.java	45.18% <42.85%> (-1.25%)	⬇️
...ser/common/AbstractServiceClientClassComposer.java	98.62% <50.00%> (-0.35%)	⬇️
...mon/AbstractTransportServiceStubClassComposer.java	93.69% <60.00%> (-4.49%)	⬇️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aba0ec0...84c04c9. Read the comment docs.

[chanseokoh]

Looked into the upper part of the diff. Will take a look at the rest later.

[chanseokoh]

Suggested change

	"https://github.com/vam-google/googleapis-discovery/archive/bb8a053b93ef8698297c41634aa9201f7e075277.zip",
	"https://github.com/googleapis/googleapis-discovery/archive/....zip",

[vam-google]

Changed to point to upstream

[chanseokoh]

Is it worth having the consistency in the argument order with others?

Suggested change

	protected abstract List<TypeNode> createClassImplements(TypeStore typeStore, Service service);
	protected abstract List<TypeNode> createClassImplements(Service service, TypeStore typeStore);

[vam-google]

Yes, it is better to have ordering consistent, though there are so much code in gapic-generator in general, so things like arguments ordering is not really a consistent thing accross the generator codebase (meaning other similar methods, not this specific one necessarily). As result it is hard to define what is the "right" ordering. for this specific one it is clearly service should go first, so reordered the parsms, thanks!

[chanseokoh]

We already checked method.hasLro().

Suggested change

	if (method.hasLro() && method.lro().operationServiceStubType() != null) {
	if (lro.operationServiceStubType() != null) {

[vam-google]

yep, that was redundant. Removed

[chanseokoh]

How to refactor this? Into a separate method?

[vam-google]

Honestly, I don't remember anymore =). But I'm pretty sure it is not about language syntax used, it meant it on architectural level. The problem in that piece of code is this whole logic of operationsStubType != null ever being true. This comes from the difference between DIREGAPIC LRO and AIP-151 LROs, and I believe "refactoring" here means removing these kind of differences in logic depending on which LRO is being processed. Proper refactoring of this is outside of scope of this PR I believe.

[chanseokoh]

I saw a lot of times, the codebase uses Arrays.asList(). What would you say?

[vam-google]

I believe all three versions are being used simultaneously:
Arrays.asList()
Collections.singletonList()
ImmutableList.of().

The reason why I don't like Arrays.asList() is that it always gets highlighted by IDEs (especially by IntelliJ) when used with only one argument (and it suggests to use Collections.singletonList() instead). On the other hand, in the rest of our codebase, which depends on guava, we use guava's collections were possible, so using them here as well. I don't have a strong preference, though.

[chanseokoh]

nit

Suggested change

	.setType(opeationsStubType)
	.setType(operationsStubType)

[vam-google]

Thanks, fixed here and in other places (apparently I copy-pased this typo in multiple places in multiple classes =)

[chanseokoh]

@Nullable?

[vam-google]

I guess we don't really use nullable annotation consistently in composers, but added it here.

[chanseokoh]

service.methods().stream().anyMatch(Method::isOperationPollingMethod)? (Haven't tested though.)

[vam-google]

Yeah, pretty idiomatic, changed it to java8 streams form as you suggested. Thanks!

[chanseokoh]

static?

[vam-google]

Honestly, I prefer not using static methods, if not necessary, as they are much less flexible. For private methods it doesn't even matter that much, but even if a method is not using any class state it might in the future or subclasses may be using it, also static methods often cause troubles during testing as they harder to mock, if necessary.

Technically for private static methods it is ok, because they are private anyways and can't be extended (and in case they are promoted to protected/public the static identifier may be removed on the moment of "promotion") and having static on them gives the reader additional information that the method does not mess with the client state, but that seems taking it a bit too far in terms of imposed conventions.

[chanseokoh]

Still working on it.

[chanseokoh]

I think it's confusing in that you first initialize transportOpeationsStubType to something but then both of the if branches overwrite the variable immediately. How about this?

Suggested change

	TypeNode transportOpeationsStubType = service.operationServiceStubType();
	if (transportOpeationsStubType == null) {
	transportOpeationsStubType = getTransportContext().transportOperationsStubTypes().get(0);
	} else {
	transportOpeationsStubType =
	TypeNode transportOpeationsStubType = service.operationServiceStubType();
	if (transportOpeationsStubType == null) {
	return getTransportContext().transportOperationsStubTypes().get(0);
	} else {
	return

But I'm fine either way actually.

[vam-google]

I'm not sure I can read the suggested change right (it abruptly ends with empty return statement). I agree that initializing something and than unavoidably overwriting it may be confusing. Here I think it was done to save a little bit of code, because on the else statement the new value is constructed based on the old value and with current logic we can do transportOpeationsStubType.reference() instead of service.operationServiceStubType().reference() which is one less method call and it is being done twice in the else statement.

[chanseokoh]

Clean this up?

[vam-google]

Yep, cleaned up

[chanseokoh]

static

[vam-google]

Please see the other comment. I'm not aware of static being a good practice/recommended way for things like this (i.e. private helper methods). If there is such, can you please post a link to it?

[chanseokoh]

static

[vam-google]

same as above

[chanseokoh]

static?

[chanseokoh]

static

[chanseokoh]

Just checking: if hasLro and isOperationPollingMethod, it will chain setPollingRequestFactory after setOperationSnapshotFactory. Is that correct?

[vam-google]

Yes, your undestanding is correct. setOperationSnapshotFactory is needed by both methods - the ones which start LRO and the ones which poll for LRO status. setPollingRequestFactory is needed only by polling methods.

[chanseokoh]

Didn't look at golden files. LGTM with some comments I already made.

[vam-google]

@googlebot I fixed it.