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

impl: url encode routing keys #12440

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Aug 22, 2023

Part of the work for #12232

I am not adding any tests. I am almost certain that these keys cannot contain characters that need to be escaped. Note that there are no corresponding changes to any generated code...

But it is simple enough to encode them and the encoding is done at code generation time.... So whatever.


This change is Reviewable

@dbolduc dbolduc temporarily deployed to internal August 22, 2023 16:45 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.01% ⚠️

Comparison is base (56b220f) 93.62% compared to head (e6e6eed) 93.62%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12440      +/-   ##
==========================================
- Coverage   93.62%   93.62%   -0.01%     
==========================================
  Files        2035     2035              
  Lines      180223   180223              
==========================================
- Hits       168742   168741       -1     
- Misses      11481    11482       +1     
Files Changed Coverage Δ
generator/internal/metadata_decorator_generator.cc 34.54% <0.00%> (ø)
...ator/internal/metadata_decorator_rest_generator.cc 0.00% <0.00%> (ø)
generator/internal/http_option_utils.cc 92.41% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbolduc dbolduc marked this pull request as ready for review August 22, 2023 17:31
@dbolduc dbolduc requested a review from a team as a code owner August 22, 2023 17:31
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

I think I remember reading that while we currently don't have cases that need this encoding, in the future we might.

This seems like a unit testable thing, if nothing else as a regression test.

@dbolduc
Copy link
Member Author

dbolduc commented Aug 29, 2023

This seems like a unit testable thing, if nothing else as a regression test.

I was overcome by laziness. Sorry. Still I think I am just going to merge this thing because it seems likely to conflict with other incoming changes.

Most of our unit tests involve parsing protos. I doubt field names can contain characters that would be url escaped... So I doubt that the protos could be parsed.... But admittedly, I didn't try.

@dbolduc dbolduc merged commit 80e0a05 into googleapis:main Aug 29, 2023
52 checks passed
@dbolduc dbolduc deleted the url-encode-x-goog-request-params branch August 29, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants