From b291c2374e243b66ed2169a1ed3722f5cfd6d69f Mon Sep 17 00:00:00 2001
From: Heemin Kim <heemin@amazon.com>
Date: Thu, 20 Apr 2023 09:43:00 -0700
Subject: [PATCH] Update based on review comments

Signed-off-by: Heemin Kim <heemin@amazon.com>
---
 .../ip2geo/action/PutDatasourceRequest.java   | 35 +++++++++++--------
 .../ip2geo/common/DatasourceHelper.java       | 23 ++++++------
 .../ip2geo/jobscheduler/DatasourceTests.java  |  8 +++--
 3 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java
index bdff2ee6..1bbe2991 100644
--- a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java
+++ b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java
@@ -110,25 +110,32 @@ private void validateEndpoint(final ActionRequestValidationException errors) {
     }
 
     private void validateManifestFile(final URL url, final ActionRequestValidationException errors) {
+        DatasourceManifest manifest;
+        try {
+            manifest = DatasourceManifest.Builder.build(url);
+        } catch (Exception e) {
+            log.info("Error occurred while reading a file from {}", url, e);
+            errors.addValidationError(String.format(Locale.ROOT, "Error occurred while reading a file from %s", url));
+            return;
+        }
+
         try {
-            DatasourceManifest manifest = DatasourceManifest.Builder.build(url);
             new URL(manifest.getUrl()).toURI(); // Validate URL complies with RFC-2396
-            if (manifest.getValidForInDays() <= updateIntervalInDays.days()) {
-                errors.addValidationError(
-                    String.format(
-                        Locale.ROOT,
-                        "updateInterval %d is should be smaller than %d",
-                        updateIntervalInDays.days(),
-                        manifest.getValidForInDays()
-                    )
-                );
-            }
         } catch (MalformedURLException | URISyntaxException e) {
             log.info("Invalid URL format is provided for url field in the manifest file", e);
             errors.addValidationError("Invalid URL format is provided for url field in the manifest file");
-        } catch (Exception e) {
-            log.info("Error occurred while reading a file from {}", url, e);
-            errors.addValidationError(String.format(Locale.ROOT, "Error occurred while reading a file from %s", url));
+            return;
+        }
+
+        if (manifest.getValidForInDays() <= updateIntervalInDays.days()) {
+            errors.addValidationError(
+                String.format(
+                    Locale.ROOT,
+                    "updateInterval %d is should be smaller than %d",
+                    updateIntervalInDays.days(),
+                    manifest.getValidForInDays()
+                )
+            );
         }
     }
 
diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceHelper.java b/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceHelper.java
index 010b66ad..ea89e585 100644
--- a/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceHelper.java
+++ b/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceHelper.java
@@ -98,17 +98,18 @@ public static void getDatasource(final Client client, final String id, final Act
             public void onResponse(final GetResponse response) {
                 if (!response.isExists()) {
                     actionListener.onResponse(null);
-                } else {
-                    try {
-                        XContentParser parser = XContentHelper.createParser(
-                            NamedXContentRegistry.EMPTY,
-                            LoggingDeprecationHandler.INSTANCE,
-                            response.getSourceAsBytesRef()
-                        );
-                        actionListener.onResponse(Datasource.PARSER.parse(parser, null));
-                    } catch (IOException e) {
-                        actionListener.onFailure(e);
-                    }
+                    return;
+                }
+
+                try {
+                    XContentParser parser = XContentHelper.createParser(
+                        NamedXContentRegistry.EMPTY,
+                        LoggingDeprecationHandler.INSTANCE,
+                        response.getSourceAsBytesRef()
+                    );
+                    actionListener.onResponse(Datasource.PARSER.parse(parser, null));
+                } catch (IOException e) {
+                    actionListener.onFailure(e);
                 }
             }
 
diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceTests.java
index 8c543da2..b5171c02 100644
--- a/src/test/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceTests.java
+++ b/src/test/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceTests.java
@@ -16,12 +16,14 @@
 import java.util.ArrayList;
 import java.util.Locale;
 
+import org.opensearch.common.Randomness;
+import org.opensearch.geospatial.GeospatialTestHelper;
 import org.opensearch.geospatial.ip2geo.common.DatasourceManifest;
 import org.opensearch.test.OpenSearchTestCase;
 
 public class DatasourceTests extends OpenSearchTestCase {
     public void testCurrentIndexName() {
-        String id = "test";
+        String id = GeospatialTestHelper.randomLowerCaseString();
         Instant now = Instant.now();
         Datasource datasource = new Datasource();
         datasource.setId(id);
@@ -33,11 +35,11 @@ public void testCurrentIndexName() {
     }
 
     public void testGetIndexNameFor() {
-        long updatedAt = 123123123l;
+        long updatedAt = Randomness.get().nextLong();
         DatasourceManifest manifest = mock(DatasourceManifest.class);
         when(manifest.getUpdatedAt()).thenReturn(updatedAt);
 
-        String id = "test";
+        String id = GeospatialTestHelper.randomLowerCaseString();
         Datasource datasource = new Datasource();
         datasource.setId(id);
         assertEquals(