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.

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

Avoid to have unexpected exception on status code 429 TooManyRequests #433

Merged
merged 7 commits into from
May 1, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,4 @@ dependencies {
tasks.named('test') { task ->
useJUnitPlatform()
}
targetCompatibility = JavaVersion.VERSION_1_8
Ifropc marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 9 additions & 1 deletion src/main/java/org/stellar/sdk/requests/ResponseHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import org.stellar.sdk.responses.TypedResponse;

import java.io.IOException;
import java.util.Optional;

import okhttp3.Response;

Expand All @@ -28,7 +29,14 @@ public T handleResponse(final Response response) throws IOException, TooManyRequ
try {
// Too Many Requests
if (response.code() == 429) {
int retryAfter = Integer.parseInt(response.header("Retry-After"));

Optional<Integer> retryAfter = Optional.empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

per other comment related to not using Optional for inputs, retryAfter can just be Integer here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on that? I personally prefer to work with Optional classes everywhere, instead of nullable types. To avoid NPE by mistake somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of the input parameter, either provide a value or none, can be accomplished with standard sdk, use an object(boxed primitives) and null as absent or do overloaded methods, using Optional as input parameter just adds extra layers that don't provide anything more, now you have to null check/guard on the optional first before referencing it to see if it's value is present/absent, it's duplicative. Optionals provide useful meta for return values, but not as input params. google java optional misuse to see more on this line of thought.

String header = response.header("Retry-After");
if (header != null) {
try {
retryAfter = Optional.of(Integer.parseInt(header));
} catch (NumberFormatException ignored) {}
}
throw new TooManyRequestsException(retryAfter);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,24 @@
package org.stellar.sdk.requests;

import java.util.Optional;

/**
* Exception thrown when too many requests were sent to the Horizon server.
* @see <a href="https://developers.stellar.org/api/introduction/rate-limiting/" target="_blank">Rate Limiting</a>
*/
public class TooManyRequestsException extends RuntimeException {
private int retryAfter;
private Optional<Integer> retryAfter;

public TooManyRequestsException(int retryAfter) {
public TooManyRequestsException(Optional<Integer> retryAfter) {
super("The rate limit for the requesting IP address is over its alloted limit.");
this.retryAfter = retryAfter;
}

/**
* Returns number of seconds a client should wait before sending requests again.
* Returns number of seconds a client should wait before sending requests again,
* or -1 this time is unknown.
*/
public int getRetryAfter() {
public Optional<Integer> getRetryAfter() {
return retryAfter;
}
}
66 changes: 66 additions & 0 deletions src/test/java/org/stellar/sdk/requests/ResponseHandlerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package org.stellar.sdk.requests;

import static org.junit.Assert.assertEquals;

import java.io.IOException;
import java.util.Optional;

import org.junit.Assert;
import org.junit.Test;

import okhttp3.OkHttpClient;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;

public class ResponseHandlerTest {

@Test
public void testTooManyRequests() throws IOException, InterruptedException {

MockResponse response = new MockResponse();
response.setResponseCode(429);
response.setHeader("Retry-After", "10");

MockWebServer mockWebServer = new MockWebServer();
mockWebServer.start();
mockWebServer.enqueue(response);

OkHttpClient okHttpClient = new OkHttpClient().newBuilder().build();
try {

AccountsRequestBuilder.execute(okHttpClient, mockWebServer.url("/"));
Assert.fail();
} catch (TooManyRequestsException tmre) {
assertEquals(Optional.of(10), tmre.getRetryAfter());
} finally {

mockWebServer.shutdown();
mockWebServer.close();
}
}

@Test
public void testTooManyRequestsNoHeader() throws IOException, InterruptedException {

MockResponse response = new MockResponse();
response.setResponseCode(429);

MockWebServer mockWebServer = new MockWebServer();

mockWebServer.start();
mockWebServer.enqueue(response);

OkHttpClient okHttpClient = new OkHttpClient().newBuilder().build();

try {
AccountsRequestBuilder.execute(okHttpClient, mockWebServer.url("/"));
Assert.fail();
} catch (TooManyRequestsException tmre) {
assertEquals(Optional.empty(), tmre.getRetryAfter());
} finally {

mockWebServer.shutdown();
mockWebServer.close();
}
}
}