From b6363896e0d3c281f374ab7857368cde356b74a2 Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Mon, 31 Jul 2023 09:48:53 -0700 Subject: [PATCH] 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 (cherry picked from commit e1f41bb21177e723aed6c2da65b0cdfb3df6e37f) --- .../UpdateDatasourceTransportAction.java | 6 ++++ .../geospatial/ip2geo/dao/GeoIpDataDao.java | 5 ++- .../UpdateDatasourceTransportActionTests.java | 35 +++++++++++++++++++ .../ip2geo/dao/GeoIpDataDaoTests.java | 2 +- 4 files changed, 46 insertions(+), 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 index c832dc89..31d77d08 100644 --- 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 index a538e813..47774b90 100644 --- 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/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java index e0a94a75..aeb90c35 100644 --- a/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java +++ b/src/test/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 listener = mock(ActionListener.class); + LockModel lockModel = randomLockModel(); + + // Run + action.doExecute(task, request, listener); + + // Verify + ArgumentCaptor> captor = ArgumentCaptor.forClass(ActionListener.class); + verify(ip2GeoLockService).acquireLock(eq(datasource.getName()), anyLong(), captor.capture()); + + // Run + captor.getValue().onResponse(lockModel); + + // Verify + ArgumentCaptor 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 index 8007d4bf..0e7a1a26 100644 --- 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")); }