update addSource() interface

Signed-off-by: Peng Huo <[email protected]>

Author: penghuo

core/src/main/java/org/opensearch/sql/datasource/DataSourceService.java

@@ -32,9 +32,9 @@ public interface DataSourceService {
   /**
    * Register {@link DataSource} defined by {@link DataSourceMetadata}.
    *
-   * @param dataSourceMetadata {@link DataSourceMetadata}.
+   * @param metadatas list of {@link DataSourceMetadata}.
    */
-  void addDataSource(DataSourceMetadata dataSourceMetadata);
+  void addDataSource(DataSourceMetadata... metadatas);
 
   /**
    * remove all the registered {@link DataSource}.

core/src/main/java/org/opensearch/sql/datasource/DataSourceServiceImpl.java

@@ -5,13 +5,15 @@
 
 package org.opensearch.sql.datasource;
 
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
-import org.apache.commons.lang3.StringUtils;
+import org.opensearch.sql.common.utils.StringUtils;
 import org.opensearch.sql.datasource.model.DataSource;
 import org.opensearch.sql.datasource.model.DataSourceMetadata;
 import org.opensearch.sql.datasource.model.DataSourceType;
@@ -58,11 +60,13 @@ public DataSource getDataSource(String dataSourceName) {
   }
 
   @Override
-  public void addDataSource(DataSourceMetadata metadata) {
-    validateDataSource(metadata);
-    dataSourceMap.put(
-        metadata.getName(),
-        dataSourceFactoryMap.get(metadata.getConnector()).createDataSource(metadata));
+  public void addDataSource(DataSourceMetadata... metadatas) {
+    for (DataSourceMetadata metadata : metadatas) {
+      validateDataSourceMetaData(metadata);
+      dataSourceMap.put(
+          metadata.getName(),
+          dataSourceFactoryMap.get(metadata.getConnector()).createDataSource(metadata));
+    }
   }
 
   @Override
@@ -75,20 +79,22 @@ public void clear() {
    *
    * @param metadata {@link DataSourceMetadata}.
    */
-  private void validateDataSource(DataSourceMetadata metadata) {
-    if (StringUtils.isEmpty(metadata.getName())) {
-      throw new IllegalArgumentException(
-          "Missing Name Field from a DataSource. Name is a required parameter.");
-    }
-    if (!metadata.getName().matches(DATASOURCE_NAME_REGEX)) {
-      throw new IllegalArgumentException(
-          String.format(
-              "DataSource Name: %s contains illegal characters. Allowed characters: a-zA-Z0-9_-*@.",
-              metadata.getName()));
-    }
-    if (Objects.isNull(metadata.getProperties())) {
-      throw new IllegalArgumentException(
-          "Missing properties field in catalog configuration. Properties are required parameters.");
-    }
+  private void validateDataSourceMetaData(DataSourceMetadata metadata) {
+    Preconditions.checkArgument(
+        !Strings.isNullOrEmpty(metadata.getName()),
+        "Missing Name Field from a DataSource. Name is a required parameter.");
+    Preconditions.checkArgument(
+        !dataSourceMap.containsKey(metadata.getName()),
+        StringUtils.format(
+            "Datasource name should be unique, Duplicate datasource found %s.",
+            metadata.getName()));
+    Preconditions.checkArgument(
+        metadata.getName().matches(DATASOURCE_NAME_REGEX),
+        StringUtils.format(
+            "DataSource Name: %s contains illegal characters. Allowed characters: a-zA-Z0-9_-*@.",
+            metadata.getName()));
+    Preconditions.checkArgument(
+        !Objects.isNull(metadata.getProperties()),
+        "Missing properties field in catalog configuration. Properties are required parameters.");
   }
 }

core/src/test/java/org/opensearch/sql/analysis/AnalyzerTestBase.java

@@ -207,7 +207,7 @@ public DataSource getDataSource(String dataSourceName) {
     }
 
     @Override
-    public void addDataSource(DataSourceMetadata metadata) {
+    public void addDataSource(DataSourceMetadata... metadatas) {
       throw new UnsupportedOperationException();
     }
 

core/src/test/java/org/opensearch/sql/catalog/DataSourceServiceImplTest.java renamed to core/src/test/java/org/opensearch/sql/datasource/DataSourceServiceImplTest.java

@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package org.opensearch.sql.catalog;
+package org.opensearch.sql.datasource;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -15,13 +15,12 @@
 import com.google.common.collect.ImmutableMap;
 import java.util.HashSet;
 import java.util.Map;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
-import org.opensearch.sql.datasource.DataSourceService;
-import org.opensearch.sql.datasource.DataSourceServiceImpl;
 import org.opensearch.sql.datasource.model.DataSource;
 import org.opensearch.sql.datasource.model.DataSourceMetadata;
 import org.opensearch.sql.datasource.model.DataSourceType;
@@ -59,6 +58,11 @@ public void setup() {
             });
   }
 
+  @AfterEach
+  public void clear() {
+    dataSourceService.clear();
+  }
+
   @Test
   void getDataSourceSuccess() {
     dataSourceService.addDataSource(DataSourceMetadata.defaultOpenSearchDataSourceMetadata());
@@ -130,6 +134,20 @@ void metaDataMissingPropertiesShouldFail() {
         exception.getMessage());
   }
 
+  @Test
+  void metaDataHasDuplicateNameShouldFail() {
+    dataSourceService.addDataSource(metadata(NAME, DataSourceType.OPENSEARCH, ImmutableMap.of()));
+    assertEquals(1, dataSourceService.getDataSources().size());
+
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> dataSourceService.addDataSource(metadata(NAME, DataSourceType.OPENSEARCH, null)));
+    assertEquals(
+        String.format("Datasource name should be unique, Duplicate datasource found %s.", NAME),
+        exception.getMessage());
+  }
+
   DataSourceMetadata metadata(String name, DataSourceType type, Map<String, String> properties) {
     DataSourceMetadata dataSourceMetadata = new DataSourceMetadata();
     dataSourceMetadata.setName(name);

core/src/test/java/org/opensearch/sql/catalog/model/auth/AuthenticationTypeTest.java renamed to core/src/test/java/org/opensearch/sql/datasource/model/auth/AuthenticationTypeTest.java

@@ -3,14 +3,13 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package org.opensearch.sql.catalog.model.auth;
+package org.opensearch.sql.datasource.model.auth;
 
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 
 import org.junit.jupiter.api.Test;
-import org.opensearch.sql.datasource.model.auth.AuthenticationType;
 
 class AuthenticationTypeTest {
   @Test

plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java

@@ -18,11 +18,8 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Locale;
 import java.util.Objects;
-import java.util.Set;
 import java.util.function.Supplier;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -242,8 +239,7 @@ public static void loadDataSources(DataSourceService dataSourceService, Settings
             try {
               List<DataSourceMetadata> metadataList =
                   objectMapper.readValue(inputStream, new TypeReference<>() {});
-              verifyDuplicateName(metadataList);
-              metadataList.forEach(metadata -> dataSourceService.addDataSource(metadata));
+              dataSourceService.addDataSource(metadataList.toArray(new DataSourceMetadata[0]));
             } catch (IOException e) {
               LOG.error(
                   "DataSource Configuration File uploaded is malformed. Verify and re-upload.", e);
@@ -254,18 +250,4 @@ public static void loadDataSources(DataSourceService dataSourceService, Settings
           return null;
         });
   }
-
-  static void verifyDuplicateName(List<DataSourceMetadata> metadataList) {
-    Set<String> seenNames = new HashSet<>();
-    for (DataSourceMetadata metadata : metadataList) {
-      if (seenNames.contains(metadata.getName())) {
-        throw new IllegalArgumentException(
-            String.format(
-                Locale.ROOT,
-                "Datasource name should be unique, Duplicate datasource found %s",
-                metadata.getName()));
-      }
-      seenNames.add(metadata.getName());
-    }
-  }
 }

plugin/src/test/java/org/opensearch/sql/plugin/datasource/DataSourceServiceImplTest.java renamed to plugin/src/test/java/org/opensearch/sql/plugin/datasource/DataSourceMetaDataTest.java

@@ -34,9 +34,9 @@
 import org.opensearch.sql.plugin.SQLPlugin;
 
 @RunWith(MockitoJUnitRunner.class)
-public class DataSourceServiceImplTest {
+public class DataSourceMetaDataTest {
 
-  public static final String CATALOG_SETTING_METADATA_KEY =
+  public static final String DATASOURCE_SETTING_METADATA_KEY =
       "plugins.query.federation.datasources.config";
 
   @Mock
@@ -45,7 +45,7 @@ public class DataSourceServiceImplTest {
   @SneakyThrows
   @Test
   public void testLoadConnectors() {
-    Settings settings = getCatalogSettings("datasources.json");
+    Settings settings = getDataSourceSettings("datasources.json");
     loadConnectors(settings);
     List<DataSourceMetadata> expected =
         new ArrayList<>() {
@@ -67,8 +67,8 @@ public void testLoadConnectors() {
 
   @SneakyThrows
   @Test
-  public void testLoadConnectorsWithMultipleCatalogs() {
-    Settings settings = getCatalogSettings("multiple_datasources.json");
+  public void testLoadConnectorsWithMultipleDataSources() {
+    Settings settings = getDataSourceSettings("multiple_datasources.json");
     loadConnectors(settings);
     List<DataSourceMetadata> expected = new ArrayList<>() {{
         add(metadata("prometheus", DataSourceType.PROMETHEUS, ImmutableMap.of(
@@ -89,29 +89,20 @@ public void testLoadConnectorsWithMultipleCatalogs() {
     verifyAddDataSourceWithMetadata(expected);
   }
 
-  @SneakyThrows
-  @Test
-  public void testLoadConnectorsWithDuplicateCatalogNames() {
-    Settings settings = getCatalogSettings("duplicate_datasource_names.json");
-    loadConnectors(settings);
-
-    verify(dataSourceService, never()).addDataSource(any());
-  }
-
   @SneakyThrows
   @Test
   public void testLoadConnectorsWithMalformedJson() {
-    Settings settings = getCatalogSettings("malformed_datasources.json");
+    Settings settings = getDataSourceSettings("malformed_datasources.json");
     loadConnectors(settings);
 
     verify(dataSourceService, never()).addDataSource(any());
   }
 
-  private Settings getCatalogSettings(String filename) throws URISyntaxException, IOException {
+  private Settings getDataSourceSettings(String filename) throws URISyntaxException, IOException {
     MockSecureSettings mockSecureSettings = new MockSecureSettings();
     ClassLoader classLoader = getClass().getClassLoader();
     Path filepath = Paths.get(classLoader.getResource(filename).toURI());
-    mockSecureSettings.setFile(CATALOG_SETTING_METADATA_KEY, Files.readAllBytes(filepath));
+    mockSecureSettings.setFile(DATASOURCE_SETTING_METADATA_KEY, Files.readAllBytes(filepath));
     return Settings.builder().setSecureSettings(mockSecureSettings).build();
   }
 
@@ -120,11 +111,11 @@ void loadConnectors(Settings settings) {
   }
 
   void verifyAddDataSourceWithMetadata(List<DataSourceMetadata> metadataList) {
-    int expectCount = metadataList.size();
     ArgumentCaptor<DataSourceMetadata> metadataCaptor =
         ArgumentCaptor.forClass(DataSourceMetadata.class);
-    verify(dataSourceService, times(expectCount)).addDataSource(metadataCaptor.capture());
+    verify(dataSourceService, times(1)).addDataSource(metadataCaptor.capture());
     List<DataSourceMetadata> actualValues = metadataCaptor.getAllValues();
+    assertEquals(metadataList.size(), actualValues.size());
     assertEquals(metadataList, actualValues);
   }