Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip generating REST methods if the RPC does not have HTTP annotation. #1117

Closed
blakeli0 opened this issue Dec 9, 2022 · 1 comment
Closed

Skip generating REST methods if the RPC does not have HTTP annotation. #1117

blakeli0 opened this issue Dec 9, 2022 · 1 comment
Assignees
@lqiu96
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@blakeli0
Copy link
Collaborator

blakeli0 commented Dec 9, 2022 •
edited
Loading

We should not generate REST methods if the RPC does not have HTTP annotation. See this proto and the proto that has it.
In this case, we are already not generating REST methods because it's BIDI streaming, but we need to use the HTTP annotation as the filter instead of using RPC type.

In addition, we should not generating the client at all if there are no RPCs with HTTP annotation. See go/actools-diregapic-parallelization-sync for details.

There could be multiple reasons an RPC does not have HTTP annotation:

  • The RPC is not supposed to be supported by REST. e.g. BIDI streaming methods
  • The RPC is not yet supported by the backend service, but could be supported in the future
  • The proto is not properly configured.
@blakeli0 blakeli0 added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 9, 2022
@lqiu96 lqiu96 mentioned this issue Jan 4, 2023
Merged
gcf-merge-on-green bot pushed a commit that referenced this issue Jan 10, 2023
@lqiu96
feat: Add callable getters for non-eligible or non-enabled REST metho…
84a1355 
…ds (#1211)

This is a PR for Part 1 of #1117 (Override method with clearer exception messages for non-eligible and non-enabled Service RPCs). Opening this while I look at other possible approaches: 

Other approaches looked at/ to revisit:
- Override createCallableClassMembers to return Map<String, Expr> (Return either VariableExpr or ThrowExpr)
- Create a duplicate createCallableClassMembers (i.e. createInvalidCallableClassMembers) that copies functionality of createCallableClassMembers but returns Map<String, ThrowExpr>

Both approaches above had an issue when setting the return type for the callableGetter. ThrowExpr's type is always set as `UnsupportedOperationException` but the MethodDefinition's return type is not (i.e. for Streams it would be ServerStreamCallable or ClientStreamCallable/ Unary is UnaryCallable vs. Operation, etc.). Would need potentially need another mapping between callableName and method return type for ThrowExprs.

This PR's implementation is copied from: https://togithub.com/googleapis/gapic-generator-java/blob/8c5e17ba325b7711f9ba9501992ab48e736ffc18/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceStubClassComposer.java#L284-L302
- Use getCallableType(protoMethod) to get the correct return type for the RPC
- ThrowExpr's return type is set to `UnsupportedOperationException`
@lqiu96 lqiu96 mentioned this issue Jan 10, 2023
Merged
@lqiu96
Copy link
Contributor

lqiu96 commented Jan 27, 2023

Closing this issue as we've merged in both Part 1 and Part 2 of this issue.

@lqiu96 lqiu96 closed this as completed Jan 27, 2023
@emmileaf emmileaf mentioned this issue Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lqiu96 lqiu96

Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@blakeli0 @lqiu96