FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore: support complex resource identifiers by miraleung · Pull Request #125 · googleapis/api-common-java · GitHub

This repository was archived by the owner on Sep 27, 2023. It is now read-only.
/ api-common-java Public archive

chore: support complex resource identifiers - #125

Merged
miraleung merged 3 commits into
masterfrom
miraleung/resource_names
May 13, 2020
ConversationCommits (3)ChecksFiles changed
Merged

chore: support complex resource identifiers#125
miraleung merged 3 commits into
masterfrom
miraleung/resource_names

Conversation

miraleung commented May 6, 2020

Copy link
Copy Markdown
Contributor

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

Relevant issue here.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 6, 2020

codecov Bot commented May 6, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@             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.

miraleung mentioned this pull request May 6, 2020
Merged
vam-google self-requested a review May 6, 2020 15:12
miraleung requested a review from chrisdunelm May 6, 2020 21:20
vam-google reviewed May 6, 2020
View reviewed changes

vam-google left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

Comment thread src/main/java/com/google/api/pathtemplate/PathTemplate.java Outdated
Comment thread src/main/java/com/google/api/pathtemplate/PathTemplate.java Outdated
Comment thread src/main/java/com/google/api/pathtemplate/PathTemplate.java Outdated
Comment thread src/main/java/com/google/api/pathtemplate/PathTemplate.java Outdated
Comment thread src/main/java/com/google/api/pathtemplate/PathTemplate.java
Comment thread src/main/java/com/google/api/pathtemplate/PathTemplate.java Outdated
Comment thread src/test/java/com/google/api/pathtemplate/PathTemplateTest.java
Comment thread src/test/java/com/google/api/pathtemplate/PathTemplateTest.java
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");

vam-google May 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

miraleung May 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

chrisdunelm May 13, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

vam-google approved these changes May 11, 2020
View reviewed changes

vam-google left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 12, 2020
chrisdunelm approved these changes May 13, 2020
View reviewed changes

chrisdunelm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM once CI passes.

alexander-fenster added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels May 13, 2020
miraleung merged commit 140d26d into master May 13, 2020
miraleung deleted the miraleung/resource_names branch May 13, 2020 22:10
chingor13 mentioned this pull request May 26, 2020
Merged
miraleung added a commit that referenced this pull request Jun 3, 2020
chore: support complex resource identifiers (#125)
b98fb5b 
* chore: support complex resource identifiers

* remove debug printf

* fix: clean up PathTemplate.java and tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
chrisdunelm chrisdunelm approved these changes
vam-google vam-google approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

chrisdunelm

vam-google

Labels

cla: yes This human has signed the Contributor License Agreement. kokoro:force-run Add this label to force Kokoro to re-run the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL