Fix NPE with multiple queries containing DOT(.) in index name. (opensearch-project#870)

* Do not remove opensearch from the list of registered catalogs.

Signed-off-by: MaxKsyunz <[email protected]>

* New Changes to handle bug:opensearch-project#866

Signed-off-by: vamsi-amazon <[email protected]>

Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: vamsi-amazon <[email protected]>
Co-authored-by: MaxKsyunz <[email protected]>

Author: 2 people

integ-test/src/test/java/org/opensearch/sql/sql/IdentifierIT.java

@@ -57,6 +57,13 @@ public void testSpecialFieldName() throws IOException {
     verifyDataRows(result, rows(10, 30));
   }
 
+  @Test
+  public void testMultipleQueriesWithSpecialIndexNames() throws IOException {
+    createIndexWithOneDoc("test.one", "test.two");
+    queryAndAssertTheDoc("SELECT * FROM test.one");
+    queryAndAssertTheDoc("SELECT * FROM test.two");
+  }
+
   private void createIndexWithOneDoc(String... indexNames) throws IOException {
     for (String indexName : indexNames) {
       new Index(indexName).addDoc("{\"age\": 30}");

plugin/src/main/java/org/opensearch/sql/plugin/catalog/CatalogServiceImpl.java

@@ -8,15 +8,18 @@
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URISyntaxException;
 import java.security.PrivilegedExceptionAction;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -36,7 +39,7 @@ public class CatalogServiceImpl implements CatalogService {
 
   private static final Logger LOG = LogManager.getLogger();
 
-  public static final String OPEN_SEARCH = "opensearch";
+  public static StorageEngine defaultOpenSearchStorageEngine;
 
   private Map<String, StorageEngine> storageEngineMap = new HashMap<>();
 
@@ -79,21 +82,22 @@ public void loadConnectors(Settings settings) {
   @Override
   public StorageEngine getStorageEngine(String catalog) {
     if (catalog == null || !storageEngineMap.containsKey(catalog)) {
-      return storageEngineMap.get(OPEN_SEARCH);
+      return defaultOpenSearchStorageEngine;
     }
     return storageEngineMap.get(catalog);
   }
 
   @Override
   public Set<String> getCatalogs() {
-    Set<String> catalogs = storageEngineMap.keySet();
-    catalogs.remove(OPEN_SEARCH);
-    return catalogs;
+    return Collections.unmodifiableSet(storageEngineMap.keySet());
   }
 
   @Override
   public void registerOpenSearchStorageEngine(StorageEngine storageEngine) {
-    storageEngineMap.put(OPEN_SEARCH, storageEngine);
+    if (storageEngine == null) {
+      throw new IllegalArgumentException("Default storage engine can't be null");
+    }
+    defaultOpenSearchStorageEngine = storageEngine;
   }
 
   private <T> T doPrivileged(PrivilegedExceptionAction<T> action) {
@@ -165,4 +169,4 @@ private void validateCatalogs(List<CatalogMetadata> catalogs) {
   }
 
 
-}
+}

plugin/src/test/java/org/opensearch/sql/plugin/catalog/CatalogServiceImplTest.java

@@ -15,15 +15,21 @@
 import lombok.SneakyThrows;
 import org.junit.Assert;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
 import org.opensearch.common.settings.MockSecureSettings;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.sql.storage.StorageEngine;
 
-
+@RunWith(MockitoJUnitRunner.class)
 public class CatalogServiceImplTest {
 
   public static final String CATALOG_SETTING_METADATA_KEY =
       "plugins.query.federation.catalog.config";
 
+  @Mock
+  private StorageEngine storageEngine;
 
   @SneakyThrows
   @Test
@@ -73,6 +79,19 @@ public void testLoadConnectorsWithMalformedJson() {
         () -> CatalogServiceImpl.getInstance().loadConnectors(settings));
   }
 
+  @SneakyThrows
+  @Test
+  public void testGetStorageEngineAfterGetCatalogs() {
+    Settings settings = getCatalogSettings("empty_catalog.json");
+    CatalogServiceImpl.getInstance().registerOpenSearchStorageEngine(storageEngine);
+    CatalogServiceImpl.getInstance().loadConnectors(settings);
+    Set<String> expected = new HashSet<>();
+    Assert.assertEquals(expected, CatalogServiceImpl.getInstance().getCatalogs());
+    Assert.assertEquals(storageEngine, CatalogServiceImpl.getInstance().getStorageEngine(null));
+    Assert.assertEquals(expected, CatalogServiceImpl.getInstance().getCatalogs());
+    Assert.assertEquals(storageEngine, CatalogServiceImpl.getInstance().getStorageEngine(null));
+  }
+
 
   private Settings getCatalogSettings(String filename) throws URISyntaxException, IOException {
     MockSecureSettings mockSecureSettings = new MockSecureSettings();

plugin/src/test/resources/empty_catalog.json

@@ -0,0 +1 @@
+[]