Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Sep 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<>();

Expand Down Expand Up @@ -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());
joshuali925 marked this conversation as resolved.
Show resolved Hide resolved
}

@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) {
Expand Down Expand Up @@ -165,4 +169,4 @@ private void validateCatalogs(List<CatalogMetadata> catalogs) {
}


}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions plugin/src/test/resources/empty_catalog.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[]