chore: Pre-DIREGAPIC Refactoring

[vam-google]

Prepare gapic-generator-java for the DIREGAPIC implementation changes.

1. Splitting Grpc-specific classes into their own package (mainly composer classes)
2. Extract the transport-agnostic logic of the composer classes into the set of abstract classes.
3. Convert most of the static methods in the ClassComposer classes into instance methods to leverage the class inheritance (transport-agnostic logic in abstract class, transport-specific portions are in transport-specific classes extending the abstract ones)
4. The resultant architecture resembles the Template Method design pattern where Abstract<Name>ClassComposer.generate() (and potentially the the other private/protected methods called from generate()) call transport-specific logic (implemented in the concrete <Transport><Name>ClassComposer classes), when appropriate.
5. Refactor the tests and golden files file structure to match the main classes package structure.
6. The golden files were split to be placed under the same directory as the clases which generate them. This was matching the existing infrastructure the best: (1) as the golden files updater makes this assumption (golden file is under the same package as the <Name>Test.java which creates it); (2) the repo currently does not have a dedicated test resources directory (there are multiple resource-like subfolders under test, it is hard to keep thing under control and avoid propagating magical path constants); (3) keeping golden files and tests together seems ok, since if the golden files were smaller they would be simply put as the expected constants in the tests themselves.
7. This PR tries to preserve the original history of the files as accurately as possible (i.e. prioritizes clear move semantics over delete/create semantics in terms of file diff, where possible).

The subsequent PR with rest implementation will looks something like this

[codecov]

Codecov Report

Merging #717 (0561d5b) into master (2318136) will increase coverage by 0.00%.
The diff coverage is 96.77%.

@@           Coverage Diff            @@
##           master     #717    +/-   ##
========================================
  Coverage   92.52%   92.52%            
========================================
  Files         124      129     +5     
  Lines       13459    13595   +136     
  Branches      981      983     +2     
========================================
+ Hits        12453    12579   +126     
- Misses        765      773     +8     
- Partials      241      243     +2     

Impacted Files	Coverage Δ
.../google/api/generator/gapic/composer/Composer.java	12.67% <ø> (ø)
...ic/composer/common/BatchingDescriptorComposer.java	99.73% <ø> (ø)
...r/gapic/composer/common/RetrySettingsComposer.java	97.76% <ø> (ø)
...ic/composer/common/ServiceClientClassComposer.java	98.45% <ø> (ø)
...apic/composer/common/ServiceStubClassComposer.java	99.19% <ø> (ø)
.../gapic/composer/grpc/MockServiceClassComposer.java	100.00% <ø> (ø)
...ic/composer/grpc/MockServiceImplClassComposer.java	99.76% <ø> (ø)
...n/AbstractServiceCallableFactoryClassComposer.java	84.31% <91.17%> (ø)
...poser/common/AbstractServiceStubClassComposer.java	97.87% <93.02%> (ø)
.../composer/grpc/ServiceClientTestClassComposer.java	95.14% <95.14%> (ø)
... and 19 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 2318136...0561d5b. Read the comment docs.

[miraleung]

Haven't gotten through a full first pass yet, but here are a couple high-level things I noticed and wanted to get your thoughts on earlier. I'll aim to finish a full review by Friday EOD.

Given the size of this PR, we could iterate on one composer per review cycle, then the resulting changes could be applied to the remaining composers all at once (and reduce the round-trip time for generalizable deltas). WDYT?

[miraleung]

Could we use the Transport enum from the next PR, instead of strings? We could have StubCommentComposer hard-code the appropriate name-casing. Similar for ClassNames and "grpcCallSettings", then we can pass the Transport into the super class instead of these three params.

Alternatively, there could be an ITransportContext interface, with a GrpcTransportContext implementation (for example) that would have getters like getStubCommentComposer(), getCallSettingsClass(), etc. Then we'd need only pass the transport context into the superclass, and each abstract composer wouldn't need to redefine all the getters or transport-specific state. Not sure yet whether ClassNames or the comment composer should be part of that context, whichever feels better implementation-wise is fine.

WDYT?

[vam-google]

There is the Transport enum in the next PR, I just removed all that kind of stuff to make the nex PR complete one (i.e. transport abstraction stuff is introduced there, not partially here). For this specific case (comment composer) it is literally about which string should be shown to the user (because even in the enum it would be called GRPC, not gRPC, so even with enum it is not quite the same; at the same time it is just about posting a comment, so I felt like it did not worth some fancier solution than this).

For the second part of your comment I'm not sure I understand what you mean by transport-specific state redefined in abstract classes. All that state is immutable, it is basically just a config. Transport-specific things are different for each composer (there is some intersection, but not that much). If we move every transport-specific stuff into context it would mean we would have to populate the context with a lot of stuff which makes sense only for one composer (i.e. the context becomes a big pile of things, many of which are not even known on the moment of initialization of the context).

I'm fine with either approach, but would prefer the current one, as it keeps the transport-specific/transport-agnostic business within the composer (does not propagate it into a much more global and shared thing as the Context). I like viewing AbstractClassComposer+GrpcClassComposer+HttpJsonClassComposer as a very close family of classes, basically one class split in 3 to take care of a very specific problem. Introducing context would unnecessarily propagate the transport-specific thing to a bigger context (i.e. will make the change architecturally more different from the original implementation).

[miraleung]

Should we make this a private static final instead, using a new TypeNode with the raw Test.class?
Motivation: There's no guarantee that fixedTypeStore has "Test". Yes, some of these error-prone cases may exist or be unavoidable, so ideally we can eliminate them where possible.

Alternatively, we could create the intersection of the gRPC and REST types in this class.

Ditto for createTypes() in the other composers.

[vam-google]

I can move it back to the public static final but that would mean recreation of TypeNode in a more custom way. I was under impression that there was a standard approach to declaring the set of TypeNode elements in each composer - the creteTypes() or createStaticTypes() (different composers have different name). I don't see big differfence between the testAnnotaion here and any other type created in the createType() methods by the composers.

We also can just remove testAnnotation field completely and access it as getFixedTypeStore("Test") instead everywhere where it is needed? Note, this approach (FIXED_TYPESTORE.get(name) or getFixedTypeStore().get(name)) has already been used heavily in the composers implementation.

There is no guarantee that the "Test" class will exist in fixedTypeStore, but the same is true for everything else in fixedTypeStore (so similarly, it does not feel like there is anything special about "Test" class compared to all the others in fixedTypeStore).

Note, in terms of when the failure can be detected, private static final FIXED_TYPESTORE = createTypes() and private final fixedTypeStore, initialized in the composer's constructor - they are identical, because fixedTypeStore is initialized in the constructor of the singleton classes (all composers are singletons), the INSTANCE is private static final variable in the classes, and as a result is instantiated in the same phase as the private static final FIXED_TYPESTORE. I.e. it is all the same thing as before, just one extra frame in the call stack (fixedTypeStore will be instantiated in the composer's class constructor stack, but still on the class initialization phase, basically right after loading the class in the memory.

[vam-google]

The changes have been merged as multiple separate PRs.