From 2fd4ce9c246f951d172d8eb04a2bc02a1213d2bb Mon Sep 17 00:00:00 2001 From: Fanis Tharropoulos Date: Mon, 16 Sep 2024 12:05:55 +0300 Subject: [PATCH 1/5] fix(api-call): handle all java.net exception cases on request fail --- src/main/java/org/typesense/api/ApiCall.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/typesense/api/ApiCall.java b/src/main/java/org/typesense/api/ApiCall.java index 25be36e..42c4b3c 100644 --- a/src/main/java/org/typesense/api/ApiCall.java +++ b/src/main/java/org/typesense/api/ApiCall.java @@ -61,7 +61,7 @@ boolean isDueForHealthCheck(Node node) { // Loops in a round-robin fashion to check for a healthy node and returns it Node getNode() { if (configuration.nearestNode != null) { - if (configuration.nearestNode.isHealthy || isDueForHealthCheck((configuration.nearestNode)) ) { + if (isDueForHealthCheck((configuration.nearestNode)) || configuration.nearestNode.isHealthy) { return configuration.nearestNode; } } @@ -181,8 +181,7 @@ T makeRequest(String endpoint, Q queryParameters, Request.Builder request } catch (Exception e) { boolean handleError = (e instanceof ServerError) || (e instanceof ServiceUnavailable) || - (e instanceof SocketTimeoutException) || - (e instanceof java.net.UnknownHostException) || + (e.getClass().getPackage().getName().startsWith("java.net")) || (e instanceof SSLException); if(!handleError) { From 834d9236ad6a299d3317cf44d1c4d2aa468a0586 Mon Sep 17 00:00:00 2001 From: Fanis Tharropoulos Date: Mon, 16 Sep 2024 12:06:30 +0300 Subject: [PATCH 2/5] feat(api-all): add optional client to class ctr for dep injection --- src/main/java/org/typesense/api/ApiCall.java | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/typesense/api/ApiCall.java b/src/main/java/org/typesense/api/ApiCall.java index 42c4b3c..669244f 100644 --- a/src/main/java/org/typesense/api/ApiCall.java +++ b/src/main/java/org/typesense/api/ApiCall.java @@ -38,6 +38,18 @@ public class ApiCall { public static final MediaType JSON = MediaType.parse("application/json; charset=utf-8"); private final ObjectMapper mapper = new ObjectMapper(); + public ApiCall(Configuration configuration, OkHttpClient client) { + this.configuration = configuration; + this.nodes = configuration.nodes; + this.apiKey = configuration.apiKey; + this.retryInterval = configuration.retryInterval; + + mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL); + mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + + this.client = client; + } + public ApiCall(Configuration configuration) { this.configuration = configuration; this.nodes = configuration.nodes; @@ -48,10 +60,10 @@ public ApiCall(Configuration configuration) { mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); client = new OkHttpClient() - .newBuilder() - .connectTimeout(configuration.connectionTimeout.getSeconds(), TimeUnit.SECONDS) - .readTimeout(configuration.readTimeout.getSeconds(), TimeUnit.SECONDS) - .build(); + .newBuilder() + .connectTimeout(configuration.connectionTimeout.getSeconds(), TimeUnit.SECONDS) + .readTimeout(configuration.readTimeout.getSeconds(), TimeUnit.SECONDS) + .build(); } boolean isDueForHealthCheck(Node node) { From 1194b4e078ba07738f821e5defad7e0880f8bb43 Mon Sep 17 00:00:00 2001 From: Fanis Tharropoulos Date: Mon, 16 Sep 2024 12:06:46 +0300 Subject: [PATCH 3/5] chore: add mockito --- build.gradle | 2 ++ gradle.properties | 1 + 2 files changed, 3 insertions(+) diff --git a/build.gradle b/build.gradle index e850b6a..b4b2252 100644 --- a/build.gradle +++ b/build.gradle @@ -68,6 +68,8 @@ dependencies { implementation group: 'javax.annotation', name: 'javax.annotation-api', version: '1.3.2' testImplementation "org.junit.jupiter:junit-jupiter-api:${junitJupiterVersion}" + testImplementation "org.mockito:mockito-core:${mokitoVerion}" + testImplementation "org.mockito:mockito-junit-jupiter:${mokitoVerion}" testImplementation "org.hamcrest:hamcrest-all:${hamcrestVersion}" testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine:${junitJupiterVersion}" diff --git a/gradle.properties b/gradle.properties index 80fc709..18187ec 100644 --- a/gradle.properties +++ b/gradle.properties @@ -7,6 +7,7 @@ hamcrestVersion=1.3 jacksonCoreVersion=2.14.1 javaxXmlBindVersion=2.3.1 junitJupiterVersion=5.9.3 +mokitoVerion=5.3.1 okhttp3Version=4.10.0 slf4jVersion=2.0.5 swaggerCoreV3Version=2.0.0 From 88d0092c90f428fb93b3d62630bf7c667fcd89fb Mon Sep 17 00:00:00 2001 From: Fanis Tharropoulos Date: Mon, 16 Sep 2024 12:09:31 +0300 Subject: [PATCH 4/5] test(api-call): add tests for java.net exception handling - Refactor existing test suite for `ApiCall` class, accessing the `nodeIndex` static variable, to avoid flaky tests - Add new test cases ensuring proper handling of all `java.net` exceptions --- .../java/org/typesense/api/APICallTest.java | 139 +++++++++++++++--- 1 file changed, 119 insertions(+), 20 deletions(-) diff --git a/src/test/java/org/typesense/api/APICallTest.java b/src/test/java/org/typesense/api/APICallTest.java index 103b27b..d8a487b 100644 --- a/src/test/java/org/typesense/api/APICallTest.java +++ b/src/test/java/org/typesense/api/APICallTest.java @@ -3,43 +3,73 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; import org.typesense.resources.Node; +import okhttp3.Call; +import okhttp3.OkHttpClient; +import okhttp3.Request; + +import java.lang.reflect.Field; +import java.net.ConnectException; import java.time.Duration; import java.util.ArrayList; import java.util.List; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; class APICallTest { + @Mock + private OkHttpClient client; + + @Mock + private Call call; + private ApiCall apiCall; private Node nearestNode; + private List nodes; + + @BeforeEach + void setUp() { + MockitoAnnotations.openMocks(this); + nodes = new ArrayList<>(); + nodes.add(new Node("http", "localhost", "8108")); + nodes.add(new Node("http", "localhost", "7108")); + nodes.add(new Node("http", "localhost", "6108")); + } - void setUpNoNearestNode() throws Exception { - List nodes = new ArrayList<>(); - nodes.add(new Node("http","localhost","8108")); - nodes.add(new Node("http","localhost","7108")); - nodes.add(new Node("http","localhost","6108")); - apiCall = new ApiCall(new Configuration(nodes, Duration.ofSeconds(3),"xyz")); + private void resetNodeIndex() throws Exception { + Field nodeIndexField = ApiCall.class.getDeclaredField("nodeIndex"); + nodeIndexField.setAccessible(true); + nodeIndexField.set(null, 0); } - void setUpNearestNode() throws Exception { - List nodes = new ArrayList<>(); - nodes.add(new Node("http","localhost","8108")); - nodes.add(new Node("http","localhost","7108")); - nodes.add(new Node("http","localhost","6108")); - nearestNode = new Node("http","localhost","0000"); - apiCall = new ApiCall(new Configuration(nearestNode, nodes, Duration.ofSeconds(3),"xyz")); + void setUpNoNearestNode() { + apiCall = new ApiCall(new Configuration(nodes, Duration.ofSeconds(3), "xyz"), client); + } + + void setUpNearestNode() { + nearestNode = new Node("http", "localhost", "0000"); + apiCall = new ApiCall(new Configuration(nearestNode, nodes, Duration.ofSeconds(3), "xyz"), client); } @AfterEach void tearDown() throws Exception { - + nodes = null; + apiCall = null; + resetNodeIndex(); } @Test - void testRoundRobin() throws Exception { + void testRoundRobin() { setUpNoNearestNode(); assertEquals("7108", apiCall.getNode().port); assertEquals("6108", apiCall.getNode().port); @@ -50,27 +80,96 @@ void testRoundRobin() throws Exception { assertEquals("8108", apiCall.getNode().port); } + @Test + void testMakeRequestWithConnectException() throws Exception { + setUpNoNearestNode(); + String endpoint = "/collections"; + Request.Builder requestBuilder = new Request.Builder().get(); + + Call mockCall = mock(Call.class); + when(client.newCall(any(Request.class))).thenReturn(mockCall); + when(mockCall.execute()).thenThrow(new ConnectException()); + + // Act + assertThrows(ConnectException.class, () -> { + apiCall.makeRequest(endpoint, null, requestBuilder, String.class); + }); + + // Additional assertions + nodes.forEach(node -> { + assertEquals(false, node.isHealthy); + }); + + verify(client, times(3)).newCall(any(Request.class)); + verify(mockCall, times(3)).execute(); + } + + @Test + void testMakeRequestWithSocketTimeoutException() throws Exception { + setUpNoNearestNode(); + String endpoint = "/collections"; + Request.Builder requestBuilder = new Request.Builder().get(); + + Call mockCall = mock(Call.class); + when(client.newCall(any(Request.class))).thenReturn(mockCall); + when(mockCall.execute()).thenThrow(new java.net.SocketTimeoutException()); + + // Act + assertThrows(java.net.SocketTimeoutException.class, () -> { + apiCall.makeRequest(endpoint, null, requestBuilder, String.class); + }); + + // Additional assertions + nodes.forEach(node -> { + assertEquals(false, node.isHealthy); + }); + + verify(client, times(3)).newCall(any(Request.class)); + verify(mockCall, times(3)).execute(); + } + + @Test + void testMakeRequestWithUnknownHostException() throws Exception { + setUpNoNearestNode(); + String endpoint = "/collections"; + Request.Builder requestBuilder = new Request.Builder().get(); + + Call mockCall = mock(Call.class); + when(client.newCall(any(Request.class))).thenReturn(mockCall); + when(mockCall.execute()).thenThrow(new java.net.UnknownHostException()); + + // Act + assertThrows(java.net.UnknownHostException.class, () -> { + apiCall.makeRequest(endpoint, null, requestBuilder, String.class); + }); + + // Additional assertions + nodes.forEach(node -> { + assertEquals(false, node.isHealthy); + }); + + verify(client, times(3)).newCall(any(Request.class)); + verify(mockCall, times(3)).execute(); + } @Test - void testUnhealthyNearestNode() throws Exception { + void testUnhealthyNearestNode() { setUpNearestNode(); nearestNode.isHealthy = false; assertEquals("7108", apiCall.getNode().port); } @Test - void testHealthyNearestNode() throws Exception { + void testHealthyNearestNode() { setUpNearestNode(); assertEquals("0000", apiCall.getNode().port); } @Test - void testUnhealthyNearestNodeDueForHealthCheck() throws Exception { + void testUnhealthyNearestNodeDueForHealthCheck() { setUpNearestNode(); nearestNode.isHealthy = false; nearestNode.lastAccessTimestamp = nearestNode.lastAccessTimestamp.minusSeconds(63); assertEquals("0000", apiCall.getNode().port); } - - } \ No newline at end of file From 323a5f414b1c98e1f625cffcd7fbbf760c1ef2dc Mon Sep 17 00:00:00 2001 From: Fanis Tharropoulos Date: Mon, 16 Sep 2024 12:47:23 +0300 Subject: [PATCH 5/5] chore: downgrade mockito ver for java 8 compatibility --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 18187ec..5d3b2d4 100644 --- a/gradle.properties +++ b/gradle.properties @@ -7,7 +7,7 @@ hamcrestVersion=1.3 jacksonCoreVersion=2.14.1 javaxXmlBindVersion=2.3.1 junitJupiterVersion=5.9.3 -mokitoVerion=5.3.1 +mokitoVerion=4.11.0 okhttp3Version=4.10.0 slf4jVersion=2.0.5 swaggerCoreV3Version=2.0.0