Skip to content

- fixes retry/redirect options customization #183

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

Merged
baywet merged 4 commits into from
Apr 14, 2021
Merged

- fixes retry/redirect options customization #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
118dc26
- fixes retry/redirect options customization
Asky-GH Apr 7, 2021
d86d453
- fixes retry/redirect options customization
Asky-GH Apr 8, 2021
804f36b
- adds unit tests for default retry and redirect options values
baywet Apr 14, 2021
ba0d5fd
- adds unit test on getHttpReqest method to test for default retry an…
baywet Apr 14, 2021
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
4 changes: 2 additions & 2 deletions src/main/java/com/microsoft/graph/http/BaseCollectionRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ public void setShouldRedirect(@Nonnull IShouldRedirect shouldRedirect) {
*
* @return Callback which is called before redirect
*/
@Nullable
@Nonnull
public IShouldRedirect getShouldRedirect() {
return baseRequest.getShouldRedirect();
}
Expand All @@ -377,7 +377,7 @@ public void setShouldRetry(@Nonnull IShouldRetry shouldretry) {
*
* @return Callback called before retry
*/
@Nullable
@Nonnull
public IShouldRetry getShouldRetry() {
return baseRequest.getShouldRetry();
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/microsoft/graph/http/BaseRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ public void setShouldRedirect(@Nonnull IShouldRedirect shouldRedirect) {
*
* @return Callback which is called before redirect
*/
@Nullable
@Nonnull
public IShouldRedirect getShouldRedirect() {
return shouldRedirect;
}
Expand All @@ -557,7 +557,7 @@ public void setShouldRetry(@Nonnull IShouldRetry shouldretry) {
*
* @return Callback called before retry
*/
@Nullable
@Nonnull
public IShouldRetry getShouldRetry() {
return shouldRetry;
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/microsoft/graph/http/BaseStreamRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ public void setShouldRedirect(@Nonnull final IShouldRedirect shouldRedirect) {
*
* @return Callback which is called before redirect
*/
@Nullable
@Nonnull
public IShouldRedirect getShouldRedirect() {
return baseRequest.getShouldRedirect();
}
Expand All @@ -244,7 +244,7 @@ public void setShouldRetry(@Nonnull final IShouldRetry shouldretry) {
*
* @return Callback called before retry
*/
@Nullable
@Nonnull
public IShouldRetry getShouldRetry() {
return baseRequest.getShouldRetry();
}
Expand Down
16 changes: 14 additions & 2 deletions src/main/java/com/microsoft/graph/http/CoreHttpProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@
import okhttp3.ResponseBody;
import okio.BufferedSink;

import static com.microsoft.graph.httpcore.middlewareoption.RedirectOptions.DEFAULT_MAX_REDIRECTS;
import static com.microsoft.graph.httpcore.middlewareoption.RedirectOptions.DEFAULT_SHOULD_REDIRECT;
import static com.microsoft.graph.httpcore.middlewareoption.RetryOptions.DEFAULT_DELAY;
import static com.microsoft.graph.httpcore.middlewareoption.RetryOptions.DEFAULT_MAX_RETRIES;
import static com.microsoft.graph.httpcore.middlewareoption.RetryOptions.DEFAULT_SHOULD_RETRY;

/**
* HTTP provider based off of OkHttp and msgraph-sdk-java-core library
*/
Expand Down Expand Up @@ -236,8 +242,14 @@ public <Result, Body> Request getHttpRequest(@Nonnull final IHttpRequest request
logger.logDebug("Starting to send request, URL " + requestUrl.toString());

// Request level middleware options
final RedirectOptions redirectOptions = request.getMaxRedirects() <= 0 ? null : new RedirectOptions(request.getMaxRedirects(), request.getShouldRedirect());
final RetryOptions retryOptions = request.getShouldRetry() == null ? null : new RetryOptions(request.getShouldRetry(), request.getMaxRetries(), request.getDelay());
final RedirectOptions redirectOptions =
request.getMaxRedirects() == DEFAULT_MAX_REDIRECTS && request.getShouldRedirect().equals(DEFAULT_SHOULD_REDIRECT)
? null
: new RedirectOptions(request.getMaxRedirects(), request.getShouldRedirect());
final RetryOptions retryOptions =
request.getMaxRetries() == DEFAULT_MAX_RETRIES && request.getDelay() == DEFAULT_DELAY && request.getShouldRetry().equals(DEFAULT_SHOULD_RETRY)
? null
: new RetryOptions(request.getShouldRetry(), request.getMaxRetries(), request.getDelay());
Comment on lines +245 to +252
Copy link
Contributor

@zengin zengin Apr 8, 2021

Choose a reason for hiding this comment

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

Is this required for giving the flexibility to custom handler implementations?

In the default implementation of RetryHandler and RedirectHandler, the constructors end up calling the default option constructors if they are null. Default constructors of options are already using these default values. This piece of code seems equivalent to:

Suggested change
final RedirectOptions redirectOptions =
request.getMaxRedirects() == DEFAULT_MAX_REDIRECTS && request.getShouldRedirect().equals(DEFAULT_SHOULD_REDIRECT)
? null
: new RedirectOptions(request.getMaxRedirects(), request.getShouldRedirect());
final RetryOptions retryOptions =
request.getMaxRetries() == DEFAULT_MAX_RETRIES && request.getDelay() == DEFAULT_DELAY && request.getShouldRetry().equals(DEFAULT_SHOULD_RETRY)
? null
: new RetryOptions(request.getShouldRetry(), request.getMaxRetries(), request.getDelay());
final RedirectOptions redirectOptions = new RedirectOptions(request.getMaxRedirects(), request.getShouldRedirect());
final RetryOptions retryOptions = new RetryOptions(request.getShouldRetry(), request.getMaxRetries(), request.getDelay());

Copy link
Contributor Author

@Asky-GH Asky-GH Apr 11, 2021

Choose a reason for hiding this comment

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

Hi @zengin
The changes you are suggesting work effectively the same as the current code of the library. If you take a look at CoreHttpProvider's line 245:

if(redirectOptions != null) {
	corehttpRequestBuilder = corehttpRequestBuilder.tag(RedirectOptions.class, redirectOptions);
}
if(retryOptions != null) {
	corehttpRequestBuilder = corehttpRequestBuilder.tag(RetryOptions.class, retryOptions);
}

RedirectHandler's line 133:

RedirectOptions redirectOptions = request.tag(RedirectOptions.class);
redirectOptions = redirectOptions != null ? redirectOptions : this.mRedirectOptions;

and RetryHandler's line 179:

RetryOptions retryOption = request.tag(RetryOptions.class);
retryOption = retryOption != null ? retryOption : mRetryOption;

you can see that mRedirectOptions and mRetryOption will never be used. Thus it is impossible to customize them so to speak globaly.

Copy link
Contributor

@zengin zengin Apr 12, 2021

Choose a reason for hiding this comment

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

Thanks for clarifying.


final Request coreHttpRequest = convertIHttpRequestToOkHttpRequest(request);
Request.Builder corehttpRequestBuilder = coreHttpRequest
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/microsoft/graph/http/IHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public interface IHttpRequest {
*
* @return Callback which is called before redirect
*/
@Nullable
@Nonnull
IShouldRedirect getShouldRedirect();

/**
Expand All @@ -134,7 +134,7 @@ public interface IHttpRequest {
*
* @return Callback called before retry
*/
@Nullable
@Nonnull
IShouldRetry getShouldRetry();

/**
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/microsoft/graph/httpcore/middlewareoption/RetryOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public RetryOptions(@Nullable final IShouldRetry shouldRetry, int maxRetries, lo
if(maxRetries < 0)
throw new IllegalArgumentException("Max retries cannot be negative");

this.mShouldretry = shouldRetry != null ? shouldRetry : DEFAULT_SHOULD_RETRY;
this.mShouldretry = shouldRetry == null ? DEFAULT_SHOULD_RETRY : shouldRetry;
this.mMaxRetries = maxRetries;
this.mDelay = delay;
}
Expand Down
89 changes: 89 additions & 0 deletions src/test/java/com/microsoft/graph/http/CoreHttpProviderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@

import com.microsoft.graph.core.GraphErrorCodes;
import com.microsoft.graph.core.IBaseClient;
import com.microsoft.graph.httpcore.middlewareoption.IShouldRedirect;
import com.microsoft.graph.httpcore.middlewareoption.IShouldRetry;
import com.microsoft.graph.httpcore.middlewareoption.RedirectOptions;
import com.microsoft.graph.httpcore.middlewareoption.RetryOptions;
import com.microsoft.graph.logger.ILogger;
import com.microsoft.graph.logger.LoggerLevel;
import com.microsoft.graph.options.HeaderOption;
Expand All @@ -21,6 +25,7 @@
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.MalformedURLException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand All @@ -37,6 +42,7 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
Expand Down Expand Up @@ -198,5 +204,88 @@ private void setCoreHttpProvider(final Object toSerialize) throws IOException {
mock(ILogger.class),
mClient);
}
@Test
public void getHttpRequestDoesntSetRetryOrRedirectOptionsOnDefaultValues() throws MalformedURLException {
final IHttpRequest absRequest = mock(IHttpRequest.class);
when(absRequest.getRequestUrl()).thenReturn(new URL("https://graph.microsoft.com/v1.0/me"));
when(absRequest.getHttpMethod()).thenReturn(HttpMethod.GET);
final ISerializer serializer = mock(ISerializer.class);
final ILogger logger = mock(ILogger.class);

mProvider = new CoreHttpProvider(serializer,
logger,
new OkHttpClient.Builder().build());

when(absRequest.getMaxRedirects()).thenReturn(RedirectOptions.DEFAULT_MAX_REDIRECTS);
when(absRequest.getShouldRedirect()).thenReturn(RedirectOptions.DEFAULT_SHOULD_REDIRECT);
Request request = mProvider.getHttpRequest(absRequest, String.class, null);
RedirectOptions redirectOptions = request.tag(RedirectOptions.class);

assertNull(redirectOptions);

when(absRequest.getShouldRetry()).thenReturn(RetryOptions.DEFAULT_SHOULD_RETRY);
when(absRequest.getMaxRetries()).thenReturn(RetryOptions.DEFAULT_MAX_RETRIES);
when(absRequest.getDelay()).thenReturn(RetryOptions.DEFAULT_DELAY);

request = mProvider.getHttpRequest(absRequest, String.class, null);
RetryOptions retryOptions = request.tag(RetryOptions.class);

assertNull(retryOptions);
}

@Test
public void getHttpRequestSetsRetryOrRedirectOptionsOnNonDefaultValues() throws MalformedURLException {
final IHttpRequest absRequest = mock(IHttpRequest.class);
when(absRequest.getRequestUrl()).thenReturn(new URL("https://graph.microsoft.com/v1.0/me"));
when(absRequest.getHttpMethod()).thenReturn(HttpMethod.GET);
final ISerializer serializer = mock(ISerializer.class);
final ILogger logger = mock(ILogger.class);

mProvider = new CoreHttpProvider(serializer,
logger,
new OkHttpClient.Builder().build());

// testing all pairs to cover all branches
when(absRequest.getMaxRedirects()).thenReturn(RedirectOptions.DEFAULT_MAX_REDIRECTS -1);
when(absRequest.getShouldRedirect()).thenReturn(mock(IShouldRedirect.class));
Request request = mProvider.getHttpRequest(absRequest, String.class, null);
RedirectOptions redirectOptions = request.tag(RedirectOptions.class);

assertNotNull(redirectOptions);

when(absRequest.getMaxRedirects()).thenReturn(RedirectOptions.DEFAULT_MAX_REDIRECTS);
when(absRequest.getShouldRedirect()).thenReturn(mock(IShouldRedirect.class));
request = mProvider.getHttpRequest(absRequest, String.class, null);
redirectOptions = request.tag(RedirectOptions.class);

assertNotNull(redirectOptions);

// testing all pairs to cover all branches
when(absRequest.getShouldRetry()).thenReturn(mock(IShouldRetry.class));
when(absRequest.getMaxRetries()).thenReturn(RetryOptions.DEFAULT_MAX_RETRIES-1);
when(absRequest.getDelay()).thenReturn(RetryOptions.DEFAULT_DELAY-1);

request = mProvider.getHttpRequest(absRequest, String.class, null);
RetryOptions retryOptions = request.tag(RetryOptions.class);

assertNotNull(retryOptions);

when(absRequest.getShouldRetry()).thenReturn(mock(IShouldRetry.class));
when(absRequest.getMaxRetries()).thenReturn(RetryOptions.DEFAULT_MAX_RETRIES);
when(absRequest.getDelay()).thenReturn(RetryOptions.DEFAULT_DELAY-1);

request = mProvider.getHttpRequest(absRequest, String.class, null);
retryOptions = request.tag(RetryOptions.class);

assertNotNull(retryOptions);

when(absRequest.getShouldRetry()).thenReturn(mock(IShouldRetry.class));
when(absRequest.getMaxRetries()).thenReturn(RetryOptions.DEFAULT_MAX_RETRIES);
when(absRequest.getDelay()).thenReturn(RetryOptions.DEFAULT_DELAY);

request = mProvider.getHttpRequest(absRequest, String.class, null);
retryOptions = request.tag(RetryOptions.class);

assertNotNull(retryOptions);
}
}
16 changes: 16 additions & 0 deletions src/test/java/com/microsoft/graph/httpcore/RedirectHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.microsoft.graph.httpcore;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

Expand Down Expand Up @@ -262,5 +263,20 @@ public void testGetRedirectRelativeLocationRequestURLwithSlash() throws Protocol
String expected = "https://graph.microsoft.com/v1.0/me/testrelativeurl";
assertTrue(request.url().toString().compareTo(expected) == 0);
}
@Test
public void testIsRedirectedIsFalseIfExceedsMaxRedirects() throws ProtocolException, IOException {
RedirectOptions options = new RedirectOptions(0, null);
RedirectHandler redirectHandler = new RedirectHandler(options);
Request httppost = new Request.Builder().url(testmeurl).build();

Response response = new Response.Builder()
.protocol(Protocol.HTTP_1_1)
.code(HttpURLConnection.HTTP_SEE_OTHER)
.message("See Other")
.addHeader("location", "/testrelativeurl")
.request(httppost)
.build();
boolean isRedirected = redirectHandler.isRedirected(httppost, response, 1, options);
assertFalse(isRedirected);
}
}
33 changes: 33 additions & 0 deletions src/test/java/com/microsoft/graph/httpcore/middlewareoption/RedirectOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package com.microsoft.graph.httpcore.middlewareoption;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;

import org.junit.jupiter.api.Test;

import okhttp3.Response;

public class RedirectOptionsTest {
@Test
public void constructorDefensiveProgramming() {
assertThrows(IllegalArgumentException.class, () -> {
new RedirectOptions(RedirectOptions.MAX_REDIRECTS +1, null);
});

assertThrows(IllegalArgumentException.class, () -> {
new RedirectOptions(-1, null);
});
}
@Test
public void defaultShouldRedirectValue() {
RedirectOptions options = new RedirectOptions();
assertEquals(options.shouldRedirect(), RedirectOptions.DEFAULT_SHOULD_REDIRECT);
}
@Test
public void defaultShouldRedirectIsTrue() {
Response response = mock(Response.class);
assertTrue(RedirectOptions.DEFAULT_SHOULD_REDIRECT.shouldRedirect(response));
}
}
32 changes: 32 additions & 0 deletions src/test/java/com/microsoft/graph/httpcore/middlewareoption/RetryOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.microsoft.graph.httpcore.middlewareoption;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import org.junit.jupiter.api.Test;

public class RetryOptionsTest {
@Test
public void constructorDefensiveProgramming() {
assertThrows(IllegalArgumentException.class, () -> {
new RetryOptions(null, RetryOptions.MAX_RETRIES +1, 0);
});

assertThrows(IllegalArgumentException.class, () -> {
new RetryOptions(null, RetryOptions.MAX_RETRIES, RetryOptions.MAX_DELAY +1);
});

assertThrows(IllegalArgumentException.class, () -> {
new RetryOptions(null, RetryOptions.MAX_RETRIES, -1);
});

assertThrows(IllegalArgumentException.class, () -> {
new RetryOptions(null, -1, RetryOptions.MAX_DELAY);
});
}
@Test
public void defaultShouldRetryValue() {
RetryOptions options = new RetryOptions();
assertEquals(options.shouldRetry(), RetryOptions.DEFAULT_SHOULD_RETRY);
}
}