Refactoring of code (opensearch-project#282)

1. Change variable name from datasourceName to name 2. Change variable name from id to name 3. Added helper methods in test code Signed-off-by: Heemin Kim <heemin@amazon.com>
heemin32 · Jul 21, 2023 · c31ded4 · c31ded4
1 parent cc41bf5
commit c31ded4
Show file tree

Hide file tree

Showing 15 changed files with 133 additions and 84 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
@@ -43,10 +43,10 @@ public class PutDatasourceRequest extends AcknowledgedRequest<PutDatasourceReque
     private static final ParseField UPDATE_INTERVAL_IN_DAYS_FIELD = new ParseField("update_interval_in_days");
     private static final int MAX_DATASOURCE_NAME_BYTES = 255;
     /**
-     * @param datasourceName the datasource name
+     * @param name the datasource name
      * @return the datasource name
      */
-    private String datasourceName;
+    private String name;
     /**
      * @param endpoint url to a manifest file for a datasource
      * @return url to a manifest file for a datasource
@@ -70,10 +70,10 @@ public class PutDatasourceRequest extends AcknowledgedRequest<PutDatasourceReque
 
     /**
      * Default constructor
-     * @param datasourceName name of a datasource
+     * @param name name of a datasource
      */
-    public PutDatasourceRequest(final String datasourceName) {
-        this.datasourceName = datasourceName;
+    public PutDatasourceRequest(final String name) {
+        this.name = name;
     }
 
     /**
@@ -83,15 +83,15 @@ public PutDatasourceRequest(final String datasourceName) {
      */
     public PutDatasourceRequest(final StreamInput in) throws IOException {
         super(in);
-        this.datasourceName = in.readString();
+        this.name = in.readString();
         this.endpoint = in.readString();
         this.updateInterval = in.readTimeValue();
     }
 
     @Override
     public void writeTo(final StreamOutput out) throws IOException {
         super.writeTo(out);
-        out.writeString(datasourceName);
+        out.writeString(name);
         out.writeString(endpoint);
         out.writeTimeValue(updateInterval);
     }
@@ -106,29 +106,29 @@ public ActionRequestValidationException validate() {
     }
 
     private void validateDatasourceName(final ActionRequestValidationException errors) {
-        if (!Strings.validFileName(datasourceName)) {
+        if (!Strings.validFileName(name)) {
             errors.addValidationError("Datasource name must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
             return;
         }
-        if (datasourceName.isEmpty()) {
+        if (name.isEmpty()) {
             errors.addValidationError("Datasource name must not be empty");
             return;
         }
-        if (datasourceName.contains("#")) {
+        if (name.contains("#")) {
             errors.addValidationError("Datasource name must not contain '#'");
             return;
         }
-        if (datasourceName.contains(":")) {
+        if (name.contains(":")) {
             errors.addValidationError("Datasource name must not contain ':'");
             return;
         }
-        if (datasourceName.charAt(0) == '_' || datasourceName.charAt(0) == '-' || datasourceName.charAt(0) == '+') {
+        if (name.charAt(0) == '_' || name.charAt(0) == '-' || name.charAt(0) == '+') {
             errors.addValidationError("Datasource name must not start with '_', '-', or '+'");
             return;
         }
         int byteCount = 0;
         try {
-            byteCount = datasourceName.getBytes("UTF-8").length;
+            byteCount = name.getBytes("UTF-8").length;
         } catch (UnsupportedEncodingException e) {
             // UTF-8 should always be supported, but rethrow this if it is not for some reason
             throw new OpenSearchException("Unable to determine length of datasource name", e);
@@ -137,7 +137,7 @@ private void validateDatasourceName(final ActionRequestValidationException error
             errors.addValidationError("Datasource name is too long, (" + byteCount + " > " + MAX_DATASOURCE_NAME_BYTES + ")");
             return;
         }
-        if (datasourceName.equals(".") || datasourceName.equals("..")) {
+        if (name.equals(".") || name.equals("..")) {
             errors.addValidationError("Datasource name must not be '.' or '..'");
             return;
         }

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportAction.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportAction.java
@@ -73,7 +73,7 @@ protected void doExecute(final Task task, final PutDatasourceRequest request, fi
         try {
             Datasource datasource = Datasource.Builder.build(request);
             IndexRequest indexRequest = new IndexRequest().index(DatasourceExtension.JOB_INDEX_NAME)
-                .id(datasource.getId())
+                .id(datasource.getName())
                 .source(datasource.toXContent(JsonXContent.contentBuilder(), null))
                 .setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE)
                 .opType(DocWriteRequest.OpType.CREATE);
@@ -100,7 +100,7 @@ public void onResponse(final IndexResponse indexResponse) {
             @Override
             public void onFailure(final Exception e) {
                 if (e instanceof VersionConflictEngineException) {
-                    listener.onFailure(new ResourceAlreadyExistsException("datasource [{}] already exists", datasource.getId()));
+                    listener.onFailure(new ResourceAlreadyExistsException("datasource [{}] already exists", datasource.getName()));
                 } else {
                     listener.onFailure(e);
                 }
@@ -119,7 +119,7 @@ protected void createDatasource(final Datasource datasource) {
         try {
             datasourceUpdateService.updateOrCreateGeoIpData(datasource);
         } catch (Exception e) {
-            log.error("Failed to create datasource for {}", datasource.getId(), e);
+            log.error("Failed to create datasource for {}", datasource.getName(), e);
             markDatasourceAsCreateFailed(datasource);
         }
     }
@@ -130,7 +130,7 @@ private void markDatasourceAsCreateFailed(final Datasource datasource) {
         try {
             datasourceFacade.updateDatasource(datasource);
         } catch (Exception e) {
-            log.error("Failed to mark datasource state as CREATE_FAILED for {}", datasource.getId(), e);
+            log.error("Failed to mark datasource state as CREATE_FAILED for {}", datasource.getName(), e);
         }
     }
 }
diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceFacade.java b/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceFacade.java
@@ -53,25 +53,25 @@ public DatasourceFacade(final Client client, final ClusterSettings clusterSettin
     public IndexResponse updateDatasource(final Datasource datasource) throws IOException {
         datasource.setLastUpdateTime(Instant.now());
         IndexRequestBuilder requestBuilder = client.prepareIndex(DatasourceExtension.JOB_INDEX_NAME);
-        requestBuilder.setId(datasource.getId());
+        requestBuilder.setId(datasource.getName());
         requestBuilder.setOpType(DocWriteRequest.OpType.INDEX);
         requestBuilder.setSource(datasource.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS));
         return client.index(requestBuilder.request()).actionGet(clusterSettings.get(Ip2GeoSettings.TIMEOUT));
     }
 
     /**
      * Get datasource from an index {@code DatasourceExtension.JOB_INDEX_NAME}
-     * @param id the name of a datasource
+     * @param name the name of a datasource
      * @return datasource
      * @throws IOException exception
      */
-    public Datasource getDatasource(final String id) throws IOException {
-        GetRequest request = new GetRequest(DatasourceExtension.JOB_INDEX_NAME, id);
+    public Datasource getDatasource(final String name) throws IOException {
+        GetRequest request = new GetRequest(DatasourceExtension.JOB_INDEX_NAME, name);
         GetResponse response;
         try {
             response = client.get(request).actionGet(clusterSettings.get(Ip2GeoSettings.TIMEOUT));
             if (response.isExists() == false) {
-                log.error("Datasource[{}] does not exist in an index[{}]", id, DatasourceExtension.JOB_INDEX_NAME);
+                log.error("Datasource[{}] does not exist in an index[{}]", name, DatasourceExtension.JOB_INDEX_NAME);
                 return null;
             }
         } catch (IndexNotFoundException e) {
@@ -89,11 +89,11 @@ public Datasource getDatasource(final String id) throws IOException {
 
     /**
      * Get datasource from an index {@code DatasourceExtension.JOB_INDEX_NAME}
-     * @param id the name of a datasource
+     * @param name the name of a datasource
      * @param actionListener the action listener
      */
-    public void getDatasource(final String id, final ActionListener<Datasource> actionListener) {
-        GetRequest request = new GetRequest(DatasourceExtension.JOB_INDEX_NAME, id);
+    public void getDatasource(final String name, final ActionListener<Datasource> actionListener) {
+        GetRequest request = new GetRequest(DatasourceExtension.JOB_INDEX_NAME, name);
         client.get(request, new ActionListener<GetResponse>() {
             @Override
             public void onResponse(final GetResponse response) {

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/Datasource.java b/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/Datasource.java
@@ -58,13 +58,13 @@ public class Datasource implements ScheduledJobParameter {
     /**
      * Default fields for job scheduling
      */
-    private static final ParseField ID_FIELD = new ParseField("id");
-    private static final ParseField ENABLED_FILED = new ParseField("update_enabled");
+    private static final ParseField NAME_FIELD = new ParseField("name");
+    private static final ParseField ENABLED_FIELD = new ParseField("update_enabled");
     private static final ParseField LAST_UPDATE_TIME_FIELD = new ParseField("last_update_time");
     private static final ParseField LAST_UPDATE_TIME_FIELD_READABLE = new ParseField("last_update_time_field");
     private static final ParseField SCHEDULE_FIELD = new ParseField("schedule");
-    private static final ParseField ENABLED_TIME_FILED = new ParseField("enabled_time");
-    private static final ParseField ENABLED_TIME_FILED_READABLE = new ParseField("enabled_time_field");
+    private static final ParseField ENABLED_TIME_FIELD = new ParseField("enabled_time");
+    private static final ParseField ENABLED_TIME_FIELD_READABLE = new ParseField("enabled_time_field");
 
     /**
      * Additional fields for datasource
@@ -80,10 +80,10 @@ public class Datasource implements ScheduledJobParameter {
      */
 
     /**
-     * @param id Id of a datasource
-     * @return Id of a datasource
+     * @param name name of a datasource
+     * @return name of a datasource
      */
-    private String id;
+    private String name;
     /**
      * @param lastUpdateTime Last update time of a datasource
      * @return Last update time of a datasource
@@ -169,10 +169,10 @@ public class Datasource implements ScheduledJobParameter {
         }
     );
     static {
-        PARSER.declareString(ConstructingObjectParser.constructorArg(), ID_FIELD);
+        PARSER.declareString(ConstructingObjectParser.constructorArg(), NAME_FIELD);
         PARSER.declareLong(ConstructingObjectParser.constructorArg(), LAST_UPDATE_TIME_FIELD);
-        PARSER.declareLong(ConstructingObjectParser.optionalConstructorArg(), ENABLED_TIME_FILED);
-        PARSER.declareBoolean(ConstructingObjectParser.constructorArg(), ENABLED_FILED);
+        PARSER.declareLong(ConstructingObjectParser.optionalConstructorArg(), ENABLED_TIME_FIELD);
+        PARSER.declareBoolean(ConstructingObjectParser.constructorArg(), ENABLED_FIELD);
         PARSER.declareObject(ConstructingObjectParser.constructorArg(), (p, c) -> ScheduleParser.parse(p), SCHEDULE_FIELD);
         PARSER.declareString(ConstructingObjectParser.constructorArg(), ENDPOINT_FIELD);
         PARSER.declareString(ConstructingObjectParser.constructorArg(), STATE_FIELD);
@@ -205,20 +205,20 @@ public Datasource(final String id, final IntervalSchedule schedule, final String
     @Override
     public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException {
         builder.startObject();
-        builder.field(ID_FIELD.getPreferredName(), id);
+        builder.field(NAME_FIELD.getPreferredName(), name);
         builder.timeField(
             LAST_UPDATE_TIME_FIELD.getPreferredName(),
             LAST_UPDATE_TIME_FIELD_READABLE.getPreferredName(),
             lastUpdateTime.toEpochMilli()
         );
         if (enabledTime != null) {
             builder.timeField(
-                ENABLED_TIME_FILED.getPreferredName(),
-                ENABLED_TIME_FILED_READABLE.getPreferredName(),
+                ENABLED_TIME_FIELD.getPreferredName(),
+                ENABLED_TIME_FIELD_READABLE.getPreferredName(),
                 enabledTime.toEpochMilli()
             );
         }
-        builder.field(ENABLED_FILED.getPreferredName(), isEnabled);
+        builder.field(ENABLED_FIELD.getPreferredName(), isEnabled);
         builder.field(SCHEDULE_FIELD.getPreferredName(), schedule);
         builder.field(ENDPOINT_FIELD.getPreferredName(), endpoint);
         builder.field(STATE_FIELD.getPreferredName(), state.name());
@@ -231,7 +231,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa
 
     @Override
     public String getName() {
-        return id;
+        return name;
     }
 
     @Override
@@ -313,7 +313,7 @@ public String indexNameFor(final DatasourceManifest manifest) {
     }
 
     private String indexNameFor(final long suffix) {
-        return String.format(Locale.ROOT, "%s.%s.%d", IP2GEO_DATA_INDEX_NAME_PREFIX, id, suffix);
+        return String.format(Locale.ROOT, "%s.%s.%d", IP2GEO_DATA_INDEX_NAME_PREFIX, name, suffix);
     }
 
     /**
@@ -567,7 +567,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa
      */
     public static class Builder {
         public static Datasource build(final PutDatasourceRequest request) {
-            String id = request.getDatasourceName();
+            String id = request.getName();
             IntervalSchedule schedule = new IntervalSchedule(
                 Instant.now().truncatedTo(ChronoUnit.MILLIS),
                 (int) request.getUpdateInterval().days(),

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceRunner.java b/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceRunner.java
@@ -153,7 +153,7 @@ protected void updateDatasource(final ScheduledJobParameter jobParameter) throws
             datasourceUpdateService.updateOrCreateGeoIpData(datasource);
             datasourceUpdateService.deleteUnusedIndices(datasource);
         } catch (Exception e) {
-            log.error("Failed to update datasource for {}", datasource.getId(), e);
+            log.error("Failed to update datasource for {}", datasource.getName(), e);
             datasource.getUpdateStats().setLastFailedAt(Instant.now());
             datasourceFacade.updateDatasource(datasource);
         }

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceUpdateService.java b/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceUpdateService.java
@@ -63,7 +63,7 @@ public void updateOrCreateGeoIpData(final Datasource datasource) throws Exceptio
         DatasourceManifest manifest = DatasourceManifest.Builder.build(url);
 
         if (shouldUpdate(datasource, manifest) == false) {
-            log.info("Skipping GeoIP database update. Update is not required for {}", datasource.getId());
+            log.info("Skipping GeoIP database update. Update is not required for {}", datasource.getName());
             datasource.getUpdateStats().setLastSkippedAt(Instant.now());
             datasourceFacade.updateDatasource(datasource);
             return;
@@ -110,7 +110,7 @@ public void deleteUnusedIndices(final Datasource parameter) {
                 datasourceFacade.updateDatasource(parameter);
             }
         } catch (Exception e) {
-            log.error("Failed to delete old indices for {}", parameter.getId(), e);
+            log.error("Failed to delete old indices for {}", parameter.getName(), e);
         }
     }
 
@@ -175,7 +175,11 @@ private void updateDatasourceAsSucceeded(
         datasource.enable();
         datasource.setState(DatasourceState.AVAILABLE);
         datasourceFacade.updateDatasource(datasource);
-        log.info("GeoIP database creation succeeded for {} and took {} seconds", datasource.getId(), Duration.between(startTime, endTime));
+        log.info(
+            "GeoIP database creation succeeded for {} and took {} seconds",
+            datasource.getName(),
+            Duration.between(startTime, endTime)
+        );
     }
 
     /***

diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/Ip2GeoTestCase.java b/src/test/java/org/opensearch/geospatial/ip2geo/Ip2GeoTestCase.java
@@ -13,6 +13,8 @@
 
 import java.io.File;
 import java.nio.file.Paths;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Locale;
@@ -39,13 +41,16 @@
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.concurrent.OpenSearchExecutors;
+import org.opensearch.geospatial.GeospatialTestHelper;
 import org.opensearch.geospatial.ip2geo.common.DatasourceFacade;
 import org.opensearch.geospatial.ip2geo.common.DatasourceState;
 import org.opensearch.geospatial.ip2geo.common.GeoIpDataFacade;
 import org.opensearch.geospatial.ip2geo.common.Ip2GeoExecutor;
 import org.opensearch.geospatial.ip2geo.common.Ip2GeoSettings;
+import org.opensearch.geospatial.ip2geo.jobscheduler.Datasource;
 import org.opensearch.geospatial.ip2geo.jobscheduler.DatasourceUpdateService;
 import org.opensearch.ingest.IngestService;
+import org.opensearch.jobscheduler.spi.schedule.IntervalSchedule;
 import org.opensearch.jobscheduler.spi.utils.LockService;
 import org.opensearch.tasks.Task;
 import org.opensearch.tasks.TaskListener;
@@ -120,6 +125,13 @@ public DatasourceState randomStateExcept(DatasourceState state) {
             .get(Randomness.createSecure().nextInt(DatasourceState.values().length - 2));
     }
 
+    public DatasourceState randomState() {
+        return Arrays.stream(DatasourceState.values())
+            .sequential()
+            .collect(Collectors.toList())
+            .get(Randomness.createSecure().nextInt(DatasourceState.values().length - 1));
+    }
+
     public String randomIpAddress() {
         return String.format(
             Locale.ROOT,
@@ -149,6 +161,33 @@ public File sampleIp2GeoFile() {
         return new File(this.getClass().getClassLoader().getResource("ip2geo/sample_valid.csv").getFile());
     }
 
+    public Datasource randomDatasource() {
+        Instant now = Instant.now().truncatedTo(ChronoUnit.MILLIS);
+        Datasource datasource = new Datasource();
+        datasource.setName(GeospatialTestHelper.randomLowerCaseString());
+        datasource.setSchedule(new IntervalSchedule(now, Randomness.get().nextInt(30) + 1, ChronoUnit.DAYS));
+        datasource.setState(randomState());
+        datasource.setIndices(Arrays.asList(GeospatialTestHelper.randomLowerCaseString(), GeospatialTestHelper.randomLowerCaseString()));
+        datasource.setEndpoint(GeospatialTestHelper.randomLowerCaseString());
+        datasource.getDatabase()
+            .setFields(Arrays.asList(GeospatialTestHelper.randomLowerCaseString(), GeospatialTestHelper.randomLowerCaseString()));
+        datasource.getDatabase().setProvider(GeospatialTestHelper.randomLowerCaseString());
+        datasource.getDatabase().setUpdatedAt(now);
+        datasource.getDatabase().setMd5Hash(GeospatialTestHelper.randomLowerCaseString());
+        datasource.getDatabase().setValidForInDays(Randomness.get().nextInt(30) + 1l);
+        datasource.getUpdateStats().setLastSkippedAt(now);
+        datasource.getUpdateStats().setLastSucceededAt(now);
+        datasource.getUpdateStats().setLastFailedAt(now);
+        datasource.getUpdateStats().setLastProcessingTimeInMillis(Randomness.get().nextLong());
+        datasource.setLastUpdateTime(now);
+        if (Randomness.get().nextInt() % 2 == 0) {
+            datasource.enable();
+        } else {
+            datasource.disable();
+        }
+        return datasource;
+    }
+
     /**
      * Temporary class of VerifyingClient until this PR(https://github.com/opensearch-project/OpenSearch/pull/7167)
      * is merged in OpenSearch core

diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequestTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequestTests.java
@@ -143,7 +143,7 @@ public void testValidateDatasourceNames() throws Exception {
         );
 
         for (Map.Entry<String, String> entry : nameToError.entrySet()) {
-            request.setDatasourceName(entry.getKey());
+            request.setName(entry.getKey());
 
             // Run
             exception = request.validate();