Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Generate method overloads when a resource field has multiple resource names configured #3081

Merged
merged 39 commits into from
Jan 31, 2020

Conversation

yihanzhen
Copy link
Contributor

@yihanzhen yihanzhen commented Jan 30, 2020

  • When a field has child_type reference and the referenced resource has multiple patterns, allow this field to be associated with multiple resources if all these resources combined have exactly all the parent patterns of the resource referenced by child_type.
  • Generate method overloads if the field with multiple resource names are in a flattening config.
  • If there are multiple fields with multiple resource names, generate all possible combinations.
  • Always generate an overload with all resource names treated as strings.
  • In Java, repeated fields with resource name configs are always treated as List<String> to avoid multiple methods having the same signature after type erasure.
  • The * annotation is not yet supported.

See changes to ArchiveBooks, LongRunningArchiveBooks and StreamingArchiveBooks for testing generating overloads for unary, longrunning and streaming methods.

See changes to MoveBooks for testing generating all the combinations.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 30, 2020
@codecov
Copy link

codecov bot commented Jan 30, 2020

Codecov Report

Merging #3081 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3081   +/-   ##
=========================================
  Coverage     87.03%   87.03%           
  Complexity     6000     6000           
=========================================
  Files           493      493           
  Lines         23869    23869           
  Branches       2584     2584           
=========================================
  Hits          20775    20775           
  Misses         2239     2239           
  Partials        855      855

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 85a63ec...f12caac. Read the comment docs.

@yihanzhen yihanzhen changed the title Child type overloads Generate method overloads when a resource field has multiple resource names configured Jan 31, 2020
Copy link
Contributor

@chrisdunelm chrisdunelm left a comment

Choose a reason for hiding this comment

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

LGTM from the generated code point of view. Just one question.

.setRequiredSingularResourceName(requiredSingularResourceName)
.setRequiredSingularResourceNameOneof(requiredSingularResourceNameOneof)
.setRequiredSingularResourceNameCommon(requiredSingularResourceNameCommon)
.setRequiredSingularResourceName(requiredSingularResourceName == null ? null : requiredSingularResourceName.toString())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct that a required resource-name is allowed to be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been how java delegates the overload with resource name to the non-flattened method.

For example: https://github.com/googleapis/java-tasks/blob/master/google-cloud-tasks/src/main/java/com/google/cloud/tasks/v2/CloudTasksClient.java#L1532-L1540

Actually I think protobuf Java does not allow setting null values on any field, so the code will still fail on the client side. I guess we just pass the error handling to protobuf :)

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM (based on our in-person conversation)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants