Merge pull request #129 from cloudsufi/cherry-pick/GA_fixes

[🍒] [PLUGIN-1927] Map fields ending with `_stc` to string type [PLUGIN-1928] Set connect & read timeout to http client and retry ConnectTimeoutException & SocketException

Author: sgarg-CS

src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowAPIException.java

@@ -4,8 +4,10 @@
 
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpStatus;
+import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.oltu.oauth2.common.exception.OAuthSystemException;
 
+import java.net.SocketException;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
@@ -65,6 +67,8 @@ public boolean isErrorRetryable() {
     }
     Throwable t = this.getCause();
     return t instanceof OAuthSystemException
+        || t instanceof ConnectTimeoutException
+        || t instanceof SocketException
         || (this.getMessage() != null
         && this.getMessage().contains(ServiceNowConstants.MAXIMUM_EXECUTION_TIME_EXCEEDED))
         || RETRYABLE_CODES.contains(getStatusCode());

src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java

@@ -65,6 +65,8 @@
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
 
+import static io.cdap.plugin.servicenow.util.ServiceNowConstants.STC_FIELD_SUFFIX;
+
 /**
  * Implementation class for ServiceNow Table API.
  */
@@ -393,9 +395,14 @@ private Schema prepareSchemaWithMetadataAPI(RestAPIResponse restAPIResponse, Lis
     }
 
     for (MetadataAPISchemaField field : metadataAPISchemaResponse.getResult().getColumns().values()) {
-      if (valueType.equals(SourceValueType.SHOW_DISPLAY_VALUE) &&
-        !Objects.equals(field.getType(), field.getInternalType())) {
-        columns.add(new ServiceNowColumn(field.getName(), field.getType()));
+      if (valueType.equals(SourceValueType.SHOW_DISPLAY_VALUE)) {
+        if (!Objects.equals(field.getType(), field.getInternalType())) {
+          columns.add(new ServiceNowColumn(field.getName(), field.getType()));
+        } else if (field.getName().endsWith(STC_FIELD_SUFFIX) && field.getType().equals("integer")) {
+          columns.add(new ServiceNowColumn(field.getName(), Schema.Type.STRING.name()));
+        } else {
+          columns.add(new ServiceNowColumn(field.getName(), field.getInternalType()));
+        }
       } else if (valueType.equals(SourceValueType.SHOW_ACTUAL_VALUE) &&
         GLIDE_TIME_DATATYPE.equalsIgnoreCase(field.getInternalType())) {
         columns.add(new ServiceNowColumn(field.getName(), GLIDE_DATE_TIME_DATATYPE));

src/main/java/io/cdap/plugin/servicenow/restapi/RestAPIClient.java

@@ -24,9 +24,11 @@
 import com.github.rholder.retry.WaitStrategies;
 import io.cdap.plugin.servicenow.apiclient.ServiceNowAPIException;
 import io.cdap.plugin.servicenow.util.ServiceNowConstants;
+import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
+import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.oltu.oauth2.client.OAuthClient;
@@ -41,6 +43,8 @@
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.net.SocketException;
+import java.util.Collections;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -50,6 +54,17 @@
  */
 public abstract class RestAPIClient {
   private static final Logger LOG = LoggerFactory.getLogger(RestAPIClient.class);
+  
+  /* Connect Timout in ms for establishing the conenction with the server */
+  private static final int DEFAULT_CONNECT_TIMEOUT_MS = 120000;
+
+  /* Read Timeout in ms for waiting for data after the connection is established */
+  private static final int DEFAULT_READ_TIMEOUT_MS = 300000;
+
+  private static final RequestConfig requestConfig = RequestConfig.custom()
+    .setConnectTimeout(DEFAULT_CONNECT_TIMEOUT_MS)
+    .setSocketTimeout(DEFAULT_READ_TIMEOUT_MS)
+    .build();
 
   /**
    * Executes the Rest API request and returns the response.
@@ -61,10 +76,13 @@ public RestAPIResponse executeGet(RestAPIRequest request) throws IOException {
     HttpGet httpGet = new HttpGet(request.getUrl());
     request.getHeaders().entrySet().forEach(e -> httpGet.addHeader(e.getKey(), e.getValue()));
 
-    try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) {
+    try (CloseableHttpClient httpClient = HttpClientBuilder.create().setDefaultRequestConfig(requestConfig).build()) {
       try (CloseableHttpResponse httpResponse = httpClient.execute(httpGet)) {
         return RestAPIResponse.parse(httpResponse, request.getResponseHeaders());
       }
+    } catch (ConnectTimeoutException | SocketException e) {
+      ServiceNowAPIException exception = new ServiceNowAPIException(e, null);
+      return new RestAPIResponse(Collections.emptyMap(), null, exception);
     }
   }
 
@@ -136,7 +154,7 @@ public RestAPIResponse executePost(RestAPIRequest request) throws IOException {
     // We're retrying all transport exceptions while executing the HTTP POST method and the generic transport
     // exceptions in HttpClient are represented by the standard java.io.IOException class
     // https://hc.apache.org/httpclient-legacy/exception-handling.html
-    try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) {
+    try (CloseableHttpClient httpClient = HttpClientBuilder.create().setDefaultRequestConfig(requestConfig).build()) {
       try (CloseableHttpResponse httpResponse = httpClient.execute(httpPost)) {
         return RestAPIResponse.parse(httpResponse, request.getResponseHeaders());
       }

src/main/java/io/cdap/plugin/servicenow/util/ServiceNowConstants.java

@@ -281,4 +281,9 @@ public interface ServiceNowConstants {
    * Default Precision supported by ServiceNow Rest API
    */
   int DEFAULT_PRECISION = 20;
+
+  /**
+   * Suffix for ServiceNow fields that store elapsed time in seconds (e.g., calendar_stc, business_stc).
+   */
+  String STC_FIELD_SUFFIX = "_stc";
 }

src/test/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImplTest.java

@@ -187,4 +187,41 @@ public void testFetchTableSchema_DisplayValueType() throws Exception {
     Assert.assertEquals(Schema.Type.STRING,
       schema.getField("user_name").getSchema().getUnionSchemas().get(0).getType());
   }
+
+  @Test
+  public void testFetchTableSchema_StcFieldsWithDisplayValueType_ParseAsString() throws Exception {
+
+    ServiceNowConnectorConfig mockConfig = Mockito.mock(ServiceNowConnectorConfig.class);
+    ServiceNowTableAPIClientImpl impl = new ServiceNowTableAPIClientImpl(mockConfig, true);
+    ServiceNowTableAPIClientImpl implSpy = Mockito.spy(impl);
+    String jsonResponse = "{\n" +
+      "  \"result\": {\n" +
+      "    \"columns\": {\n" +
+      "      \"calendar_stc\": {\n" +
+      "        \"label\": \"Resolve time\",\n" +
+      "        \"type\": \"integer\",\n" +
+      "        \"name\": \"calendar_stc\",\n" +
+      "        \"internal_type\": \"integer\"\n" +
+      "      },\n" +
+      "      \"business_stc\": {\n" +
+      "        \"label\": \"Business resolve time\",\n" +
+      "        \"type\": \"integer\",\n" +
+      "        \"name\": \"business_stc\",\n" +
+      "        \"internal_type\": \"integer\"\n" +
+      "      }\n" +
+      "    }\n" +
+      "  }\n" +
+      "}";
+    RestAPIResponse mockResponse = new RestAPIResponse(Collections.emptyMap(), jsonResponse, null);
+    Mockito.doReturn(mockResponse).when(implSpy).executeGetWithRetries(Mockito.any());
+    Schema schema = implSpy.fetchTableSchema("incident", "dummy-access-token",
+                                             SourceValueType.SHOW_DISPLAY_VALUE, SchemaType.METADATA_API_BASED);
+    Assert.assertNotNull(schema);
+    Assert.assertEquals("record", schema.getDisplayName());
+    Assert.assertEquals(2, schema.getFields().size());
+    Assert.assertEquals(Schema.Type.STRING,
+                        schema.getField("business_stc").getSchema().getUnionSchemas().get(0).getType());
+    Assert.assertEquals(Schema.Type.STRING,
+                        schema.getField("calendar_stc").getSchema().getUnionSchemas().get(0).getType());
+  }
 }

src/test/java/io/cdap/plugin/servicenow/restapi/RestAPIClientTest.java

@@ -6,6 +6,7 @@
 import org.apache.http.HttpStatus;
 import org.apache.http.StatusLine;
 import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.message.BasicStatusLine;
@@ -19,6 +20,7 @@
 import org.powermock.modules.junit4.PowerMockRunner;
 
 import java.io.IOException;
+import java.net.SocketException;
 
 @RunWith(PowerMockRunner.class)
 @PrepareForTest({
@@ -102,4 +104,49 @@ public void testExecuteGet_StatusOk() throws IOException {
     ServiceNowTableAPIClientImpl client = new ServiceNowTableAPIClientImpl(config, true);
     client.executeGet(request);
   }
+
+  @Test
+  public void testExecuteGet_throwConnectTimeoutException_markAsRetryable() throws IOException {
+    CloseableHttpClient httpClient = Mockito.mock(CloseableHttpClient.class);
+    HttpClientBuilder httpClientBuilder = Mockito.mock(HttpClientBuilder.class);
+    PowerMockito.mockStatic(HttpClientBuilder.class);
+    PowerMockito.when(HttpClientBuilder.create()).thenReturn(httpClientBuilder);
+    Mockito.when(httpClientBuilder.build()).thenReturn(httpClient);
+    Mockito.when(httpClient.execute(Mockito.any()))
+      .thenThrow(new ConnectTimeoutException("Connection timed out"));
+
+    ServiceNowTableAPIRequestBuilder builder = new ServiceNowTableAPIRequestBuilder("url");
+    RestAPIRequest request = builder.build();
+
+    ServiceNowConnectorConfig config = Mockito.mock(ServiceNowConnectorConfig.class);
+    ServiceNowTableAPIClientImpl client = new ServiceNowTableAPIClientImpl(config, true);
+    RestAPIResponse actualResponse = client.executeGet(request);
+    Assert.assertNotNull(actualResponse.getException());
+    Assert.assertTrue(actualResponse.getException().isErrorRetryable());
+    Throwable ex = actualResponse.getException().getCause();
+    Assert.assertTrue("Expected ConnectTimeoutException or similar, got: " + ex,
+      ex instanceof ConnectTimeoutException || ex.getMessage().toLowerCase().contains("Connection timed out"));
+  }
+
+  @Test
+  public void testExecuteGet_throwSocketException_markAsRetryable() throws IOException {
+    CloseableHttpClient httpClient = Mockito.mock(CloseableHttpClient.class);
+    HttpClientBuilder httpClientBuilder = Mockito.mock(HttpClientBuilder.class);
+    PowerMockito.mockStatic(HttpClientBuilder.class);
+    PowerMockito.when(HttpClientBuilder.create()).thenReturn(httpClientBuilder);
+    Mockito.when(httpClientBuilder.build()).thenReturn(httpClient);
+    Mockito.when(httpClient.execute(Mockito.any()))
+      .thenThrow(new SocketException());
+
+    ServiceNowTableAPIRequestBuilder builder = new ServiceNowTableAPIRequestBuilder("url");
+    RestAPIRequest request = builder.build();
+
+    ServiceNowConnectorConfig config = Mockito.mock(ServiceNowConnectorConfig.class);
+    ServiceNowTableAPIClientImpl client = new ServiceNowTableAPIClientImpl(config, true);
+    RestAPIResponse actualResponse = client.executeGet(request);
+    Assert.assertNotNull(actualResponse.getException());
+    Assert.assertTrue(actualResponse.getException().isErrorRetryable());
+    Throwable ex = actualResponse.getException().getCause();
+    Assert.assertTrue("Expected SocketException or similar, got: " + ex, ex instanceof SocketException);
+  }
 }