Added input parameter validation with few small changes

1. Added datasource name validation in delete/update api 2. Added datasource name validation in processor creation method 3. Changed index name delimiter from `.` to `-` to be consistent with others Signed-off-by: Heemin Kim <heemin@amazon.com>
opensearch-project · Jul 27, 2023 · 2ba26c8 · 2ba26c8
1 parent 323dd1d
commit 2ba26c8
Show file tree

Hide file tree

Showing 12 changed files with 232 additions and 112 deletions.
diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/DeleteDatasourceRequest.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/DeleteDatasourceRequest.java
@@ -15,6 +15,7 @@
 import org.opensearch.action.ActionRequestValidationException;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
+import org.opensearch.geospatial.ip2geo.common.ParameterValidator;
 
 /**
  * GeoIP datasource delete request
@@ -23,6 +24,7 @@
 @Setter
 @AllArgsConstructor
 public class DeleteDatasourceRequest extends ActionRequest {
+    private static final ParameterValidator VALIDATOR = new ParameterValidator();
     /**
      * @param name the datasource name
      * @return the datasource name
@@ -43,9 +45,9 @@ public DeleteDatasourceRequest(final StreamInput in) throws IOException {
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException errors = null;
-        if (name == null || name.isBlank()) {
+        if (VALIDATOR.validateDatasourceName(name).isEmpty() == false) {
             errors = new ActionRequestValidationException();
-            errors.addValidationError("Datasource name should not be empty");
+            errors.addValidationError("no such datasource exist");
         }
         return errors;
     }

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java
@@ -6,39 +6,37 @@
 package org.opensearch.geospatial.ip2geo.action;
 
 import java.io.IOException;
-import java.io.UnsupportedEncodingException;
 import java.net.MalformedURLException;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.util.List;
 import java.util.Locale;
 
-import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.Setter;
 import lombok.extern.log4j.Log4j2;
 
-import org.opensearch.OpenSearchException;
 import org.opensearch.action.ActionRequest;
 import org.opensearch.action.ActionRequestValidationException;
-import org.opensearch.common.Strings;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.core.ParseField;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.xcontent.ObjectParser;
 import org.opensearch.geospatial.ip2geo.common.DatasourceManifest;
+import org.opensearch.geospatial.ip2geo.common.ParameterValidator;
 
 /**
  * Ip2Geo datasource creation request
  */
 @Getter
 @Setter
 @Log4j2
-@EqualsAndHashCode(callSuper = false)
 public class PutDatasourceRequest extends ActionRequest {
-    private static final int MAX_DATASOURCE_NAME_BYTES = 255;
     public static final ParseField ENDPOINT_FIELD = new ParseField("endpoint");
     public static final ParseField UPDATE_INTERVAL_IN_DAYS_FIELD = new ParseField("update_interval_in_days");
+    private static final ParameterValidator VALIDATOR = new ParameterValidator();
+
     /**
      * @param name the datasource name
      * @return the datasource name
@@ -96,50 +94,15 @@ public void writeTo(final StreamOutput out) throws IOException {
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException errors = new ActionRequestValidationException();
-        validateDatasourceName(errors);
+        List<String> errorMsgs = VALIDATOR.validateDatasourceName(name);
+        if (errorMsgs.isEmpty() == false) {
+            errorMsgs.stream().forEach(msg -> errors.addValidationError(msg));
+        }
         validateEndpoint(errors);
         validateUpdateInterval(errors);
         return errors.validationErrors().isEmpty() ? null : errors;
     }
 
-    private void validateDatasourceName(final ActionRequestValidationException errors) {
-        if (!Strings.validFileName(name)) {
-            errors.addValidationError("Datasource name must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
-            return;
-        }
-        if (name.isEmpty()) {
-            errors.addValidationError("Datasource name must not be empty");
-            return;
-        }
-        if (name.contains("#")) {
-            errors.addValidationError("Datasource name must not contain '#'");
-            return;
-        }
-        if (name.contains(":")) {
-            errors.addValidationError("Datasource name must not contain ':'");
-            return;
-        }
-        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 = 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);
-        }
-        if (byteCount > MAX_DATASOURCE_NAME_BYTES) {
-            errors.addValidationError("Datasource name is too long, (" + byteCount + " > " + MAX_DATASOURCE_NAME_BYTES + ")");
-            return;
-        }
-        if (name.equals(".") || name.equals("..")) {
-            errors.addValidationError("Datasource name must not be '.' or '..'");
-            return;
-        }
-    }
-
     /**
      * Conduct following validation on endpoint
      * 1. endpoint format complies with RFC-2396

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequest.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequest.java
@@ -24,6 +24,7 @@
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.xcontent.ObjectParser;
 import org.opensearch.geospatial.ip2geo.common.DatasourceManifest;
+import org.opensearch.geospatial.ip2geo.common.ParameterValidator;
 
 /**
  * Ip2Geo datasource update request
@@ -36,6 +37,8 @@ public class UpdateDatasourceRequest extends ActionRequest {
     public static final ParseField ENDPOINT_FIELD = new ParseField("endpoint");
     public static final ParseField UPDATE_INTERVAL_IN_DAYS_FIELD = new ParseField("update_interval_in_days");
     private static final int MAX_DATASOURCE_NAME_BYTES = 255;
+    private static final ParameterValidator VALIDATOR = new ParameterValidator();
+
     /**
      * @param name the datasource name
      * @return the datasource name
@@ -93,6 +96,9 @@ public void writeTo(final StreamOutput out) throws IOException {
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException errors = new ActionRequestValidationException();
+        if (VALIDATOR.validateDatasourceName(name).isEmpty() == false) {
+            errors.addValidationError("no such datasource exist");
+        }
         if (endpoint == null && updateInterval == null) {
             errors.addValidationError("no values to update");
         }

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/common/ParameterValidator.java b/src/main/java/org/opensearch/geospatial/ip2geo/common/ParameterValidator.java
@@ -0,0 +1,58 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.geospatial.ip2geo.common;
+
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+
+import org.apache.commons.lang3.StringUtils;
+import org.opensearch.common.Strings;
+
+/**
+ * Parameter validator for IP2Geo APIs
+ */
+public class ParameterValidator {
+    private static final int MAX_DATASOURCE_NAME_BYTES = 127;
+
+    /**
+     * Validate datasource name and return list of error messages
+     *
+     * @param datasourceName datasource name
+     * @return Error messages. Empty list if there is no violation.
+     */
+    public List<String> validateDatasourceName(final String datasourceName) {
+        List<String> errorMsgs = new ArrayList<>();
+        if (StringUtils.isBlank(datasourceName)) {
+            errorMsgs.add("datasource name must not be empty");
+            return errorMsgs;
+        }
+
+        if (!Strings.validFileName(datasourceName)) {
+            errorMsgs.add(
+                String.format(Locale.ROOT, "datasource name must not contain the following characters %s", Strings.INVALID_FILENAME_CHARS)
+            );
+        }
+        if (datasourceName.contains("#")) {
+            errorMsgs.add("datasource name must not contain '#'");
+        }
+        if (datasourceName.contains(":")) {
+            errorMsgs.add("datasource name must not contain ':'");
+        }
+        if (datasourceName.charAt(0) == '_' || datasourceName.charAt(0) == '-' || datasourceName.charAt(0) == '+') {
+            errorMsgs.add("datasource name must not start with '_', '-', or '+'");
+        }
+        int byteCount = datasourceName.getBytes(StandardCharsets.UTF_8).length;
+        if (byteCount > MAX_DATASOURCE_NAME_BYTES) {
+            errorMsgs.add(String.format(Locale.ROOT, "datasource name is too long, (%d > %d)", byteCount, MAX_DATASOURCE_NAME_BYTES));
+        }
+        if (datasourceName.equals(".") || datasourceName.equals("..")) {
+            errorMsgs.add("datasource name must not be '.' or '..'");
+        }
+        return errorMsgs;
+    }
+}
diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/Datasource.java b/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/Datasource.java
@@ -50,7 +50,7 @@ public class Datasource implements Writeable, ScheduledJobParameter {
     /**
      * Prefix of indices having Ip2Geo data
      */
-    public static final String IP2GEO_DATA_INDEX_NAME_PREFIX = ".geospatial.ip2geo.data";
+    public static final String IP2GEO_DATA_INDEX_NAME_PREFIX = ".geospatial-ip2geo-data";
 
     /**
      * Default fields for job scheduling

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceExtension.java b/src/main/java/org/opensearch/geospatial/ip2geo/jobscheduler/DatasourceExtension.java
@@ -26,7 +26,7 @@ public class DatasourceExtension implements JobSchedulerExtension {
     /**
      * Job index name for a datasource
      */
-    public static final String JOB_INDEX_NAME = ".scheduler_geospatial_ip2geo_datasource";
+    public static final String JOB_INDEX_NAME = ".scheduler-geospatial-ip2geo-datasource";
     /**
      * Job index setting
      *

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/processor/Ip2GeoProcessor.java b/src/main/java/org/opensearch/geospatial/ip2geo/processor/Ip2GeoProcessor.java
@@ -4,6 +4,7 @@
  */
 package org.opensearch.geospatial.ip2geo.processor;
 
+import static org.opensearch.ingest.ConfigurationUtils.newConfigurationException;
 import static org.opensearch.ingest.ConfigurationUtils.readBooleanProperty;
 import static org.opensearch.ingest.ConfigurationUtils.readOptionalList;
 import static org.opensearch.ingest.ConfigurationUtils.readStringProperty;
@@ -17,12 +18,12 @@
 import java.util.function.BiConsumer;
 import java.util.stream.Collectors;
 
-import lombok.AllArgsConstructor;
 import lombok.Getter;
 import lombok.extern.log4j.Log4j2;
 
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.geospatial.ip2geo.common.DatasourceState;
+import org.opensearch.geospatial.ip2geo.common.ParameterValidator;
 import org.opensearch.geospatial.ip2geo.dao.DatasourceDao;
 import org.opensearch.geospatial.ip2geo.dao.GeoIpDataDao;
 import org.opensearch.geospatial.ip2geo.dao.Ip2GeoCachedDao;
@@ -232,13 +233,25 @@ public String getType() {
     /**
      * Ip2Geo processor factory
      */
-    @AllArgsConstructor
     public static final class Factory implements Processor.Factory {
+        private static final ParameterValidator VALIDATOR = new ParameterValidator();
         private final IngestService ingestService;
         private final DatasourceDao datasourceDao;
         private final GeoIpDataDao geoIpDataDao;
         private final Ip2GeoCachedDao ip2GeoCachedDao;
 
+        public Factory(
+            final IngestService ingestService,
+            final DatasourceDao datasourceDao,
+            final GeoIpDataDao geoIpDataDao,
+            final Ip2GeoCachedDao ip2GeoCachedDao
+        ) {
+            this.ingestService = ingestService;
+            this.datasourceDao = datasourceDao;
+            this.geoIpDataDao = geoIpDataDao;
+            this.ip2GeoCachedDao = ip2GeoCachedDao;
+        }
+
         /**
          * Within this method, blocking request cannot be called because this method is executed in a transport thread.
          * This means, validation using data in an index won't work.
@@ -256,6 +269,11 @@ public Ip2GeoProcessor create(
             List<String> propertyNames = readOptionalList(TYPE, processorTag, config, CONFIG_PROPERTIES);
             boolean ignoreMissing = readBooleanProperty(TYPE, processorTag, config, CONFIG_IGNORE_MISSING, false);
 
+            List<String> error = VALIDATOR.validateDatasourceName(datasourceName);
+            if (error.isEmpty() == false) {
+                throw newConfigurationException(TYPE, processorTag, "datasource", error.get(0));
+            }
+
             return new Ip2GeoProcessor(
                 processorTag,
                 description,

diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/action/DeleteDatasourceRequestTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/action/DeleteDatasourceRequestTests.java
@@ -50,4 +50,16 @@ public void testValidate_whenBlank_thenError() {
         assertNotNull(error.validationErrors());
         assertFalse(error.validationErrors().isEmpty());
     }
+
+    public void testValidate_whenInvalidDatasourceName_thenFails() {
+        String invalidName = "_" + GeospatialTestHelper.randomLowerCaseString();
+        DeleteDatasourceRequest request = new DeleteDatasourceRequest(invalidName);
+
+        // Run
+        ActionRequestValidationException exception = request.validate();
+
+        // Verify
+        assertEquals(1, exception.validationErrors().size());
+        assertTrue(exception.validationErrors().get(0).contains("no such datasource"));
+    }
 }