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

Less magic pathtemplate strings #2402

Conversation

andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Nov 2, 2018

Partially addresses googleapis/google-cloud-java#3604.

Don't force pathtemplate strings to start with a literal string.

The status quo, for path templates, is to magically append a literal string to templated paths that begin with a bracketed string, e.g. the path {project}/zones/{zone} is rendered as projects/{project}/zones/{zone}. I don't remember exactly why we decided to do this, though it may have been related to purity of usage of PathTemplates, where all existing Google grpc path templates start with a projects (or possibly organizations) substring.

More importantly, this PR allows us to parse a resource string returned from the server (see issue googleapis/google-cloud-java#3604), which often looks like https://www.googleapis.com/compute/v1/projects/{project}/zones/{zone}. The compute.v1.json Discovery API specifies that the baseUrl is exactly https://www.googleapis.com/compute/v1/projects. We can then take a resource string from the server, remove the given baseUrl prefix, and then parse the remaining suffix {project}/zones/{zone} into a ProjectsZoneName.

See googleapis/google-cloud-java#3899 and googleapis/discovery-artifact-manager#95 for the resulting surface diffs from this generator change.

@andreamlin andreamlin changed the title Don't force pathtemplate strings to start with a literal string (WIP) Less magic pathtemplate strings (WIP) Nov 2, 2018
@andreamlin
Copy link
Contributor Author

PTAL

@andreamlin andreamlin changed the title Less magic pathtemplate strings (WIP) Less magic pathtemplate strings Nov 5, 2018
@garrettjonesgoogle
Copy link
Member

Couldn't we also remove /projects off the end of baseUrl, so that we can then easily combine the strings?

I'm pretty concerned about the inconsistency of project name patterns for people using multiple APIs in Cloud.

@andreamlin
Copy link
Contributor Author

I agree that there would be an inconsistency of project name patterns across different Google Cloud APIs, but this change would restore consistency in

  • how a method endpoint is simply getBaseUrl() + resourceName.toString()
  • how prior users of Compute would have created a serialized resource name for Compute resources, because this is how it's been defined in the Discovery doc
  • (this hasn't been implemented yet) what generated code comments would say, which would be constructed from relevant fields like method.path and method.description.

I'm generally wary of any kind of magic that happens after configgen; all guessed-at logic should be put in configgen. Removing /projects/ from the end of baseUrl is more magic; how would we know to remove that from the baseUrl? Endpoint URLs are defined in the service yaml, which isn't generally an input for discogapic. We get lucky in this API because projects is the plural of {project}, but we could also encounter a projects/{project_id} where we can't figure out if we should remove the ending suffix from the baseurl.

@codecov-io
Copy link

codecov-io commented Nov 5, 2018

Codecov Report

Merging #2402 into fix_compute_resource_parsing will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@                        Coverage Diff                         @@
##             fix_compute_resource_parsing    #2402      +/-   ##
==================================================================
+ Coverage                            86.8%   86.83%   +0.02%     
+ Complexity                           5365     5349      -16     
==================================================================
  Files                                 459      459              
  Lines                               21179    21088      -91     
  Branches                             2309     2296      -13     
==================================================================
- Hits                                18384    18311      -73     
+ Misses                               1972     1961      -11     
+ Partials                              823      816       -7
Impacted Files Coverage Δ Complexity Δ
...degen/discogapic/transformer/DiscoGapicParser.java 92.72% <ø> (+1.2%) 22 <0> (ø) ⬇️
...google/api/codegen/config/PageStreamingConfig.java 45.23% <0%> (-10.48%) 7% <0%> (-4%)
...om/google/api/codegen/config/ProtoMethodModel.java 86.56% <0%> (-1.5%) 33% <0%> (-1%)
...egen/transformer/java/JavaMethodViewGenerator.java 91.46% <0%> (-1.22%) 18% <0%> (-1%)
...om/google/api/codegen/gapic/GapicGeneratorApp.java 27.19% <0%> (-1.02%) 7% <0%> (ø)
...m/google/api/codegen/config/GapicMethodConfig.java 78.5% <0%> (-0.19%) 21% <0%> (-2%)
.../com/google/api/codegen/metacode/InitCodeNode.java 86.73% <0%> (-0.12%) 79% <0%> (-1%)
...pi/codegen/viewmodel/ReadFileInitCodeLineView.java 100% <0%> (ø) 3% <0%> (ø) ⬇️
...e/api/codegen/configgen/ProtoPagingParameters.java 100% <0%> (ø) 6% <0%> (-3%) ⬇️
.../google/api/codegen/config/GapicProductConfig.java 77.05% <0%> (+0.07%) 75% <0%> (ø) ⬇️
... and 2 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 03f61fb...1cf8449. Read the comment docs.

@andreamlin andreamlin force-pushed the compute_resource_names branch from 85dcd3f to 3980ae0 Compare November 5, 2018 18:50
@andreamlin andreamlin force-pushed the compute_resource_names branch from 3980ae0 to 485f177 Compare November 5, 2018 19:47
@andreamlin andreamlin requested review from chingor13 and pongad and removed request for garrettjonesgoogle December 7, 2018 23:37
@andreamlin
Copy link
Contributor Author

The circleCI compute-discovery-test fails because this PR would require a new version of the compute_gapic.yaml to be generated using this PR.

PTAL

@andreamlin andreamlin changed the base branch from master to fix_compute_resource_parsing December 10, 2018 23:59
@andreamlin
Copy link
Contributor Author

Merging into a separate branch, since this would be a breaking change for the Compute client.

@andreamlin andreamlin merged commit d78306f into googleapis:fix_compute_resource_parsing Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants