Bump Retrofit version to 2.0.0 #137

DmitriyZaitsev · 2016-03-24T13:38:51Z

No description provided.

lenguyenthanh · 2016-03-24T14:12:46Z

@DmitriyZaitsev: Thanks for your contribution, but it'd better to make it pass all the tests before create a PR. On the other hand, we have an issue #111 about ChangeBaseUrl class and Retrofit v2.0.0, please take a look at it.

lenguyenthanh · 2016-03-24T14:17:52Z

app/src/main/java/com/artemzin/qualitymatters/api/ChangeableBaseUrl.java


 /**
 * Such implementation allows us easily change base url in the integration and functional tests!
 */
-public class ChangeableBaseUrl implements BaseUrl {
+public class ChangeableBaseUrl {


Retrofit 2.0.0 drops the mutable base URL, so this class doesn't make sense any more and it'll break the tests.

Yes, there is an issue for that: #111.

For now you can do it via OkHttp interceptor/

Thanks for pointing me out. I'll dig in this direction.

DmitriyZaitsev · 2016-03-24T14:23:10Z

@lenguyenthanh thanks for your comment. Of course, before making this PR I made sure that local test have passed successfully.

UPD: Will check again

DmitriyZaitsev · 2016-03-24T14:37:22Z

Indeed. My fault. :(

Attempt №3 has brought me closer...
Attempt №4. Fixed unit tests

DmitriyZaitsev · 2016-03-24T21:55:21Z

app/src/main/java/com/artemzin/qualitymatters/network/HostSelectionInterceptor.java

+        if (host != null) {
+            final HttpUrl newUrl = request.url()
+                .newBuilder()
+                .host(host)


It seems I should parse baseUrl and build newUrl also with port() and scheme() here

yup, probably

DmitriyZaitsev · 2016-03-25T10:54:25Z

@artem-zinnatullin
MockWebServerRule.java, Line 50
Mock web server cannot be stopped somehow. Could you please take a look? If he stopped, all tests have passed successfully.

codecov-io · 2016-03-27T20:37:12Z

Current coverage is `82.32%`

Merging #137 into master will increase coverage by +0.74% as of 06f9f83

@@            master    #137   diff @@
======================================
  Files           38      38       
  Stmts          554     577    +23
  Branches        47      50     +3
  Methods          0       0       
======================================
+ Hit            452     475    +23
  Partial         29      29       
  Missed          73      73

Review entire Coverage Diff as of 06f9f83

Powered by Codecov. Updated on successful CI builds.

artem-zinnatullin · 2016-03-28T13:26:09Z

app/src/main/java/com/artemzin/qualitymatters/network/HostSelectionInterceptor.java

+        final String baseUrl = this.baseUrl;
+
+        if (baseUrl != null) {
+            request = modifyRequest(request, baseUrl);


What do you think about the problem that we redirect all requests (not only api ones) to the mock webserver?

I see 2 solutions:

Do not rely on interceptor and recreate retrofit instance (clean solution)

Interceptor should know about real base url and redirect only requests that would hit real base url

Actually, we do not redirect all requests to the mock webserver.
By default HostSelectionInterceptor is dummy. It doesn't affect the current logic at all until baseUrl is set.
In fact, we added the interceptor that does nothing, though it has the property of dynamic change of the base url if there is a desire.

We do redirect all requests that goes through that instance of OkHttpClient as soon as baseUrl is set in the interceptor

That's right. I mean that dynamic url is used for testing purposes only and now only test code mutates the interceptor.

Nothing prevents us from hitting not only API in future and functional tests will cover that and we may have problems, that's why I'm trying to say

No. Recreate OkHttpClient. :) Network module will provide two different OkHttpClient instances. One for Retrofit only (with HostSelectionInterceptor) and the second one - for other dudes e.g. Picasso (without that interceptor).

Wait wait wait then :)

Please check square/retrofit#1652 and #111

The idea is that Retrofit has immutable baseUrl so we can recreate Retrofit and switch it to new url.

Recreating OkHttpClient is not that great, we use it for many things and using one instance for everything is great because it reuses same connection pool and we don't allocate memory and resources for using multiple clients

I prefer to recreate Retrofit than OkHttp, because baseUrl is something depends on Retrofit not OkHttp. If we change baseUrl in an Intercepter, we add magic to our code. It's not really good.

Actually, what I'm doing in my project is: Using some kind of TestModule so that we can inject mock or fake url for testing.

I made some investigations and don't see beautiful solution. The problem is object graph is created before MockWebServer starts. If we want to instatiate Retrofit with baseUrl taken from mockWebServer, we should set that url into ApiModule. Using Provider<> in this case looks like a smelling hack for me.
When I was looking for solution, I found that Jake Wharton in his u2020 app uses the same approach I suggest: he uses separate OkHttpClient for API calls @Named("Api"). And I believe it's great because makes code cleaner and gives two extra benefits:
• we can use dynamic url only for api in tests
• api client won't be busy and shouldn't wait while Picasso loads images.

Anyway, I think such refactoring is out of this scope.
I suggest next solution: use two OkHttpClients for now and next step - if you insist, make refactoring in other PR.

Plastix · 2016-05-10T11:57:18Z

I have a question that deals with OkHttp interceptors so I'm going to ask it here. I only want my interceptor action to be performed on certain requests, e.g. signing OAuth headers only for certain requests. What is the best way to "enable" and "disable" the functionality of my interceptor?

I was thinking of having a boolean enabled in the interceptor and mutating that. Is there a better cleaner way? Thanks!

artem-zinnatullin · 2016-05-10T15:53:28Z

Better to have no state at all, can you pass this flag as request header
and detect and remove it in the interceptor?

On Tue, 10 May 2016, 14:57 Plastix, [email protected] wrote:

I have a question that deals with OkHttp interceptors so I'm going to ask
it here. I only want my interceptor action to be performed on certain
requests, e.g. signing OAuth headers only for certain requests. What is the
best way to "enable" and "disable" the functionality of my interceptor?

I was thinking of having a boolean enabled in the interceptor and
mutating that. Is there a better cleaner way? Thanks!

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#137 (comment)

Plastix · 2016-05-10T16:33:28Z

@artem-zinnatullin Thanks. I just figured this out myself before you responded. I'm going to be using a request header.

artem-zinnatullin · 2016-07-04T22:38:06Z

Closing for now, will be part of 2.0

lenguyenthanh reviewed Mar 24, 2016
View reviewed changes

DmitriyZaitsev force-pushed the dz/update-retrofit branch from 1a7db71 to 91b5e43 Compare March 24, 2016 21:34

DmitriyZaitsev reviewed Mar 24, 2016
View reviewed changes

artem-zinnatullin added the enhancement label Mar 24, 2016

artem-zinnatullin added this to the v1.0 milestone Mar 24, 2016

artem-zinnatullin self-assigned this Mar 24, 2016

DmitriyZaitsev force-pushed the dz/update-retrofit branch 3 times, most recently from 889c8ad to e4a82b7 Compare March 25, 2016 00:11

DmitriyZaitsev force-pushed the dz/update-retrofit branch from e4a82b7 to e7d7e16 Compare March 27, 2016 20:23

artem-zinnatullin reviewed Mar 28, 2016
View reviewed changes

artem-zinnatullin mentioned this pull request Mar 29, 2016

Use a NetworkBitmapClient as a wrapper for Picasso #139

Merged

DmitriyZaitsev added 2 commits April 1, 2016 17:51

Bump Retrofit version to 2.0.0

4d77b59

Create another OkHttpClient for rest api

ef3620f

DmitriyZaitsev force-pushed the dz/update-retrofit branch from e7d7e16 to ef3620f Compare April 1, 2016 18:09

artem-zinnatullin closed this Jul 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump Retrofit version to 2.0.0 #137

Bump Retrofit version to 2.0.0 #137

DmitriyZaitsev commented Mar 24, 2016

lenguyenthanh commented Mar 24, 2016

lenguyenthanh Mar 24, 2016

artem-zinnatullin Mar 24, 2016

DmitriyZaitsev Mar 24, 2016

DmitriyZaitsev commented Mar 24, 2016

DmitriyZaitsev commented Mar 24, 2016

DmitriyZaitsev Mar 24, 2016

artem-zinnatullin Mar 24, 2016

DmitriyZaitsev commented Mar 25, 2016

codecov-io commented Mar 27, 2016

artem-zinnatullin Mar 28, 2016

DmitriyZaitsev Mar 28, 2016

artem-zinnatullin Mar 28, 2016

DmitriyZaitsev Mar 28, 2016

artem-zinnatullin Mar 28, 2016

DmitriyZaitsev Mar 29, 2016

artem-zinnatullin Mar 29, 2016

lenguyenthanh Mar 30, 2016

DmitriyZaitsev Apr 1, 2016

DmitriyZaitsev Apr 1, 2016

Plastix commented May 10, 2016

artem-zinnatullin commented May 10, 2016

Plastix commented May 10, 2016

artem-zinnatullin commented Jul 4, 2016

Bump Retrofit version to 2.0.0 #137

Bump Retrofit version to 2.0.0 #137

Conversation

DmitriyZaitsev commented Mar 24, 2016

lenguyenthanh commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyZaitsev commented Mar 24, 2016

DmitriyZaitsev commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyZaitsev commented Mar 25, 2016

codecov-io commented Mar 27, 2016

Current coverage is 82.32%

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plastix commented May 10, 2016

artem-zinnatullin commented May 10, 2016

Plastix commented May 10, 2016

artem-zinnatullin commented Jul 4, 2016

Current coverage is `82.32%`