diff --git a/CHANGELOG.md b/CHANGELOG.md index 8291a4953..3f7907b31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Infrastructure * Upgrade gradle to 8.4 [1289](https://github.com/opensearch-project/k-NN/pull/1289) * Refactor security testing to install from individual components [#1307](https://github.com/opensearch-project/k-NN/pull/1307) +* Refactor integ tests that access model index [#1423](https://github.com/opensearch-project/k-NN/pull/1423) ### Documentation ### Maintenance * Update developer guide to include M1 Setup [#1222](https://github.com/opensearch-project/k-NN/pull/1222) diff --git a/src/test/java/org/opensearch/knn/plugin/action/RestDeleteModelHandlerIT.java b/src/test/java/org/opensearch/knn/plugin/action/RestDeleteModelHandlerIT.java index b9f1b0a79..2bc7e04b2 100644 --- a/src/test/java/org/opensearch/knn/plugin/action/RestDeleteModelHandlerIT.java +++ b/src/test/java/org/opensearch/knn/plugin/action/RestDeleteModelHandlerIT.java @@ -11,6 +11,7 @@ package org.opensearch.knn.plugin.action; +import lombok.SneakyThrows; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.opensearch.client.Request; import org.opensearch.client.Response; @@ -30,7 +31,6 @@ import static org.opensearch.knn.common.KNNConstants.KNN_ENGINE; import static org.opensearch.knn.common.KNNConstants.MODEL_ID; import static org.opensearch.knn.common.KNNConstants.MODELS; -import static org.opensearch.knn.common.KNNConstants.MODEL_INDEX_NAME; import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_SPACE_TYPE; import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NLIST; import static org.opensearch.knn.common.KNNConstants.METHOD_ENCODER_PARAMETER; @@ -43,9 +43,8 @@ public class RestDeleteModelHandlerIT extends KNNRestTestCase { - public void testDeleteModelExists() throws Exception { - createModelSystemIndex(); - + @SneakyThrows + public void testDelete_whenModelExists_thenDeletionSucceeds() { String modelId = "test-model-id"; String trainingIndexName = "train-index"; String trainingFieldName = "train-field"; @@ -80,42 +79,8 @@ public void testDeleteModelExists() throws Exception { assertTrue(ex.getMessage().contains(modelId)); } - public void testDeleteTrainingModel() throws Exception { - createModelSystemIndex(); - - String modelId = "test-model-id"; - String trainingIndexName = "train-index"; - String trainingFieldName = "train-field"; - int dimension = 8; - String modelDescription = "dummy description"; - - createBasicKnnIndex(trainingIndexName, trainingFieldName, dimension); - // we do not wait for training to be completed - ingestDataAndTrainModel(modelId, trainingIndexName, trainingFieldName, dimension, modelDescription); - - Response getModelResponse = getModel(modelId, List.of()); - assertEquals(RestStatus.OK, RestStatus.fromCode(getModelResponse.getStatusLine().getStatusCode())); - - String responseBody = EntityUtils.toString(getModelResponse.getEntity()); - assertNotNull(responseBody); - - Map responseMap = createParser(MediaTypeRegistry.getDefaultMediaType().xContent(), responseBody).map(); - - assertEquals(modelId, responseMap.get(MODEL_ID)); - - String deleteModelRestURI = String.join("/", KNNPlugin.KNN_BASE_URI, MODELS, modelId); - Request deleteModelRequest = new Request("DELETE", deleteModelRestURI); - - ResponseException ex = expectThrows(ResponseException.class, () -> client().performRequest(deleteModelRequest)); - assertEquals(RestStatus.CONFLICT.getStatus(), ex.getResponse().getStatusLine().getStatusCode()); - - // need to wait for training operation as it's required for after test cleanup - assertTrainingSucceeds(modelId, NUM_OF_ATTEMPTS, DELAY_MILLI_SEC); - } - - public void testDeleteModelFailsInvalid() throws Exception { + public void testDelete_whenModelIDIsInvalid_thenFail() { String modelId = "invalid-model-id"; - createModelSystemIndex(); String restURI = String.join("/", KNNPlugin.KNN_BASE_URI, MODELS, modelId); Request request = new Request("DELETE", restURI); @@ -124,7 +89,8 @@ public void testDeleteModelFailsInvalid() throws Exception { } // Test Train Model -> Delete Model -> Train Model with same modelId - public void testTrainingDeletedModel() throws Exception { + @SneakyThrows + public void testTraining_whenModelHasBeenDeleted_thenSucceedTrainingModelWithSameID() { String modelId = "test-model-id1"; String trainingIndexName1 = "train-index-1"; String trainingIndexName2 = "train-index-2"; @@ -141,8 +107,6 @@ public void testTrainingDeletedModel() throws Exception { Response response = client().performRequest(request); assertEquals(request.getEndpoint() + ": failed", RestStatus.OK, RestStatus.fromCode(response.getStatusLine().getStatusCode())); - assertEquals(0, getDocCount(MODEL_INDEX_NAME)); - // Train Model again with same ModelId trainModel(modelId, trainingIndexName2, trainingFieldName, dimension); } diff --git a/src/test/java/org/opensearch/knn/plugin/action/RestGetModelHandlerIT.java b/src/test/java/org/opensearch/knn/plugin/action/RestGetModelHandlerIT.java index 20e0d58c3..a846b5dcd 100644 --- a/src/test/java/org/opensearch/knn/plugin/action/RestGetModelHandlerIT.java +++ b/src/test/java/org/opensearch/knn/plugin/action/RestGetModelHandlerIT.java @@ -12,6 +12,7 @@ package org.opensearch.knn.plugin.action; import joptsimple.internal.Strings; +import lombok.SneakyThrows; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.opensearch.client.Request; import org.opensearch.client.Response; @@ -21,7 +22,6 @@ import org.opensearch.knn.plugin.KNNPlugin; import org.opensearch.core.rest.RestStatus; -import java.io.IOException; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -44,9 +44,8 @@ public class RestGetModelHandlerIT extends KNNRestTestCase { - public void testGetModelExists() throws Exception { - createModelSystemIndex(); - + @SneakyThrows + public void testGetModel_whenModelIdExists_thenSucceed() { String modelId = "test-model-id"; String trainingIndexName = "train-index"; String trainingFieldName = "train-field"; @@ -81,8 +80,8 @@ public void testGetModelExists() throws Exception { assertEquals(L2.getValue(), responseMap.get(METHOD_PARAMETER_SPACE_TYPE)); } - public void testGetModelExistsWithFilter() throws Exception { - createModelSystemIndex(); + @SneakyThrows + public void testGetModel_whenFilterApplied_thenReturnExpectedFields() { String modelId = "test-model-id"; String trainingIndexName = "train-index"; String trainingFieldName = "train-field"; @@ -118,8 +117,7 @@ public void testGetModelExistsWithFilter() throws Exception { assertFalse(responseMap.containsKey(MODEL_STATE)); } - public void testGetModelFailsInvalid() throws IOException { - createModelSystemIndex(); + public void testGetModel_whenModelIDIsInValid_thenFail() { String restURI = String.join("/", KNNPlugin.KNN_BASE_URI, MODELS, "invalid-model-id"); Request request = new Request("GET", restURI); @@ -127,8 +125,7 @@ public void testGetModelFailsInvalid() throws IOException { assertTrue(ex.getMessage().contains("\"invalid-model-id\"")); } - public void testGetModelFailsBlank() throws IOException { - createModelSystemIndex(); + public void testGetModel_whenIDIsBlank_thenFail() { String restURI = String.join("/", KNNPlugin.KNN_BASE_URI, MODELS, " "); Request request = new Request("GET", restURI); diff --git a/src/test/java/org/opensearch/knn/plugin/action/RestKNNStatsHandlerIT.java b/src/test/java/org/opensearch/knn/plugin/action/RestKNNStatsHandlerIT.java index 411a51c3c..2e13c7049 100644 --- a/src/test/java/org/opensearch/knn/plugin/action/RestKNNStatsHandlerIT.java +++ b/src/test/java/org/opensearch/knn/plugin/action/RestKNNStatsHandlerIT.java @@ -339,7 +339,6 @@ public void testScriptStats_multipleShards() throws Exception { } public void testModelIndexHealthMetricsStats() throws Exception { - // Create request that filters only model index String modelIndexStatusName = StatNames.MODEL_INDEX_STATUS.getName(); // index can be created in one of previous tests, and as we do not delete it each test the check below became optional if (!systemIndexExists(MODEL_INDEX_NAME)) { @@ -351,7 +350,11 @@ public void testModelIndexHealthMetricsStats() throws Exception { // Check that model health status is null since model index is not created to system yet assertNull(statsMap.get(StatNames.MODEL_INDEX_STATUS.getName())); - createModelSystemIndex(); + // Train a model so that the system index will get created + createBasicKnnIndex(TRAINING_INDEX, TRAINING_FIELD, DIMENSION); + bulkIngestRandomVectors(TRAINING_INDEX, TRAINING_FIELD, NUM_DOCS, DIMENSION); + trainKnnModel(TEST_MODEL_ID, TRAINING_INDEX, TRAINING_FIELD, DIMENSION, MODEL_DESCRIPTION); + validateModelCreated(TEST_MODEL_ID); } Response response = getKnnStats(Collections.emptyList(), Arrays.asList(modelIndexStatusName)); diff --git a/src/test/java/org/opensearch/knn/plugin/action/RestSearchModelHandlerIT.java b/src/test/java/org/opensearch/knn/plugin/action/RestSearchModelHandlerIT.java index ae3e71528..e04c5e61c 100644 --- a/src/test/java/org/opensearch/knn/plugin/action/RestSearchModelHandlerIT.java +++ b/src/test/java/org/opensearch/knn/plugin/action/RestSearchModelHandlerIT.java @@ -11,6 +11,7 @@ package org.opensearch.knn.plugin.action; +import lombok.SneakyThrows; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.opensearch.action.search.SearchResponse; import org.opensearch.client.Request; @@ -19,22 +20,18 @@ import org.opensearch.core.xcontent.XContentParser; import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.knn.KNNRestTestCase; -import org.opensearch.knn.index.SpaceType; -import org.opensearch.knn.index.util.KNNEngine; import org.opensearch.knn.indices.Model; -import org.opensearch.knn.indices.ModelMetadata; -import org.opensearch.knn.indices.ModelState; import org.opensearch.knn.plugin.KNNPlugin; import org.opensearch.core.rest.RestStatus; import org.opensearch.search.SearchHit; -import java.io.IOException; import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; import static org.opensearch.knn.common.KNNConstants.MODELS; +import static org.opensearch.knn.common.KNNConstants.MODEL_INDEX_NAME; import static org.opensearch.knn.common.KNNConstants.PARAM_SIZE; import static org.opensearch.knn.common.KNNConstants.SEARCH_MODEL_MAX_SIZE; import static org.opensearch.knn.common.KNNConstants.SEARCH_MODEL_MIN_SIZE; @@ -47,12 +44,7 @@ public class RestSearchModelHandlerIT extends KNNRestTestCase { - private ModelMetadata getModelMetadata() { - return new ModelMetadata(KNNEngine.DEFAULT, SpaceType.DEFAULT, 4, ModelState.CREATED, "2021-03-27", "test model", "", ""); - } - - public void testNotSupportedParams() throws IOException { - createModelSystemIndex(); + public void testSearch_whenUnSupportedParamsPassed_thenFail() { String restURI = String.join("/", KNNPlugin.KNN_BASE_URI, MODELS, "_search"); Map invalidParams = new HashMap<>(); invalidParams.put("index", "index-name"); @@ -61,27 +53,31 @@ public void testNotSupportedParams() throws IOException { expectThrows(ResponseException.class, () -> client().performRequest(request)); } - public void testNoModelExists() throws Exception { - createModelSystemIndex(); + @SneakyThrows + public void testSearch_whenNoModelExists_thenReturnEmptyResults() { + // Currently, if the model index exists, we will return empty hits. If it does not exist, we will + // throw an exception. This is somewhat of a bug considering that the model index is supposed to be + // an implementation detail abstracted away from the user. However, in order to test, we need to handle + // the 2 different scenarios String restURI = String.join("/", KNNPlugin.KNN_BASE_URI, MODELS, "_search"); Request request = new Request("GET", restURI); request.setJsonEntity("{\n" + " \"query\": {\n" + " \"match_all\": {}\n" + " }\n" + "}"); - - Response response = client().performRequest(request); - assertEquals(RestStatus.OK, RestStatus.fromCode(response.getStatusLine().getStatusCode())); - - String responseBody = EntityUtils.toString(response.getEntity()); - assertNotNull(responseBody); - - XContentParser parser = createParser(MediaTypeRegistry.getDefaultMediaType().xContent(), responseBody); - SearchResponse searchResponse = SearchResponse.fromXContent(parser); - assertNotNull(searchResponse); - assertEquals(searchResponse.getHits().getHits().length, 0); - + if (!systemIndexExists(MODEL_INDEX_NAME)) { + ResponseException ex = expectThrows(ResponseException.class, () -> client().performRequest(request)); + assertEquals(RestStatus.NOT_FOUND.getStatus(), ex.getResponse().getStatusLine().getStatusCode()); + } else { + Response response = client().performRequest(request); + assertEquals(RestStatus.OK, RestStatus.fromCode(response.getStatusLine().getStatusCode())); + String responseBody = EntityUtils.toString(response.getEntity()); + assertNotNull(responseBody); + XContentParser parser = createParser(MediaTypeRegistry.getDefaultMediaType().xContent(), responseBody); + SearchResponse searchResponse = SearchResponse.fromXContent(parser); + assertNotNull(searchResponse); + assertEquals(searchResponse.getHits().getHits().length, 0); + } } - public void testSizeValidationFailsInvalidSize() throws IOException { - createModelSystemIndex(); + public void testSearch_whenInvalidSizePassed_thenFail() { for (Integer invalidSize : Arrays.asList(SEARCH_MODEL_MIN_SIZE - 1, SEARCH_MODEL_MAX_SIZE + 1)) { String restURI = String.join("/", KNNPlugin.KNN_BASE_URI, MODELS, "_search?" + PARAM_SIZE + "=" + invalidSize); Request request = new Request("GET", restURI); @@ -101,8 +97,8 @@ public void testSizeValidationFailsInvalidSize() throws IOException { } - public void testSearchModelExists() throws Exception { - createModelSystemIndex(); + @SneakyThrows + public void testSearch_whenModelExists_thenSuccess() { String trainingIndex = "irrelevant-index"; String trainingFieldName = "train-field"; int dimension = 8; @@ -151,7 +147,6 @@ public void testSearchModelExists() throws Exception { } public void testSearchModelWithoutSource() throws Exception { - createModelSystemIndex(); String trainingIndex = "irrelevant-index"; String trainingFieldName = "train-field"; int dimension = 8; @@ -192,7 +187,6 @@ public void testSearchModelWithoutSource() throws Exception { } public void testSearchModelWithSourceFilteringIncludes() throws Exception { - createModelSystemIndex(); String trainingIndex = "irrelevant-index"; String trainingFieldName = "train-field"; int dimension = 8; @@ -244,7 +238,6 @@ public void testSearchModelWithSourceFilteringIncludes() throws Exception { } public void testSearchModelWithSourceFilteringExcludes() throws Exception { - createModelSystemIndex(); String trainingIndex = "irrelevant-index"; String trainingFieldName = "train-field"; int dimension = 8; diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index 3af8b7ec8..1d984927f 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -5,8 +5,6 @@ package org.opensearch.knn; -import com.google.common.base.Charsets; -import com.google.common.io.Resources; import com.google.common.primitives.Floats; import lombok.extern.log4j.Log4j2; import org.apache.commons.lang.StringUtils; @@ -22,7 +20,6 @@ import org.opensearch.knn.index.query.KNNQueryBuilder; import org.opensearch.knn.index.KNNSettings; import org.opensearch.knn.index.SpaceType; -import org.opensearch.knn.indices.ModelDao; import org.opensearch.knn.indices.ModelState; import org.opensearch.knn.plugin.KNNPlugin; import org.opensearch.knn.plugin.script.KNNScoringScriptEngine; @@ -51,7 +48,6 @@ import java.io.IOException; import java.io.InputStream; -import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; @@ -77,8 +73,6 @@ import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NLIST; import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_SPACE_TYPE; import static org.opensearch.knn.common.KNNConstants.MODEL_DESCRIPTION; -import static org.opensearch.knn.common.KNNConstants.MODEL_INDEX_MAPPING_PATH; -import static org.opensearch.knn.common.KNNConstants.MODEL_INDEX_NAME; import static org.opensearch.knn.common.KNNConstants.MODEL_STATE; import static org.opensearch.knn.common.KNNConstants.TRAIN_FIELD_PARAMETER; import static org.opensearch.knn.common.KNNConstants.TRAIN_INDEX_PARAMETER; @@ -781,20 +775,6 @@ protected String getIndexSettingByName(String indexName, String settingName, boo } } - protected void createModelSystemIndex() throws IOException { - URL url = ModelDao.class.getClassLoader().getResource(MODEL_INDEX_MAPPING_PATH); - if (url == null) { - throw new IllegalStateException("Unable to retrieve mapping for \"" + MODEL_INDEX_NAME + "\""); - } - - String mapping = Resources.toString(url, Charsets.UTF_8); - mapping = mapping.substring(1, mapping.length() - 1); - - if (!systemIndexExists(MODEL_INDEX_NAME)) { - createIndex(MODEL_INDEX_NAME, Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 0).build(), mapping); - } - } - /** * Clear cache *