Changes in invalid input handling (#373)

1. Switch exception type from OpenSearchException to IllegalArgumentException for invalid endpoint 2. Add cluster state validation for update API Signed-off-by: Heemin Kim <heemin@amazon.com>
opensearch-project · Jul 31, 2023 · e1f41bb · e1f41bb
1 parent 26b1938
commit e1f41bb
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 2 deletions.
diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportAction.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportAction.java
@@ -24,6 +24,7 @@
 import org.opensearch.geospatial.exceptions.ConcurrentModificationException;
 import org.opensearch.geospatial.exceptions.IncompatibleDatasourceException;
 import org.opensearch.geospatial.ip2geo.common.DatasourceManifest;
+import org.opensearch.geospatial.ip2geo.common.DatasourceState;
 import org.opensearch.geospatial.ip2geo.common.Ip2GeoLockService;
 import org.opensearch.geospatial.ip2geo.dao.DatasourceDao;
 import org.opensearch.geospatial.ip2geo.jobscheduler.Datasource;
@@ -94,6 +95,11 @@ protected void doExecute(final Task task, final UpdateDatasourceRequest request,
                         if (datasource == null) {
                             throw new ResourceNotFoundException("no such datasource exist");
                         }
+                        if (DatasourceState.AVAILABLE.equals(datasource.getState()) == false) {
+                            throw new IllegalArgumentException(
+                                String.format(Locale.ROOT, "data source is not in an [%s] state", DatasourceState.AVAILABLE)
+                            );
+                        }
                         validate(request, datasource);
                         updateIfChanged(request, datasource);
                         lockService.releaseLock(lock);

diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java b/src/main/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java
@@ -21,6 +21,7 @@
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Queue;
@@ -192,7 +193,9 @@ protected CSVParser internalGetDatabaseReader(final DatasourceManifest manifest,
             }
             return new CSVParser(new BufferedReader(new InputStreamReader(zipIn)), CSVFormat.RFC4180);
         }
-        throw new OpenSearchException("database file [{}] does not exist in the zip file [{}]", manifest.getDbName(), manifest.getUrl());
+        throw new IllegalArgumentException(
+            String.format(Locale.ROOT, "database file [%s] does not exist in the zip file [%s]", manifest.getDbName(), manifest.getUrl())
+        );
     }
 
     /**

diff --git a/...st/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java b/...st/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java
@@ -30,6 +30,7 @@
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.geospatial.exceptions.IncompatibleDatasourceException;
 import org.opensearch.geospatial.ip2geo.Ip2GeoTestCase;
+import org.opensearch.geospatial.ip2geo.common.DatasourceState;
 import org.opensearch.geospatial.ip2geo.jobscheduler.Datasource;
 import org.opensearch.geospatial.ip2geo.jobscheduler.DatasourceTask;
 import org.opensearch.jobscheduler.spi.LockModel;
@@ -87,6 +88,7 @@ private void validateDoExecuteWithLockError(final Exception exception) {
     @SneakyThrows
     public void testDoExecute_whenValidInput_thenUpdate() {
         Datasource datasource = randomDatasource(Instant.now().minusSeconds(60));
+        datasource.setState(DatasourceState.AVAILABLE);
         datasource.setTask(DatasourceTask.DELETE_UNUSED_INDICES);
         Instant originalStartTime = datasource.getSchedule().getStartTime();
         UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName());
@@ -124,6 +126,7 @@ public void testDoExecute_whenValidInput_thenUpdate() {
     @SneakyThrows
     public void testDoExecute_whenNoChangesInValues_thenNoUpdate() {
         Datasource datasource = randomDatasource();
+        datasource.setState(DatasourceState.AVAILABLE);
         UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName());
         request.setEndpoint(datasource.getEndpoint());
 
@@ -177,9 +180,40 @@ public void testDoExecute_whenNoDatasource_thenError() {
         verify(ip2GeoLockService).releaseLock(eq(lockModel));
     }
 
+    @SneakyThrows
+    public void testDoExecute_whenNotInAvailableState_thenError() {
+        Datasource datasource = randomDatasource();
+        datasource.setState(DatasourceState.CREATE_FAILED);
+        UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName());
+        request.setEndpoint(datasource.getEndpoint());
+
+        Task task = mock(Task.class);
+        when(datasourceDao.getDatasource(datasource.getName())).thenReturn(datasource);
+        ActionListener<AcknowledgedResponse> listener = mock(ActionListener.class);
+        LockModel lockModel = randomLockModel();
+
+        // Run
+        action.doExecute(task, request, listener);
+
+        // Verify
+        ArgumentCaptor<ActionListener<LockModel>> captor = ArgumentCaptor.forClass(ActionListener.class);
+        verify(ip2GeoLockService).acquireLock(eq(datasource.getName()), anyLong(), captor.capture());
+
+        // Run
+        captor.getValue().onResponse(lockModel);
+
+        // Verify
+        ArgumentCaptor<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
+        verify(listener).onFailure(exceptionCaptor.capture());
+        assertEquals(IllegalArgumentException.class, exceptionCaptor.getValue().getClass());
+        exceptionCaptor.getValue().getMessage().contains("not in an available");
+        verify(ip2GeoLockService).releaseLock(eq(lockModel));
+    }
+
     @SneakyThrows
     public void testDoExecute_whenIncompatibleFields_thenError() {
         Datasource datasource = randomDatasource();
+        datasource.setState(DatasourceState.AVAILABLE);
         UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName());
         request.setEndpoint(sampleManifestUrl());
 
@@ -211,6 +245,7 @@ public void testDoExecute_whenIncompatibleFields_thenError() {
     @SneakyThrows
     public void testDoExecute_whenLargeUpdateInterval_thenError() {
         Datasource datasource = randomDatasource();
+        datasource.setState(DatasourceState.AVAILABLE);
         UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName());
         request.setUpdateInterval(TimeValue.timeValueDays(datasource.getDatabase().getValidForInDays()));
 

diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDaoTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDaoTests.java
@@ -144,7 +144,7 @@ public void testGetDatabaseReaderNoFile() throws Exception {
             Instant.now().toEpochMilli(),
             "tester"
         );
-        OpenSearchException exception = expectThrows(OpenSearchException.class, () -> noOpsGeoIpDataDao.getDatabaseReader(manifest));
+        Exception exception = expectThrows(IllegalArgumentException.class, () -> noOpsGeoIpDataDao.getDatabaseReader(manifest));
         assertTrue(exception.getMessage().contains("does not exist"));
     }