Skip to content

Issue-6 Support custom interceptors #7

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

Closed
gldiazcardenas wants to merge 3 commits into from
Closed

Issue-6 Support custom interceptors #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1757a96
Issue-6 Support custom interceptors
gldiazcardenas Dec 7, 2023
93f03d2
Added test to verify custom interceptor
gldiazcardenas Dec 7, 2023
6ab177e
Minor comment
gldiazcardenas Dec 7, 2023
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
14 changes: 13 additions & 1 deletion src/main/java/com/taboola/rest/api/RestAPIClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package com.taboola.rest.api;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;

import okhttp3.Interceptor;

@vladimanaev vladimanaev Dec 15, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

best to avoid coupling to okhttp3 so I prefer creating our own dedicated Interceptor interface

@gldiazcardenas gldiazcardenas Dec 18, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see, good point.

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

Expand Down Expand Up @@ -74,6 +76,7 @@ public static class RestAPIClientBuilder {
private ObjectMapper objectMapper;
private final StringResponseFactories stringResponseFactories = new StringResponseFactories();
private HttpLoggingLevel loggingLevel;
private List<Interceptor> interceptors;

public RestAPIClientBuilder setLoggingLevel(HttpLoggingLevel loggingLevel) {
this.loggingLevel = loggingLevel;
Expand Down Expand Up @@ -155,12 +158,21 @@ public RestAPIClientBuilder addStringBodyResponseFactory(Class<?> clazz, StringR
return this;
}

public RestAPIClientBuilder setInterceptors(Interceptor... interceptors) {
return setInterceptors(Arrays.asList(interceptors));
}

public RestAPIClientBuilder setInterceptors(List<Interceptor> interceptors) {
this.interceptors = interceptors;
return this;
}

public RestAPIClient build() {
organizeState();
String finalUserAgent = String.format("%s/%s/%s (%s)", userAgentPrefix, restAPIVersion, VERSION, userAgentSuffix);
Collection<RequestHeader> headers = getAllHeaders(this.headers, finalUserAgent);
CommunicationConfig config = new CommunicationConfig(baseUrl, connectionTimeoutMillis, readTimeoutMillis, writeTimeoutMillis, maxIdleConnections,
keepAliveDurationMillis, headers, debug, exceptionFactory, objectMapper, stringResponseFactories, loggingLevel);
keepAliveDurationMillis, headers, debug, exceptionFactory, objectMapper, stringResponseFactories, loggingLevel, interceptors);
return new RestAPIClient(new CommunicationFactory(config));
}

Expand Down
11 changes: 8 additions & 3 deletions src/main/java/com/taboola/rest/api/internal/CommunicationFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,20 @@ private Retrofit.Builder createRetrofitBuilder(CommunicationConfig config) {
}

private OkHttpClient createOkHttpClient(CommunicationConfig config) {
return new OkHttpClient.Builder()
OkHttpClient.Builder builder = new OkHttpClient.Builder()
.addInterceptor(new HeadersInterceptor(config.getHeaders()))
.addInterceptor(createLoggingInterceptor(config))
.readTimeout(config.getReadTimeoutMillis(), TimeUnit.MILLISECONDS)
.writeTimeout(config.getWriteTimeoutMillis(), TimeUnit.MILLISECONDS)
.connectTimeout(config.getConnectionTimeoutMillis(), TimeUnit.MILLISECONDS)
.connectionPool(new ConnectionPool(config.getMaxIdleConnections(),
config.getKeepAliveDurationMillis(), TimeUnit.MILLISECONDS))
.build();
config.getKeepAliveDurationMillis(), TimeUnit.MILLISECONDS));

if (config.getInterceptors() != null) {
config.getInterceptors().forEach(builder::addInterceptor);

@vladimanaev vladimanaev Dec 15, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I afraid of such a wide access to API protocol, I thought better to add some "safe" AroundAspect like interceptor which will be safe not to break API calls but allow different before/after code parts.

@gldiazcardenas gldiazcardenas Dec 18, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

When do you think the aspect oriented flows would be in place? I wonder how it will be implemented.

@vladimanaev vladimanaev Dec 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe meanwhile we can create something simple to allow interceptors but limit them to specific code parts like "beforeCall", "afterCall" you can do by creating dedicated interface and create one general interceptor that run list of such interfaces and let developer use those which will both limit the exposure of internal flows vs give enough abilities to save things or log or etc. WDYT?

@gldiazcardenas gldiazcardenas Dec 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think it is fine, as long as the new interface provides access to the request and response.. if it does not, then this dummy interceptor might be useless. so basically could be an interface like:

public interface Interceptor {

beforeCall(Request);

afterCall(Request, Response);

}

WDYT?

@vladimanaev vladimanaev Dec 20, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can provide both request and response data we just need to plan what response should contain, for example raw Retrofit response is problematic since you dont know which call adapter is handling it since interceptors happening before call adapter.
So sound like we need to handle "before" inside the interceptor wrapper but response to provide part of SynchronousCallAdapterFactory or StringConverterFactory.

For example I think that those provided "before/after" code injection parts can not manipulate request nor response so both request and response references should be immutable.

}

return builder.build();
}


Expand Down
18 changes: 18 additions & 0 deletions src/main/java/com/taboola/rest/api/internal/config/CommunicationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@


import java.util.Collection;
import java.util.List;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.taboola.rest.api.exceptions.factories.ExceptionFactory;
import com.taboola.rest.api.internal.StringResponseFactories;
import com.taboola.rest.api.model.HttpLoggingLevel;
import com.taboola.rest.api.model.RequestHeader;
import okhttp3.Interceptor;

/**
* Created by vladi
Expand All @@ -29,11 +31,22 @@ public class CommunicationConfig {
private final ObjectMapper objectMapper;
private final StringResponseFactories stringResponseFactories;
private final HttpLoggingLevel loggingLevel;
private final List<Interceptor> interceptors;

public CommunicationConfig(String baseUrl, Long connectionTimeoutMillis, Long readTimeoutMillis,
Long writeTimeoutMillis, Integer maxIdleConnections, Long keepAliveDurationMillis,
Collection<RequestHeader> headers, boolean debug, ExceptionFactory exceptionFactory,
ObjectMapper objectMapper, StringResponseFactories stringResponseFactories, HttpLoggingLevel loggingLevel) {
// This one here for backward compatibility
this(baseUrl, connectionTimeoutMillis, readTimeoutMillis, writeTimeoutMillis, maxIdleConnections, keepAliveDurationMillis,
headers, debug, exceptionFactory, objectMapper, stringResponseFactories, loggingLevel, null);
}

public CommunicationConfig(String baseUrl, Long connectionTimeoutMillis, Long readTimeoutMillis,
Long writeTimeoutMillis, Integer maxIdleConnections, Long keepAliveDurationMillis,
Collection<RequestHeader> headers, boolean debug, ExceptionFactory exceptionFactory,
ObjectMapper objectMapper, StringResponseFactories stringResponseFactories,
HttpLoggingLevel loggingLevel, List<Interceptor> interceptors) {
this.baseUrl = baseUrl;
this.connectionTimeoutMillis = connectionTimeoutMillis;
this.readTimeoutMillis = readTimeoutMillis;
Expand All @@ -46,6 +59,7 @@ public CommunicationConfig(String baseUrl, Long connectionTimeoutMillis, Long re
this.objectMapper = objectMapper;
this.stringResponseFactories = stringResponseFactories;
this.loggingLevel = loggingLevel;
this.interceptors = interceptors;
}

public String getBaseUrl() {
Expand Down Expand Up @@ -96,6 +110,10 @@ public HttpLoggingLevel getLoggingLevel() {
return loggingLevel;
}

public List<Interceptor> getInterceptors() {
return interceptors;
}

@Override
public String toString() {
return "CommunicationConfig{" +
Expand Down
45 changes: 42 additions & 3 deletions src/test/java/com/taboola/rest/api/RestAPIClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
package com.taboola.rest.api;

import com.taboola.rest.api.exceptions.RestAPIException;
import okhttp3.Interceptor;
import okhttp3.MediaType;
import okhttp3.Protocol;
import okhttp3.Response;
import okhttp3.ResponseBody;
import org.junit.Test;
import org.mockito.Mockito;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
import retrofit2.http.GET;
import retrofit2.http.Header;
import retrofit2.http.Headers;
import static org.junit.Assert.assertNotNull;

import org.junit.Test;
import java.io.IOException;

import com.taboola.rest.api.exceptions.RestAPIException;
import static org.junit.Assert.assertNotNull;

public class RestAPIClientTest {

Expand All @@ -23,4 +32,34 @@ public void testBuild_whenEmptyBuilder_expectingClientBuilt() {
assertNotNull("Missing rest client instance", restAPIClient);
assertNotNull("Failed to generate endpoint", restAPIClient.createRetrofitEndpoint(TestEndpoint.class));
}

@Test
public void testRun_WithCustomInterceptor_ExpectInterceptorCalled() throws IOException {
Interceptor customInterceptor = Mockito.mock(Interceptor.class);

Mockito.when(customInterceptor.intercept(Mockito.any()))
.then((Answer<Response>) invocationOnMock -> {
Interceptor.Chain chain = invocationOnMock.getArgumentAt(0, Interceptor.Chain.class);
return new Response.Builder()
.request(chain.request())
.code(200)
.protocol(Protocol.HTTP_1_1)
.message("OK")
.body(ResponseBody.create(MediaType.parse("application/json"), "{\"test\":\"test\"}"))
.build();
});

RestAPIClient restAPIClient = RestAPIClient.builder().setBaseUrl("http://example.com/")
.setInterceptors(customInterceptor)
.build();

assertNotNull("Missing rest client instance", restAPIClient);

TestEndpoint endpoint = restAPIClient.createRetrofitEndpoint(TestEndpoint.class);
assertNotNull("Failed to generate endpoint", endpoint);

endpoint.getTestObject("test");

Mockito.verify(customInterceptor, Mockito.times(1)).intercept(Mockito.any());
}
}