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

Bump Retrofit version to 2.0.0 #137

Closed
wants to merge 2 commits into from
Closed

Bump Retrofit version to 2.0.0 #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d77b59
Bump Retrofit version to 2.0.0
DmitriyZaitsev Mar 24, 2016
ef3620f
Create another OkHttpClient for rest api
DmitriyZaitsev Mar 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/src/debug/java/com/artemzin/qualitymatters/network/OkHttpInterceptorsModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ public HttpLoggingInterceptor provideHttpLoggingInterceptor() {
return new HttpLoggingInterceptor(message -> Timber.d(message));
}

@Provides @Singleton @NonNull
public HostSelectionInterceptor provideHostSelectionInterceptor() {
return new HostSelectionInterceptor();
}

@Provides @OkHttpInterceptors @Singleton @NonNull
public List<Interceptor> provideOkHttpInterceptors(@NonNull HttpLoggingInterceptor httpLoggingInterceptor) {
return singletonList(httpLoggingInterceptor);
Expand Down
9 changes: 7 additions & 2 deletions ...ionalTests/java/com/artemzin/qualitymatters/functional_tests/rules/MockWebServerRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import com.artemzin.qualitymatters.functional_tests.TestUtils;

import java.io.IOException;
import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
Expand Down Expand Up @@ -36,7 +37,7 @@ public void evaluate() throws Throwable {
final MockWebServer mockWebServer = new MockWebServer();
mockWebServer.start();

TestUtils.app().applicationComponent().changeableBaseUrl().setBaseUrl(mockWebServer.url("").toString());
TestUtils.app().applicationComponent().hostSelectionInterceptor().setBaseUrl(mockWebServer.url("").toString());

if (!needsMockWebServer.setupMethod().isEmpty()) {
final Method setupMethod = testClassInstance.getClass().getDeclaredMethod(needsMockWebServer.setupMethod(), MockWebServer.class);
Expand All @@ -47,7 +48,11 @@ public void evaluate() throws Throwable {
try {
base.evaluate();
} finally {
mockWebServer.shutdown();
try {
mockWebServer.shutdown();
} catch (IOException ignored) {
// do nothing.
}
}
} else {
// No need to setup a MockWebServer, just evaluate the test.
Expand Down
2 changes: 1 addition & 1 deletion ...m/artemzin/qualitymatters/integration_tests/api/QualityMattersRestApiIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public void beforeEachTest() throws IOException {
mockWebServer.start();

// Change base url to the mocked
QualityMattersIntegrationRobolectricTestRunner.qualityMattersApp().applicationComponent().changeableBaseUrl().setBaseUrl(mockWebServer.url("").toString());
QualityMattersIntegrationRobolectricTestRunner.qualityMattersApp().applicationComponent().hostSelectionInterceptor().setBaseUrl(mockWebServer.url("").toString());

qualityMattersRestApi = QualityMattersIntegrationRobolectricTestRunner.qualityMattersApp().applicationComponent().qualityMattersApi();
}
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/java/com/artemzin/qualitymatters/ApplicationComponent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
import android.support.annotation.NonNull;

import com.artemzin.qualitymatters.api.ApiModule;
import com.artemzin.qualitymatters.api.ChangeableBaseUrl;
import com.artemzin.qualitymatters.api.QualityMattersRestApi;
import com.artemzin.qualitymatters.developer_settings.DeveloperSettingsComponent;
import com.artemzin.qualitymatters.developer_settings.DeveloperSettingsModule;
import com.artemzin.qualitymatters.developer_settings.LeakCanaryProxy;
import com.artemzin.qualitymatters.models.ModelsModule;
import com.artemzin.qualitymatters.network.HostSelectionInterceptor;
import com.artemzin.qualitymatters.network.NetworkModule;
import com.artemzin.qualitymatters.network.OkHttpInterceptorsModule;
import com.artemzin.qualitymatters.performance.AsyncJobsModule;
Expand Down Expand Up @@ -42,7 +42,7 @@ public interface ApplicationComponent {
QualityMattersRestApi qualityMattersApi();

@NonNull
ChangeableBaseUrl changeableBaseUrl();
HostSelectionInterceptor hostSelectionInterceptor();

// Provide AsyncJobObserver from the real app to the tests without need in injection to the test.
@NonNull
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/artemzin/qualitymatters/QualityMattersApp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void onCreate() {
protected DaggerApplicationComponent.Builder prepareApplicationComponent() {
return DaggerApplicationComponent.builder()
.applicationModule(new ApplicationModule(this))
// This url may be changed dynamically for tests! See ChangeableBaseUrl.
// This url may be changed dynamically for tests! See HostSelectionInterceptor.
.apiModule(new ApiModule("https://raw.githubusercontent.com/artem-zinnatullin/qualitymatters/master/rest_api/"));
}

Expand Down
26 changes: 13 additions & 13 deletions app/src/main/java/com/artemzin/qualitymatters/api/ApiModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import android.support.annotation.NonNull;

import com.artemzin.qualitymatters.BuildConfig;
import com.artemzin.qualitymatters.network.ForRestApi;
import com.fasterxml.jackson.databind.ObjectMapper;

import javax.inject.Singleton;
Expand All @@ -18,26 +19,25 @@
public class ApiModule {

@NonNull
private final ChangeableBaseUrl changeableBaseUrl;
private final String baseUrl;

public ApiModule(@NonNull String baseUrl) {
changeableBaseUrl = new ChangeableBaseUrl(baseUrl);
this.baseUrl = baseUrl;
}

@Provides @NonNull @Singleton
public ChangeableBaseUrl provideChangeableBaseUrl() {
return changeableBaseUrl;
public QualityMattersRestApi provideQualityMattersApi(@NonNull Retrofit retrofit) {
return retrofit.create(QualityMattersRestApi.class);
}

@Provides @NonNull @Singleton
public QualityMattersRestApi provideQualityMattersApi(@NonNull OkHttpClient okHttpClient, @NonNull ObjectMapper objectMapper, @NonNull ChangeableBaseUrl changeableBaseUrl) {
@Provides @NonNull
public Retrofit provideRetrofit(@NonNull @ForRestApi OkHttpClient okHttpClient, @NonNull ObjectMapper objectMapper) {
return new Retrofit.Builder()
.baseUrl(changeableBaseUrl)
.client(okHttpClient)
.addConverterFactory(JacksonConverterFactory.create(objectMapper))
.addCallAdapterFactory(RxJavaCallAdapterFactory.create())
.validateEagerly(BuildConfig.DEBUG) // Fail early: check Retrofit configuration at creation time in Debug build.
.build()
.create(QualityMattersRestApi.class);
.baseUrl(baseUrl)
.client(okHttpClient)
.addConverterFactory(JacksonConverterFactory.create(objectMapper))
.addCallAdapterFactory(RxJavaCallAdapterFactory.create())
.validateEagerly(BuildConfig.DEBUG) // Fail early: check Retrofit configuration at creation time in Debug build.
.build();
}
}
30 changes: 0 additions & 30 deletions app/src/main/java/com/artemzin/qualitymatters/api/ChangeableBaseUrl.java
View file
Open in desktop

This file was deleted.

13 changes: 13 additions & 0 deletions app/src/main/java/com/artemzin/qualitymatters/network/ForRestApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.artemzin.qualitymatters.network;

import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import javax.inject.Qualifier;

import static java.lang.annotation.RetentionPolicy.RUNTIME;

@Documented
@Qualifier
@Retention(RUNTIME)
public @interface ForRestApi {
}
54 changes: 54 additions & 0 deletions app/src/main/java/com/artemzin/qualitymatters/network/HostSelectionInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package com.artemzin.qualitymatters.network;

import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;
import okhttp3.HttpUrl;
import okhttp3.Interceptor;
import okhttp3.Request;
import okhttp3.Response;

/**
* Such implementation allows us easily change base url in the integration and functional tests!
*/
public class HostSelectionInterceptor implements Interceptor {

@Nullable
private volatile String baseUrl;
Copy link
Owner

Choose a reason for hiding this comment

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

Nullability annotation pls


@Override @NonNull
public Response intercept(@NonNull Chain chain) throws IOException {
Request request = chain.request();
final String baseUrl = this.baseUrl;

if (baseUrl != null) {
request = modifyRequest(request, baseUrl);
Copy link
Owner

Choose a reason for hiding this comment

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

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

I see 2 solutions:

  1. Do not rely on interceptor and recreate retrofit instance (clean solution)
  2. Interceptor should know about real base url and redirect only requests that would hit real base url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
return chain.proceed(request);
}

public void setBaseUrl(@Nullable String baseUrl) {
this.baseUrl = baseUrl;
}

@NonNull
Request modifyRequest(@NonNull final Request request, @Nullable String baseUrl) throws MalformedURLException {
final URL url = new URL(baseUrl);
final HttpUrl newUrl = request.url()
.newBuilder()
.scheme(url.getProtocol())
.host(url.getHost())
.port(url.getPort())
.build();
return request.newBuilder()
.url(newUrl)
.build();
}

@Nullable
public String getBaseUrl() {
Copy link
Owner

Choose a reason for hiding this comment

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

nullability

return baseUrl;
}
}
27 changes: 18 additions & 9 deletions app/src/main/java/com/artemzin/qualitymatters/network/NetworkModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
package com.artemzin.qualitymatters.network;

import android.support.annotation.NonNull;

import java.util.List;

import javax.inject.Singleton;

import dagger.Module;
import dagger.Provides;
import java.util.List;
import javax.inject.Singleton;
import okhttp3.Interceptor;
import okhttp3.OkHttpClient;

Expand All @@ -17,16 +14,28 @@ public class NetworkModule {
@Provides @NonNull @Singleton
public OkHttpClient provideOkHttpClient(@OkHttpInterceptors @NonNull List<Interceptor> interceptors,
@OkHttpNetworkInterceptors @NonNull List<Interceptor> networkInterceptors) {
final OkHttpClient.Builder okHttpBuilder = new OkHttpClient.Builder();
return newOkHttpBuilder(interceptors, networkInterceptors).build();
}

@Provides @NonNull @Singleton @ForRestApi
public OkHttpClient provideOkHttpClientForRestApi(@OkHttpInterceptors @NonNull List<Interceptor> interceptors,
@OkHttpNetworkInterceptors @NonNull List<Interceptor> networkInterceptors,
@NonNull HostSelectionInterceptor hostSelectionInterceptor) {
return newOkHttpBuilder(interceptors, networkInterceptors)
.addInterceptor(hostSelectionInterceptor)
.build();
}

private static OkHttpClient.Builder newOkHttpBuilder(@NonNull List<Interceptor> interceptors, @NonNull List<Interceptor> networkInterceptors) {
final OkHttpClient.Builder okHttpBuilder = new OkHttpClient.Builder();
//noinspection Convert2streamapi
for (Interceptor interceptor : interceptors) {
okHttpBuilder.addInterceptor(interceptor);
}

//noinspection Convert2streamapi
for (Interceptor networkInterceptor : networkInterceptors) {
okHttpBuilder.addNetworkInterceptor(networkInterceptor);
}

return okHttpBuilder.build();
return okHttpBuilder;
}
}
10 changes: 8 additions & 2 deletions app/src/release/java/com/artemzin/qualitymatters/network/OkHttpInterceptorsModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,22 @@
import okhttp3.Interceptor;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;

/**
* Provides OkHttp interceptors for release build.
*/
@Module
public class OkHttpInterceptorsModule {

@Provides @Singleton @NonNull
public HostSelectionInterceptor provideHostSelectionInterceptor() {
return new HostSelectionInterceptor();
}

@Provides @OkHttpInterceptors @Singleton @NonNull
public List<Interceptor> provideOkHttpInterceptors() {
return emptyList();
public List<Interceptor> provideOkHttpInterceptors(@NonNull HostSelectionInterceptor hostSelectionInterceptor) {
return singletonList(hostSelectionInterceptor);
}

@Provides @OkHttpNetworkInterceptors @Singleton @NonNull
Expand Down
23 changes: 0 additions & 23 deletions app/src/unitTests/java/com/artemzin/qualitymatters/api/ChangeableBaseUrlTest.java
View file
Open in desktop

This file was deleted.

Loading