Fix IndexServiceTests sort field test cases (#18352)

As far as I can tell these test cases have never been working as
intended. They appear to have been created as a copy-paste error from
`testIllegalFsyncInterval`. They passed because the createIndex call was
failing due to setting the translog sync interval to an invalid value,
which is what the try-catch was expecting. After removing the invalid
setting the tests did not work properly for multiple reasons. I believe
I have fixed the tests back to the original intent.

The reason for digging into this test was to remove the deprecated
`createIndex` calls from OpenSearchSingleNodeTestCase which take the now
removed type parameter. I will follow up with a separate PR to do that
work, as this was a significant enough change to stand on its own.

Signed-off-by: Andrew Ross <andrross@amazon.com>

Author: andrross

server/src/test/java/org/opensearch/index/IndexServiceTests.java

@@ -34,6 +34,7 @@
 
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.SortField;
+import org.apache.lucene.search.SortedNumericSortField;
 import org.apache.lucene.search.TopDocs;
 import org.opensearch.Version;
 import org.opensearch.action.support.ActiveShardCount;
@@ -61,16 +62,20 @@
 import org.opensearch.threadpool.ThreadPool;
 
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 
 import static org.opensearch.index.shard.IndexShardTestCase.getEngine;
 import static org.opensearch.test.InternalSettingsPlugin.TRANSLOG_RETENTION_CHECK_INTERVAL_SETTING;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.core.IsEqual.equalTo;
 
 /** Unit test(s) for IndexService */
@@ -81,6 +86,11 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
         return Collections.singleton(InternalSettingsPlugin.class);
     }
 
+    @Override
+    protected boolean forbidPrivateIndexSettings() {
+        return false;
+    }
+
     public static CompressedXContent filter(QueryBuilder filterBuilder) throws IOException {
         XContentBuilder builder = XContentFactory.jsonBuilder();
         filterBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS);
@@ -535,64 +545,41 @@ public void testUpdateRemoteTranslogBufferIntervalDynamically() {
     }
 
     public void testIndexSort() {
-        Settings settings = Settings.builder()
-            .put(IndexSettings.INDEX_TRANSLOG_SYNC_INTERVAL_SETTING.getKey(), "0ms") // disable
-            .putList("index.sort.field", "sortfield")
-            .build();
-        try {
-            // Integer index sort should be remained to int sort type
-            IndexService index = createIndex("test", settings, createTestMapping("integer"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.INT);
-
-            // Long index sort should be remained to long sort type
-            index = createIndex("test", settings, createTestMapping("long"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.LONG);
-
-            // Float index sort should be remained to float sort type
-            index = createIndex("test", settings, createTestMapping("float"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.FLOAT);
-
-            // Double index sort should be remained to double sort type
-            index = createIndex("test", settings, createTestMapping("double"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.DOUBLE);
-
-            // String index sort should be remained to string sort type
-            index = createIndex("test", settings, createTestMapping("string"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.STRING);
-        } catch (IllegalArgumentException ex) {
-            assertEquals("failed to parse value [0ms] for setting [index.translog.sync_interval], must be >= [100ms]", ex.getMessage());
-        }
+        runSortFieldTest(Settings.builder(), SortField.Type.INT);
     }
 
     public void testIndexSortBackwardCompatible() {
-        Settings settings = Settings.builder()
-            .put(IndexSettings.INDEX_TRANSLOG_SYNC_INTERVAL_SETTING.getKey(), "0ms") // disable
-            .put(IndexMetadata.SETTING_INDEX_VERSION_CREATED.getKey(), Version.V_2_6_1)
-            .putList("index.sort.field", "sortfield")
-            .build();
-        try {
-            // Integer index sort should be converted to long sort type
-            IndexService index = createIndex("test", settings, createTestMapping("integer"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.LONG);
-
-            // Long index sort should be remained to long sort type
-            index = createIndex("test", settings, createTestMapping("long"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.LONG);
-
-            // Float index sort should be remained to float sort type
-            index = createIndex("test", settings, createTestMapping("float"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.FLOAT);
-
-            // Double index sort should be remained to double sort type
-            index = createIndex("test", settings, createTestMapping("double"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.DOUBLE);
+        runSortFieldTest(
+            Settings.builder().put(IndexMetadata.SETTING_INDEX_VERSION_CREATED.getKey(), Version.V_2_6_1),
+            SortField.Type.LONG
+        );
+    }
 
-            // String index sort should be remained to string sort type
-            index = createIndex("test", settings, createTestMapping("string"));
-            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.STRING);
-        } catch (IllegalArgumentException ex) {
-            assertEquals("failed to parse value [0ms] for setting [index.translog.sync_interval], must be >= [100ms]", ex.getMessage());
-        }
+    private void runSortFieldTest(Settings.Builder settingsBuilder, SortField.Type expectedIntType) {
+        String[] sortFieldNames = new String[] { "int-sortfield", "long-sortfield", "double-sortfield", "float-sortfield" };
+        Settings settings = settingsBuilder.putList("index.sort.field", sortFieldNames).build();
+        IndexService index = createIndexWithSimpleMappings(
+            "sort-field-test-index",
+            settings,
+            "int-sortfield",
+            "type=integer",
+            "long-sortfield",
+            "type=long",
+            "double-sortfield",
+            "type=double",
+            "float-sortfield",
+            "type=float"
+        );
+        SortField[] sortFields = index.getIndexSortSupplier().get().getSort();
+        Map<String, SortField.Type> map = Arrays.stream(sortFields)
+            .filter(s -> s instanceof SortedNumericSortField)
+            .map(s -> (SortedNumericSortField) s)
+            .collect(Collectors.toMap(SortField::getField, SortedNumericSortField::getNumericType));
+        assertThat(map.keySet(), containsInAnyOrder(sortFieldNames));
+        assertThat(map.get("int-sortfield"), equalTo(expectedIntType));
+        assertThat(map.get("long-sortfield"), equalTo(SortField.Type.LONG));
+        assertThat(map.get("float-sortfield"), equalTo(SortField.Type.FLOAT));
+        assertThat(map.get("double-sortfield"), equalTo(SortField.Type.DOUBLE));
     }
 
     public void testReplicationTask() throws Exception {
@@ -843,15 +830,4 @@ public void testRefreshTaskUpdatesWithDynamicShardLevelRefreshes() throws Except
         client().admin().cluster().prepareUpdateSettings().setTransientSettings(Settings.builder().putNull("*")).get();
 
     }
-
-    private static String createTestMapping(String type) {
-        return "  \"properties\": {\n"
-            + "    \"test\": {\n"
-            + "      \"type\": \"text\"\n"
-            + "    },\n"
-            + "    \"sortfield\": {\n"
-            + "      \"type\": \" + type + \"\n"
-            + "    }\n"
-            + "  }";
-    }
 }

test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java

@@ -368,6 +368,17 @@ protected IndexService createIndex(String index, Settings settings, String type,
         return createIndex(index, createIndexRequestBuilder);
     }
 
+    /**
+     * Creates an index with mappings provided in the format expected by {@link org.opensearch.action.admin.indices.create.CreateIndexRequest#simpleMapping(String...)}
+     */
+    protected IndexService createIndexWithSimpleMappings(String index, Settings settings, String... mappings) {
+        CreateIndexRequestBuilder createIndexRequestBuilder = client().admin().indices().prepareCreate(index).setSettings(settings);
+        if (mappings != null) {
+            createIndexRequestBuilder.setMapping(mappings);
+        }
+        return createIndex(index, createIndexRequestBuilder);
+    }
+
     protected IndexService createIndex(String index, CreateIndexRequestBuilder createIndexRequestBuilder) {
         assertAcked(createIndexRequestBuilder.get());
         // Wait for the index to be allocated so that cluster state updates don't override