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.

Sign up for GitHub

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

Jump to bottom

fix: disable uri normalization in ApacheHttpRequest #804

Merged
merged 4 commits into from Aug 30, 2019
Merged

fix: disable uri normalization in ApacheHttpRequest #804

merged 4 commits into from Aug 30, 2019

Conversation

chingor13
Copy link
Collaborator

Fixes #795

@chingor13 chingor13 requested a review from a team as a code owner August 28, 2019 16:53
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 28, 2019
elharo
elharo reviewed Aug 28, 2019
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Test?

@chingor13
Copy link
Collaborator Author

The Apache HttpClient implementation does a great job and making itself uninspectable once initialized - we would need to test its behavior (or use reflection to look at internals).

For these transport adapters, we will need to set up an integration test that somehow ensures the desired behavior.

@chingor13 chingor13 mentioned this pull request Aug 28, 2019
Merged
@chingor13
Copy link
Collaborator Author

Filed #805 for adding integration tests

elharo
elharo reviewed Aug 28, 2019
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

The initial bug report has enough details that this should be reproducible by behavior as you suggest, even if we have to hit an actual URL.

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 29, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 29, 2019
elharo
elharo reviewed Aug 29, 2019
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

More effort than I would have gone to. I would have just pinged the real URL, but this is more robust. :-)

...nt-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java Outdated Show resolved Hide resolved
...nt-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java Outdated Show resolved Hide resolved
elharo
elharo reviewed Aug 30, 2019
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

setNormalizeUri is not well documented in the Apache docs. What are the side effects of this change beyond the double slash issue?

...nt-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java Outdated Show resolved Hide resolved
@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 30, 2019
@chingor13 chingor13 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. labels Aug 30, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 30, 2019
@chingor13 chingor13 merged commit 0e6d451 into googleapis:master Aug 30, 2019
@chingor13 chingor13 deleted the disable-uri-normalize branch August 30, 2019 19:49
@chingor13 chingor13 mentioned this pull request Sep 11, 2019
Merged
clundin25 pushed a commit to clundin25/google-http-java-client that referenced this pull request Aug 11, 2022
@renovate-bot @eaball35
chore(deps): update dependency com.google.appengine:appengine to v1.9…
2038227 
….93 (googleapis#804)

Co-authored-by: Emily Ball <emilyball@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elharo elharo elharo approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

After upgrading to 1.31.0, library returns 403 while curl succeeds
4 participants
@chingor13 @elharo @googlebot @kokoro-team