Upgrade okhttp3 to latest #231

usulkies · 2019-11-12T12:16:48Z

Changes

Upgrading the okhttp3 version to the latest on the 3.x line

Change few tests assertion to match encoded URL

References

N/A

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

This change adds test coverage
This change has been tested on the latest version of the platform/language or why not

Checklist

I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors

lbalmaceda · 2019-11-12T14:44:07Z

src/test/java/com/auth0/client/mgmt/LogEventsEntityTest.java

@@ -106,7 +106,7 @@ public void shouldListLogEventsWithQuery() throws Exception {
        assertThat(recordedRequest, hasMethodAndPath("GET", "/api/v2/logs"));
        assertThat(recordedRequest, hasHeader("Content-Type", "application/json"));
        assertThat(recordedRequest, hasHeader("Authorization", "Bearer apiToken"));
-        assertThat(recordedRequest, hasQueryParameter("q", "email%3A%5C*%40gmail.com"));
+        assertThat(recordedRequest, hasQueryParameter("q", "email:\\*@gmail.com"));


I wonder if this change on behavior could be a breaking change??

@lbalmaceda
This is my confession:
I'm not a real contributor to auth0...
I'm just suffering from dependency convergence. We're using Zipkin for tracing reporting, and it's already using version 3.14.3 of okhttp3.
For some reason, the auth0 project, which we're also using, is still back on version 3.9.1.
I fear these 2 versions are not compatible.
So I tried to just upgrade the okhttp3 version but found out that some tests are breaking, probably because of changes made between these versions handling URL encoding.
I'll be happy if someone who really understands what is the use of okhttp3 for auth0 will be able to handle this version bump.
I think it's important for auth0 to remain relevant for users, to always try and bump to the latest dependencies versions. Consider using @dependabot for this.
Could you please help with this?
Thanks a lot,
Uziel

… addQueryParameter

usulkies · 2019-12-09T22:00:31Z

@lbalmaceda
I tried another solution, after digging the changes a bit.
I changed the code in a way that preserves expected behavior as documented in tests.
Did it be replacing all calls to HttpUrl.addQueryParameter to HttpUrl.addEncodedQueryParameter.
The other method do not try to encode the value as the first is now doing (it didn't do it in the old version of okhttp3).
I hope now it'll be approved. 🙂
Thanks for your review!

lbalmaceda · 2019-12-26T18:23:25Z

@usulkies Thanks for the keep looking for a workaround to keep the tests untouched.
I have to say it looks odd if we need to change every mention of addQueryParameter, especially because addEncodedQueryParameter is meant to skip the UTF8 encoding step and add it as is. If anything, a problem would raise when weird ASCII chars are being used. I fear to block the authentication of users if we happen to break it with this PR.

Give me a few days to try this out manually, please.

usulkies · 2020-01-20T12:08:50Z

Give me a few days to try this out manually, please.

Hi @lbalmaceda ,
Any news?

lbalmaceda · 2020-01-20T16:21:26Z

@usulkies I couldn't check this yet. Let me add a backlog item so at least the team is aware.

MorrisonCole · 2020-02-04T07:59:23Z

Ran into this as well and raised #236 before I noticed this PR. This is really quite an old dependency now 😅

lbalmaceda · 2020-04-03T13:38:16Z

@usulkies Sorry for the delay here. We're going through some issues categorization and we think this is something worth adding to the SDK. We will investigate and test a bit the changes in terms of char encoding, and would need to bump the OkHttp version to the latest 3.x. Is that something you can do in the meantime?
Thanks!

usulkies · 2020-05-20T10:51:11Z

@usulkies Sorry for the delay here. We're going through some issues categorization and we think this is something worth adding to the SDK. We will investigate and test a bit the changes in terms of char encoding, and would need to bump the OkHttp version to the latest 3.x. Is that something you can do in the meantime?
Thanks!

Hi @lbalmaceda I'm not sure I understood your question. What would you like me to do in the meantime?

lbalmaceda · 2020-05-20T13:57:30Z

👋 that would be bumping to the current latest version, which according to maven central is 3.14.9. I'm planning to bring this issue up in our next planning meeting.

usulkies · 2020-05-20T18:37:43Z

👋 that would be bumping to the current latest version, which according to maven central is 3.14.9. I'm planning to bring this issue up in our next planning meeting.

Done

usulkies · 2020-05-25T10:37:17Z

@lbalmaceda When is the next planning meeting? Can you guess when we'll have this released?
It is currently a blocker for other libraries I'm using that I can't bump because they require a higher version of okhttp3 and it will break auth0.

jimmyjames · 2020-05-26T19:29:24Z

Hi @usulkies, I'm going to be reviewing this and will be following up this week. Thanks!

jimmyjames · 2020-05-27T19:54:55Z

Regarding the issues observed with tests failing prior to updating some usages to addEncodedQueryParameter is that starting in version 3.10.0, OkHttp started URL-encoding more characters (I believe this is the commit). This caused query params with the newly encoded characters (such as /, ,, @, =, among others) to be URL-encoded, which in itself is a good thing, but the tests were expecting the old behavior prior to that change.

Rather than changing to not URL encode query parameters (which is what addEncodedQueryParameter does), the better fix is to the tests themselves. And actually, very few tests would need updating, as we have a custom matcher that we can update to get the query params back from the URL.

@usulkies I've gotten this working locally, so instead of asking you to revert your changes on this PR to use addEncodedQueryParameter and update the request matcher, I'll follow up shortly with a new PR targeting the same version of OkHttp 👍

jimmyjames · 2020-05-27T20:14:20Z

Closing this PR as it has been addressed in #262. We are hoping to do a release by the end of this week that will include this change; I'll follow up here when that's done. Thanks for your patience all! 🙇

usulkies · 2020-05-31T06:24:04Z

Thanks @jimmyjames !
This is really helpful!

jimmyjames · 2020-06-02T19:16:04Z

Just to make sure to close this loop - version 1.18.0 was released Friday and includes an update to use OkHttp 3.14.9 👍

usulkies requested a review from a team November 12, 2019 12:16

lbalmaceda reviewed Nov 12, 2019

View reviewed changes

Uziel Sulkies added 3 commits December 9, 2019 23:51

Upgrade okhttp3 to latest

09d228a

Preserve compatibility by calling addEncodedQueryParameter instead of…

1e057da

… addQueryParameter

Revert changes

d5f6d04

Merge branch 'master' into update_okhttp3

536a79c

usulkies requested a review from lbalmaceda December 11, 2019 10:37

lbalmaceda self-assigned this Dec 26, 2019

lbalmaceda added the dependencies One or more dependencies are being bumped label Apr 3, 2020

Merge branch 'master' into update_okhttp3

239b348

Upgrade okhttp to 3.14.9

e72dc2f

Merge branch 'master' into update_okhttp3

fe8e028

Merge branch 'master' into update_okhttp3

e3612a0

jimmyjames mentioned this pull request May 26, 2020

Explicit dependency on bouncycastle for tests #261

Merged

5 tasks

Merge branch 'master' into update_okhttp3

878130d

jimmyjames mentioned this pull request May 27, 2020

Update to OkHttp 3.14.9 #262

Merged

5 tasks

jimmyjames closed this May 27, 2020

usulkies deleted the update_okhttp3 branch May 31, 2020 06:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade okhttp3 to latest #231

Upgrade okhttp3 to latest #231

usulkies commented Nov 12, 2019 •

edited

Loading

lbalmaceda Nov 12, 2019

usulkies Dec 9, 2019

usulkies commented Dec 9, 2019

lbalmaceda commented Dec 26, 2019

usulkies commented Jan 20, 2020

lbalmaceda commented Jan 20, 2020

MorrisonCole commented Feb 4, 2020 •

edited

Loading

lbalmaceda commented Apr 3, 2020

usulkies commented May 20, 2020

lbalmaceda commented May 20, 2020

usulkies commented May 20, 2020

usulkies commented May 25, 2020

jimmyjames commented May 26, 2020

jimmyjames commented May 27, 2020

jimmyjames commented May 27, 2020

usulkies commented May 31, 2020

jimmyjames commented Jun 2, 2020

Upgrade okhttp3 to latest #231

Upgrade okhttp3 to latest #231

Conversation

usulkies commented Nov 12, 2019 • edited Loading

Changes

References

Testing

Checklist

lbalmaceda Nov 12, 2019

Choose a reason for hiding this comment

usulkies Dec 9, 2019

Choose a reason for hiding this comment

usulkies commented Dec 9, 2019

lbalmaceda commented Dec 26, 2019

usulkies commented Jan 20, 2020

lbalmaceda commented Jan 20, 2020

MorrisonCole commented Feb 4, 2020 • edited Loading

lbalmaceda commented Apr 3, 2020

usulkies commented May 20, 2020

lbalmaceda commented May 20, 2020

usulkies commented May 20, 2020

usulkies commented May 25, 2020

jimmyjames commented May 26, 2020

jimmyjames commented May 27, 2020

jimmyjames commented May 27, 2020

usulkies commented May 31, 2020

jimmyjames commented Jun 2, 2020

usulkies commented Nov 12, 2019 •

edited

Loading

MorrisonCole commented Feb 4, 2020 •

edited

Loading