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.

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

chore: support complex resource identifiers #125

Merged
merged 3 commits into from May 13, 2020
Merged

Conversation

miraleung
Copy link
Collaborator

@miraleung miraleung commented May 6, 2020

Add support for the non-slash resource name separators. That is, "-", "~", "_", ".". These changes are needed for gapic-generator.

Relevant issue here.

@googlebot googlebot added the cla: yes label May 6, 2020
@codecov
Copy link

@codecov codecov bot commented May 6, 2020

Codecov Report

Merging #125 into master will increase coverage by 3.02%.
The diff coverage is 88.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #125      +/-   ##
============================================
+ Coverage     60.93%   63.96%   +3.02%     
- Complexity      148      163      +15     
============================================
  Files            14       14              
  Lines           622      702      +80     
  Branches         92      112      +20     
============================================
+ Hits            379      449      +70     
- Misses          217      219       +2     
- Partials         26       34       +8     
Impacted Files Coverage Δ Complexity Δ
...java/com/google/api/pathtemplate/PathTemplate.java 69.06% <88.33%> (+4.21%) 106.00 <7.00> (+15.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 b6e9dad...12b22f3. Read the comment docs.

Copy link
Collaborator

@vam-google vam-google left a comment

There is a lot of complex logic, which is hard to comprehend and "compile" in head =) (especially the parseComplexResourceId() method), so given the test cases, I trust that it works correctly.

Basically looks good, with a few questions:

  1. It is probably a silly question, but I'm wondering if in the implementation we replaced the complex delimiters with /, remembering the mapping between the original delimiters and "fake" slashes, then propagated that to the existing PathTemplate logic (which knows only slashes), and then used the saved mapping, if needed, to reverse that in PathTemplate.match() implementation. Does it have a chance to work? If yes, it can result in much simpler implementation.
    Or, in other words: are the "new" delimiters just another syntax for delimiters or is there a fundamental difference between /{a}/{b} and /{a}~{b} formats (does ~ put {a} and {b} in some sort of different relation compared to / separator)?

  2. This is slightly related to the question 1 (because it may make 1 more feasible). Should we forbade usage of a delimiter character in the body of the resoruce name? (see a comment for one of the test cases for more details).

Truth.assertThat(match.get("project")).isEqualTo("project-123");
Truth.assertThat(match.get("zone_a")).isEqualTo("europe-west3-c");
Truth.assertThat(match.get("zone_b")).isEqualTo("us-east3-a");
Truth.assertThat(match.get("zone_c")).isEqualTo("us");
Copy link
Collaborator

@vam-google vam-google May 6, 2020

Choose a reason for hiding this comment

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

Look like it uses greedy approach and for the last ambiguous segment ({zone_c}-{zone_d}), when provided with us-west2-b-europe-west2-b input it treats the stuff before the first hyphen (us) as zone_c and everything else as zone_d.

This ambiguity looks like a real problem. Even thought the implementation behaves predictably and reasonably, looks like us-west2-b-europe-west2-b should be split as zone_c=us-west2-b and zone_d=europe-west2-b.

Should we forbade a character in the body if the same character uses as a splitter?

Copy link
Collaborator Author

@miraleung miraleung May 7, 2020

Choose a reason for hiding this comment

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

This is a spec question, let's take this discussion over there. @chrisdunelm FYI.

Choose a reason for hiding this comment

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

Yes, this is a known potential problem.
The spec currently states that it is the responsibility of the API producer to ensure that any split character(s) are not present in the variables. As things are now, I consider this sufficient.
An alternative would be to verify that no parsed variable values contain any of the [used] split character(s); but I think this would be too much,

@miraleung
Copy link
Collaborator Author

@miraleung miraleung commented May 7, 2020

Responding to @vam-google's comment:

  1. Mixing up the existing "/" and complex resource ID delimiter parsing logic risks introducing error edge cases. For example, if someone passes in projects/{project}-zones/{zone_a}/{zone_b} where they meant projects/{project}/zones/{zone_a}-{zone_b}, the former could result in a valid parsing under the mixed implementation. Not only would we require additional logic to separate out literals and IDs (which could become even more complex), we'd risk having literals being parsed in as IDs, and vice-versa.
    One advantage of the current set-up is that it's easy to isolate and toggle complex resource ID parsing on/off, or add in a whitelist for the relevant clients if that becomes necessary.
  2. PTAL at the comment here.

Copy link
Collaborator

@vam-google vam-google left a comment

The travis jobs are failing on formatting validation. Please run ./gradlew googleJavaFormat to automatically format the files and push updated files.

LGTM, to not block this work, assuming build jobs pass and the spec "ambiguity" (the hyphen thing) is resolved/clarified in the spec.

@miraleung miraleung added kokoro:run kokoro:force-run labels May 12, 2020
Copy link

@chrisdunelm chrisdunelm left a comment

LGTM once CI passes.

@alexander-fenster alexander-fenster added kokoro:force-run and removed kokoro:force-run kokoro:run labels May 13, 2020
@miraleung miraleung merged commit 140d26d into master May 13, 2020
5 checks passed
@miraleung miraleung deleted the miraleung/resource_names branch May 13, 2020
@chingor13 chingor13 mentioned this pull request May 26, 2020
miraleung added a commit that referenced this issue Jun 3, 2020
* chore: support complex resource identifiers

* remove debug printf

* fix: clean up PathTemplate.java and tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kokoro:force-run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants