feat: add DIREGAPIC support for PHP #3305

bshaffer · 2020-11-10T22:31:11Z

Resubmission of #3301 from origin so all tests pass

Adds supportsGrpcTransport, which returns false when transport_protocol is set to REST
Modifies PHP GAPIC to:
- ensure documentation for transport and transportConfig service clients options omit grpc
- omit gcpApiConfigPath config in getClientDefaults
- omit importing any grpc-related classes
- Adds defaultTransport and getSupportedTransport functions to override the transport verification functionality in google/gax
Adds new baseline test for DIREGAPIC PHP
Updates PHP bazel rules to allow for REST transport

codecov · 2020-11-10T22:44:35Z

Codecov Report

Merging #3305 (3886997) into master (994ffa2) will increase coverage by 0.00%.
The diff coverage is 90.47%.

@@            Coverage Diff            @@
##             master    #3305   +/-   ##
=========================================
  Coverage     87.13%   87.14%           
- Complexity     6115     6120    +5     
=========================================
  Files           495      495           
  Lines         24159    24172   +13     
  Branches       2634     2637    +3     
=========================================
+ Hits          21051    21064   +13     
  Misses         2242     2242           
  Partials        866      866

Impacted Files	Coverage Δ	Complexity Δ
...sformer/csharp/CSharpGapicUnitTestTransformer.java	`96.89% <ø> (ø)`	`21.00 <0.00> (ø)`
...n/transformer/java/JavaSurfaceTestTransformer.java	`90.84% <ø> (ø)`	`34.00 <0.00> (ø)`
...gle/api/codegen/viewmodel/DynamicLangXApiView.java	`77.77% <ø> (ø)`	`13.00 <0.00> (ø)`
...en/transformer/php/PhpGapicSurfaceTransformer.java	`93.02% <87.50%> (-0.45%)`	`46.00 <1.00> (+3.00)`	⬇️
...e/api/codegen/transformer/TestCaseTransformer.java	`98.60% <100.00%> (ø)`	`56.00 <3.00> (+1.00)`
...nsformer/nodejs/NodeJSGapicSurfaceTransformer.java	`100.00% <100.00%> (ø)`	`29.00 <0.00> (ø)`
.../transformer/py/PythonGapicSurfaceTransformer.java	`99.58% <100.00%> (+<0.01%)`	`43.00 <0.00> (ø)`
.../transformer/ruby/RubyGapicSurfaceTransformer.java	`99.29% <100.00%> (+<0.01%)`	`46.00 <0.00> (ø)`
...pi/codegen/transformer/MockServiceTransformer.java	`93.10% <0.00%> (+3.44%)`	`15.00% <0.00%> (+1.00%)`

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 994ffa2...3886997. Read the comment docs.

vchudnov-g

Looking good! Only nits so far, but please wait for @vam-google to TAL.

vchudnov-g · 2020-11-16T20:58:35Z

src/main/java/com/google/api/codegen/transformer/php/PhpGapicSurfaceTransformer.java

-        namer.getAndSaveNicknameForGrpcClientTypeName(
-            context.getImportTypeTable(), context.getInterfaceModel());
-    apiImplClass.grpcClientTypeName(grpcClientTypeName);
+    if (supportsGrpcTransport()) {


I would add a comment before this if block making it explicitly that we're generating GAPICs that support only one transport for now.

For PHP, this block generates a GAPIC that supports both.

I've added comments to clarify, let me know if this is sufficient!

src/main/java/com/google/api/codegen/transformer/php/PhpGapicSurfaceTransformer.java

vam-google

Overall looks good. Just a few comments.
I'm also worried a bit about the addition of one more huge test file library_rest.proto. Why can't we use one of the existing library.proto. Is it because of streaming methods?

vam-google · 2020-11-17T19:40:03Z

src/main/java/com/google/api/codegen/transformer/php/PhpGapicSurfaceTransformer.java

+    } else {
+      // PHP generates a client that only supports REST
+      apiImplClass.grpcClientTypeName("");
+      apiImplClass.stubs(new ArrayList<>());


nit: probably Collections.emptyList() is what you need here.

vam-google · 2020-11-17T19:43:36Z

src/main/java/com/google/api/codegen/transformer/php/PhpGapicSurfaceTransformer.java

@@ -426,6 +437,9 @@ private RestPlaceholderConfigView generateRestPlaceholderConfigView(
    List<GrpcStreamingDetailView> result = new ArrayList<>();

    for (MethodModel method : context.getGrpcStreamingMethods()) {
+      if (!supportsGrpcTransport()) {
+        throw new RuntimeException("Streaming methods only valid for gRPC transport");


How will this behave for existing APIs? There are many APIs that PHP generates the multitransport client and those have streaming methods. I believe in that case PHP still creates a client, just the streaming methods are no-op, but the generation itself does not fail and all the non-streaming methods are present in the generated surface. Will make gapic-generator throwing exception for such APIs and failing the whole generation process as a result?

Also, please do not use RuntimeException directly. It is an exception designed to be a base for the other specific exceptions. Please pick something more specific (UnsupportedOperation or something like that)

vam-google

LGTM

bshaffer added 8 commits November 10, 2020 14:29

add transport to php gapic bazel rules

287a658

feat: adds support for PHP DIREGAPIC (WIP)

ab81e22

adds DIREGAPIC baseline

66e206c

ensures streaming protos cannot be generated for PHP REST-only GAPICs

0f7e4ee

addresses two review comments

3dcab1b

simplifies gapic.yaml for v2

b783693

uses supportsGrpcTransport instead of isRestOnlyTransport

88d5714

simplify gapic v2 yaml even more

6e59c21

bshaffer requested a review from a team as a code owner November 10, 2020 22:31

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 10, 2020

bshaffer mentioned this pull request Nov 10, 2020

DIREGAPIC for PHP support #3301

Closed

bshaffer requested a review from dwsupplee November 10, 2020 22:41

bshaffer added 2 commits November 13, 2020 12:55

Merge branch 'master' into add-rest-only-transport-option

dc89f18

fix baseline test

9e0fd51

vchudnov-g reviewed Nov 16, 2020

View reviewed changes

bshaffer added 2 commits November 17, 2020 06:20

address review comments

76f6c14

Merge branch 'master' into add-rest-only-transport-option

8daae83

vam-google reviewed Nov 17, 2020

View reviewed changes

vam-google approved these changes Nov 17, 2020

View reviewed changes

bshaffer added 2 commits December 1, 2020 10:43

Merge branch 'master' into add-rest-only-transport-option

e9704b5

Merge branch 'master' into add-rest-only-transport-option

3886997

bshaffer merged commit 0456289 into master Dec 8, 2020

bshaffer deleted the add-rest-only-transport-option branch December 8, 2020 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add DIREGAPIC support for PHP #3305

feat: add DIREGAPIC support for PHP #3305

bshaffer commented Nov 10, 2020 •

edited

Loading

codecov bot commented Nov 10, 2020 •

edited

Loading

vchudnov-g left a comment

vchudnov-g Nov 16, 2020

bshaffer Nov 17, 2020

bshaffer Nov 17, 2020

vam-google left a comment

vam-google Nov 17, 2020

vam-google Nov 17, 2020

vam-google left a comment

feat: add DIREGAPIC support for PHP #3305

feat: add DIREGAPIC support for PHP #3305

Conversation

bshaffer commented Nov 10, 2020 • edited Loading

codecov bot commented Nov 10, 2020 • edited Loading

Codecov Report

vchudnov-g left a comment

Choose a reason for hiding this comment

vchudnov-g Nov 16, 2020

Choose a reason for hiding this comment

bshaffer Nov 17, 2020

Choose a reason for hiding this comment

bshaffer Nov 17, 2020

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

vam-google Nov 17, 2020

Choose a reason for hiding this comment

vam-google Nov 17, 2020

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

bshaffer commented Nov 10, 2020 •

edited

Loading

codecov bot commented Nov 10, 2020 •

edited

Loading