From 55409eb894be7fc476e8d69efc5431f03bce524a Mon Sep 17 00:00:00 2001 From: Eric Date: Wed, 8 Nov 2023 21:19:38 +0000 Subject: [PATCH] Revert "Implement patch API for datasources (#2273) (#2329)" This reverts commit 4c151fe07ca2da7a54dcc4a11945d7acf92ec6fb. Signed-off-by: Eric --- .../sql/datasource/DataSourceService.java | 10 +-- .../sql/analysis/AnalyzerTestBase.java | 3 - .../PatchDataSourceActionRequest.java | 49 ------------ .../PatchDataSourceActionResponse.java | 31 -------- .../rest/RestDataSourceQueryAction.java | 61 ++++---------- .../service/DataSourceServiceImpl.java | 50 ++---------- .../TransportPatchDataSourceAction.java | 74 ----------------- .../utils/XContentParserUtils.java | 72 ----------------- .../service/DataSourceServiceImplTest.java | 42 +--------- .../TransportPatchDataSourceActionTest.java | 79 ------------------- .../utils/XContentParserUtilsTest.java | 74 ----------------- docs/user/ppl/admin/datasources.rst | 14 ---- .../sql/datasource/DataSourceAPIsIT.java | 29 ------- .../sql/legacy/SQLIntegTestCase.java | 10 --- .../org/opensearch/sql/plugin/SQLPlugin.java | 14 ++-- 15 files changed, 32 insertions(+), 580 deletions(-) delete mode 100644 datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionRequest.java delete mode 100644 datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionResponse.java delete mode 100644 datasources/src/main/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceAction.java delete mode 100644 datasources/src/test/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceActionTest.java diff --git a/core/src/main/java/org/opensearch/sql/datasource/DataSourceService.java b/core/src/main/java/org/opensearch/sql/datasource/DataSourceService.java index 162fe9e8f8..6dace50f99 100644 --- a/core/src/main/java/org/opensearch/sql/datasource/DataSourceService.java +++ b/core/src/main/java/org/opensearch/sql/datasource/DataSourceService.java @@ -5,7 +5,6 @@ package org.opensearch.sql.datasource; -import java.util.Map; import java.util.Set; import org.opensearch.sql.datasource.model.DataSource; import org.opensearch.sql.datasource.model.DataSourceMetadata; @@ -57,19 +56,12 @@ public interface DataSourceService { void createDataSource(DataSourceMetadata metadata); /** - * Updates {@link DataSource} corresponding to dataSourceMetadata (all fields needed). + * Updates {@link DataSource} corresponding to dataSourceMetadata. * * @param dataSourceMetadata {@link DataSourceMetadata}. */ void updateDataSource(DataSourceMetadata dataSourceMetadata); - /** - * Patches {@link DataSource} corresponding to the given name (only fields to be changed needed). - * - * @param dataSourceData - */ - void patchDataSource(Map dataSourceData); - /** * Deletes {@link DataSource} corresponding to the DataSource name. * diff --git a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTestBase.java b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTestBase.java index 569cdd96f8..508567582b 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTestBase.java +++ b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTestBase.java @@ -231,9 +231,6 @@ public DataSource getDataSource(String dataSourceName) { @Override public void updateDataSource(DataSourceMetadata dataSourceMetadata) {} - @Override - public void patchDataSource(Map dataSourceData) {} - @Override public void deleteDataSource(String dataSourceName) {} diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionRequest.java b/datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionRequest.java deleted file mode 100644 index 9443ea561e..0000000000 --- a/datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionRequest.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * - * * Copyright OpenSearch Contributors - * * SPDX-License-Identifier: Apache-2.0 - * - */ - -package org.opensearch.sql.datasources.model.transport; - -import static org.opensearch.sql.analysis.DataSourceSchemaIdentifierNameResolver.DEFAULT_DATASOURCE_NAME; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.CONNECTOR_FIELD; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.NAME_FIELD; - -import java.io.IOException; -import java.util.Map; -import lombok.Getter; -import org.opensearch.action.ActionRequest; -import org.opensearch.action.ActionRequestValidationException; -import org.opensearch.core.common.io.stream.StreamInput; - -public class PatchDataSourceActionRequest extends ActionRequest { - - @Getter private Map dataSourceData; - - /** Constructor of UpdateDataSourceActionRequest from StreamInput. */ - public PatchDataSourceActionRequest(StreamInput in) throws IOException { - super(in); - } - - public PatchDataSourceActionRequest(Map dataSourceData) { - this.dataSourceData = dataSourceData; - } - - @Override - public ActionRequestValidationException validate() { - if (this.dataSourceData.get(NAME_FIELD).equals(DEFAULT_DATASOURCE_NAME)) { - ActionRequestValidationException exception = new ActionRequestValidationException(); - exception.addValidationError( - "Not allowed to update datasource with name : " + DEFAULT_DATASOURCE_NAME); - return exception; - } else if (this.dataSourceData.get(CONNECTOR_FIELD) != null) { - ActionRequestValidationException exception = new ActionRequestValidationException(); - exception.addValidationError("Not allowed to update connector for datasource"); - return exception; - } else { - return null; - } - } -} diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionResponse.java b/datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionResponse.java deleted file mode 100644 index 18873a6731..0000000000 --- a/datasources/src/main/java/org/opensearch/sql/datasources/model/transport/PatchDataSourceActionResponse.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * - * * Copyright OpenSearch Contributors - * * SPDX-License-Identifier: Apache-2.0 - * - */ - -package org.opensearch.sql.datasources.model.transport; - -import java.io.IOException; -import lombok.Getter; -import lombok.RequiredArgsConstructor; -import org.opensearch.core.action.ActionResponse; -import org.opensearch.core.common.io.stream.StreamInput; -import org.opensearch.core.common.io.stream.StreamOutput; - -@RequiredArgsConstructor -public class PatchDataSourceActionResponse extends ActionResponse { - - @Getter private final String result; - - public PatchDataSourceActionResponse(StreamInput in) throws IOException { - super(in); - result = in.readString(); - } - - @Override - public void writeTo(StreamOutput streamOutput) throws IOException { - streamOutput.writeString(result); - } -} diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java b/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java index c207f55738..2947afc5b9 100644 --- a/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java +++ b/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java @@ -10,13 +10,15 @@ import static org.opensearch.core.rest.RestStatus.BAD_REQUEST; import static org.opensearch.core.rest.RestStatus.NOT_FOUND; import static org.opensearch.core.rest.RestStatus.SERVICE_UNAVAILABLE; -import static org.opensearch.rest.RestRequest.Method.*; +import static org.opensearch.rest.RestRequest.Method.DELETE; +import static org.opensearch.rest.RestRequest.Method.GET; +import static org.opensearch.rest.RestRequest.Method.POST; +import static org.opensearch.rest.RestRequest.Method.PUT; import com.google.common.collect.ImmutableList; import java.io.IOException; import java.util.List; import java.util.Locale; -import java.util.Map; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.OpenSearchException; @@ -30,8 +32,18 @@ import org.opensearch.sql.datasource.model.DataSourceMetadata; import org.opensearch.sql.datasources.exceptions.DataSourceNotFoundException; import org.opensearch.sql.datasources.exceptions.ErrorMessage; -import org.opensearch.sql.datasources.model.transport.*; -import org.opensearch.sql.datasources.transport.*; +import org.opensearch.sql.datasources.model.transport.CreateDataSourceActionRequest; +import org.opensearch.sql.datasources.model.transport.CreateDataSourceActionResponse; +import org.opensearch.sql.datasources.model.transport.DeleteDataSourceActionRequest; +import org.opensearch.sql.datasources.model.transport.DeleteDataSourceActionResponse; +import org.opensearch.sql.datasources.model.transport.GetDataSourceActionRequest; +import org.opensearch.sql.datasources.model.transport.GetDataSourceActionResponse; +import org.opensearch.sql.datasources.model.transport.UpdateDataSourceActionRequest; +import org.opensearch.sql.datasources.model.transport.UpdateDataSourceActionResponse; +import org.opensearch.sql.datasources.transport.TransportCreateDataSourceAction; +import org.opensearch.sql.datasources.transport.TransportDeleteDataSourceAction; +import org.opensearch.sql.datasources.transport.TransportGetDataSourceAction; +import org.opensearch.sql.datasources.transport.TransportUpdateDataSourceAction; import org.opensearch.sql.datasources.utils.Scheduler; import org.opensearch.sql.datasources.utils.XContentParserUtils; @@ -86,17 +98,6 @@ public List routes() { */ new Route(PUT, BASE_DATASOURCE_ACTION_URL), - /* - * PATCH datasources - * Request body: - * Ref - * [org.opensearch.sql.plugin.transport.datasource.model.PatchDataSourceActionRequest] - * Response body: - * Ref - * [org.opensearch.sql.plugin.transport.datasource.model.PatchDataSourceActionResponse] - */ - new Route(PATCH, BASE_DATASOURCE_ACTION_URL), - /* * DELETE datasources * Request body: Ref @@ -121,8 +122,6 @@ protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient return executeUpdateRequest(restRequest, nodeClient); case DELETE: return executeDeleteRequest(restRequest, nodeClient); - case PATCH: - return executePatchRequest(restRequest, nodeClient); default: return restChannel -> restChannel.sendResponse( @@ -217,34 +216,6 @@ public void onFailure(Exception e) { })); } - private RestChannelConsumer executePatchRequest(RestRequest restRequest, NodeClient nodeClient) - throws IOException { - Map dataSourceData = XContentParserUtils.toMap(restRequest.contentParser()); - return restChannel -> - Scheduler.schedule( - nodeClient, - () -> - nodeClient.execute( - TransportPatchDataSourceAction.ACTION_TYPE, - new PatchDataSourceActionRequest(dataSourceData), - new ActionListener<>() { - @Override - public void onResponse( - PatchDataSourceActionResponse patchDataSourceActionResponse) { - restChannel.sendResponse( - new BytesRestResponse( - RestStatus.OK, - "application/json; charset=UTF-8", - patchDataSourceActionResponse.getResult())); - } - - @Override - public void onFailure(Exception e) { - handleException(e, restChannel); - } - })); - } - private RestChannelConsumer executeDeleteRequest(RestRequest restRequest, NodeClient nodeClient) { String dataSourceName = restRequest.param("dataSourceName"); diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/service/DataSourceServiceImpl.java b/datasources/src/main/java/org/opensearch/sql/datasources/service/DataSourceServiceImpl.java index 8ba618fb44..25e8006d66 100644 --- a/datasources/src/main/java/org/opensearch/sql/datasources/service/DataSourceServiceImpl.java +++ b/datasources/src/main/java/org/opensearch/sql/datasources/service/DataSourceServiceImpl.java @@ -6,11 +6,15 @@ package org.opensearch.sql.datasources.service; import static org.opensearch.sql.analysis.DataSourceSchemaIdentifierNameResolver.DEFAULT_DATASOURCE_NAME; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.*; import com.google.common.base.Preconditions; import com.google.common.base.Strings; -import java.util.*; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; import org.opensearch.sql.common.utils.StringUtils; import org.opensearch.sql.datasource.DataSourceService; import org.opensearch.sql.datasource.model.DataSource; @@ -96,19 +100,6 @@ public void updateDataSource(DataSourceMetadata dataSourceMetadata) { } } - @Override - public void patchDataSource(Map dataSourceData) { - if (!dataSourceData.get(NAME_FIELD).equals(DEFAULT_DATASOURCE_NAME)) { - DataSourceMetadata dataSourceMetadata = - getRawDataSourceMetadata((String) dataSourceData.get(NAME_FIELD)); - replaceOldDatasourceMetadata(dataSourceData, dataSourceMetadata); - updateDataSource(dataSourceMetadata); - } else { - throw new UnsupportedOperationException( - "Not allowed to update default datasource :" + DEFAULT_DATASOURCE_NAME); - } - } - @Override public void deleteDataSource(String dataSourceName) { if (dataSourceName.equals(DEFAULT_DATASOURCE_NAME)) { @@ -145,35 +136,6 @@ private void validateDataSourceMetaData(DataSourceMetadata metadata) { + " Properties are required parameters."); } - /** - * Replaces the fields in the map of the given metadata. - * - * @param dataSourceData - * @param metadata {@link DataSourceMetadata}. - */ - private void replaceOldDatasourceMetadata( - Map dataSourceData, DataSourceMetadata metadata) { - - for (String key : dataSourceData.keySet()) { - switch (key) { - // Name and connector should not be modified - case DESCRIPTION_FIELD: - metadata.setDescription((String) dataSourceData.get(DESCRIPTION_FIELD)); - break; - case ALLOWED_ROLES_FIELD: - metadata.setAllowedRoles((List) dataSourceData.get(ALLOWED_ROLES_FIELD)); - break; - case PROPERTIES_FIELD: - Map properties = new HashMap<>(metadata.getProperties()); - properties.putAll(((Map) dataSourceData.get(PROPERTIES_FIELD))); - break; - case NAME_FIELD: - case CONNECTOR_FIELD: - break; - } - } - } - @Override public DataSourceMetadata getRawDataSourceMetadata(String dataSourceName) { if (dataSourceName.equals(DEFAULT_DATASOURCE_NAME)) { diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceAction.java b/datasources/src/main/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceAction.java deleted file mode 100644 index 303e905cec..0000000000 --- a/datasources/src/main/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceAction.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * - * * Copyright OpenSearch Contributors - * * SPDX-License-Identifier: Apache-2.0 - * - */ - -package org.opensearch.sql.datasources.transport; - -import static org.opensearch.sql.datasources.utils.XContentParserUtils.NAME_FIELD; -import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY; - -import org.opensearch.action.ActionType; -import org.opensearch.action.support.ActionFilters; -import org.opensearch.action.support.HandledTransportAction; -import org.opensearch.common.inject.Inject; -import org.opensearch.core.action.ActionListener; -import org.opensearch.sql.datasource.DataSourceService; -import org.opensearch.sql.datasources.model.transport.PatchDataSourceActionRequest; -import org.opensearch.sql.datasources.model.transport.PatchDataSourceActionResponse; -import org.opensearch.sql.datasources.service.DataSourceServiceImpl; -import org.opensearch.sql.protocol.response.format.JsonResponseFormatter; -import org.opensearch.tasks.Task; -import org.opensearch.transport.TransportService; - -public class TransportPatchDataSourceAction - extends HandledTransportAction { - - public static final String NAME = "cluster:admin/opensearch/ql/datasources/patch"; - public static final ActionType ACTION_TYPE = - new ActionType<>(NAME, PatchDataSourceActionResponse::new); - - private DataSourceService dataSourceService; - - /** - * TransportPatchDataSourceAction action for updating datasource. - * - * @param transportService transportService. - * @param actionFilters actionFilters. - * @param dataSourceService dataSourceService. - */ - @Inject - public TransportPatchDataSourceAction( - TransportService transportService, - ActionFilters actionFilters, - DataSourceServiceImpl dataSourceService) { - super( - TransportPatchDataSourceAction.NAME, - transportService, - actionFilters, - PatchDataSourceActionRequest::new); - this.dataSourceService = dataSourceService; - } - - @Override - protected void doExecute( - Task task, - PatchDataSourceActionRequest request, - ActionListener actionListener) { - try { - dataSourceService.patchDataSource(request.getDataSourceData()); - String responseContent = - new JsonResponseFormatter(PRETTY) { - @Override - protected Object buildJsonObject(String response) { - return response; - } - }.format("Updated DataSource with name " + request.getDataSourceData().get(NAME_FIELD)); - actionListener.onResponse(new PatchDataSourceActionResponse(responseContent)); - } catch (Exception e) { - actionListener.onFailure(e); - } - } -} diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/utils/XContentParserUtils.java b/datasources/src/main/java/org/opensearch/sql/datasources/utils/XContentParserUtils.java index 6af2a5a761..261f13870a 100644 --- a/datasources/src/main/java/org/opensearch/sql/datasources/utils/XContentParserUtils.java +++ b/datasources/src/main/java/org/opensearch/sql/datasources/utils/XContentParserUtils.java @@ -90,59 +90,6 @@ public static DataSourceMetadata toDataSourceMetadata(XContentParser parser) thr name, description, connector, allowedRoles, properties, resultIndex); } - public static Map toMap(XContentParser parser) throws IOException { - Map resultMap = new HashMap<>(); - String name; - String description; - List allowedRoles = new ArrayList<>(); - Map properties = new HashMap<>(); - String resultIndex; - ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); - while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String fieldName = parser.currentName(); - parser.nextToken(); - switch (fieldName) { - case NAME_FIELD: - name = parser.textOrNull(); - resultMap.put(NAME_FIELD, name); - break; - case DESCRIPTION_FIELD: - description = parser.textOrNull(); - resultMap.put(DESCRIPTION_FIELD, description); - break; - case CONNECTOR_FIELD: - // no-op - datasource connector should not be modified - break; - case ALLOWED_ROLES_FIELD: - while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - allowedRoles.add(parser.text()); - } - resultMap.put(ALLOWED_ROLES_FIELD, allowedRoles); - break; - case PROPERTIES_FIELD: - ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.currentToken(), parser); - while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String key = parser.currentName(); - parser.nextToken(); - String value = parser.textOrNull(); - properties.put(key, value); - } - resultMap.put(PROPERTIES_FIELD, properties); - break; - case RESULT_INDEX_FIELD: - resultIndex = parser.textOrNull(); - resultMap.put(RESULT_INDEX_FIELD, resultIndex); - break; - default: - throw new IllegalArgumentException("Unknown field: " + fieldName); - } - } - if (resultMap.get(NAME_FIELD) == null || resultMap.get(NAME_FIELD) == "") { - throw new IllegalArgumentException("Name is a required field."); - } - return resultMap; - } - /** * Converts json string to DataSourceMetadata. * @@ -162,25 +109,6 @@ public static DataSourceMetadata toDataSourceMetadata(String json) throws IOExce } } - /** - * Converts json string to Map. - * - * @param json jsonstring. - * @return DataSourceData - * @throws IOException IOException. - */ - public static Map toMap(String json) throws IOException { - try (XContentParser parser = - XContentType.JSON - .xContent() - .createParser( - NamedXContentRegistry.EMPTY, - DeprecationHandler.THROW_UNSUPPORTED_OPERATION, - json)) { - return toMap(parser); - } - } - /** * Converts DataSourceMetadata to XContentBuilder. * diff --git a/datasources/src/test/java/org/opensearch/sql/datasources/service/DataSourceServiceImplTest.java b/datasources/src/test/java/org/opensearch/sql/datasources/service/DataSourceServiceImplTest.java index c62e586dae..6164d8b73f 100644 --- a/datasources/src/test/java/org/opensearch/sql/datasources/service/DataSourceServiceImplTest.java +++ b/datasources/src/test/java/org/opensearch/sql/datasources/service/DataSourceServiceImplTest.java @@ -18,7 +18,6 @@ import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import static org.opensearch.sql.analysis.DataSourceSchemaIdentifierNameResolver.DEFAULT_DATASOURCE_NAME; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.*; import com.google.common.collect.ImmutableMap; import java.util.ArrayList; @@ -265,6 +264,7 @@ void testGetDataSourceMetadataSetWithDefaultDatasource() { @Test void testUpdateDataSourceSuccessCase() { + DataSourceMetadata dataSourceMetadata = metadata("testDS", DataSourceType.OPENSEARCH, Collections.emptyList(), ImmutableMap.of()); dataSourceService.updateDataSource(dataSourceMetadata); @@ -289,46 +289,6 @@ void testUpdateDefaultDataSource() { unsupportedOperationException.getMessage()); } - @Test - void testPatchDefaultDataSource() { - Map dataSourceData = - Map.of(NAME_FIELD, DEFAULT_DATASOURCE_NAME, DESCRIPTION_FIELD, "test"); - UnsupportedOperationException unsupportedOperationException = - assertThrows( - UnsupportedOperationException.class, - () -> dataSourceService.patchDataSource(dataSourceData)); - assertEquals( - "Not allowed to update default datasource :" + DEFAULT_DATASOURCE_NAME, - unsupportedOperationException.getMessage()); - } - - @Test - void testPatchDataSourceSuccessCase() { - // Tests that patch underlying implementation is to call update - Map dataSourceData = - new HashMap<>( - Map.of( - NAME_FIELD, - "testDS", - DESCRIPTION_FIELD, - "test", - CONNECTOR_FIELD, - "PROMETHEUS", - ALLOWED_ROLES_FIELD, - new ArrayList<>(), - PROPERTIES_FIELD, - Map.of(), - RESULT_INDEX_FIELD, - "")); - DataSourceMetadata getData = - metadata("testDS", DataSourceType.OPENSEARCH, Collections.emptyList(), ImmutableMap.of()); - when(dataSourceMetadataStorage.getDataSourceMetadata("testDS")) - .thenReturn(Optional.ofNullable(getData)); - - dataSourceService.patchDataSource(dataSourceData); - verify(dataSourceMetadataStorage, times(1)).updateDataSourceMetadata(any()); - } - @Test void testDeleteDatasource() { dataSourceService.deleteDataSource("testDS"); diff --git a/datasources/src/test/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceActionTest.java b/datasources/src/test/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceActionTest.java deleted file mode 100644 index 5e1e7df112..0000000000 --- a/datasources/src/test/java/org/opensearch/sql/datasources/transport/TransportPatchDataSourceActionTest.java +++ /dev/null @@ -1,79 +0,0 @@ -package org.opensearch.sql.datasources.transport; - -import static org.mockito.Mockito.*; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.*; - -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.mockito.junit.jupiter.MockitoExtension; -import org.opensearch.action.support.ActionFilters; -import org.opensearch.core.action.ActionListener; -import org.opensearch.sql.datasources.model.transport.PatchDataSourceActionRequest; -import org.opensearch.sql.datasources.model.transport.PatchDataSourceActionResponse; -import org.opensearch.sql.datasources.service.DataSourceServiceImpl; -import org.opensearch.tasks.Task; -import org.opensearch.transport.TransportService; - -@ExtendWith(MockitoExtension.class) -public class TransportPatchDataSourceActionTest { - - @Mock private TransportService transportService; - @Mock private TransportPatchDataSourceAction action; - @Mock private DataSourceServiceImpl dataSourceService; - @Mock private Task task; - @Mock private ActionListener actionListener; - - @Captor - private ArgumentCaptor patchDataSourceActionResponseArgumentCaptor; - - @Captor private ArgumentCaptor exceptionArgumentCaptor; - - @BeforeEach - public void setUp() { - action = - new TransportPatchDataSourceAction( - transportService, new ActionFilters(new HashSet<>()), dataSourceService); - } - - @Test - public void testDoExecute() { - Map dataSourceData = new HashMap<>(); - dataSourceData.put(NAME_FIELD, "test_datasource"); - dataSourceData.put(DESCRIPTION_FIELD, "test"); - - PatchDataSourceActionRequest request = new PatchDataSourceActionRequest(dataSourceData); - - action.doExecute(task, request, actionListener); - verify(dataSourceService, times(1)).patchDataSource(dataSourceData); - Mockito.verify(actionListener) - .onResponse(patchDataSourceActionResponseArgumentCaptor.capture()); - PatchDataSourceActionResponse patchDataSourceActionResponse = - patchDataSourceActionResponseArgumentCaptor.getValue(); - String responseAsJson = "\"Updated DataSource with name test_datasource\""; - Assertions.assertEquals(responseAsJson, patchDataSourceActionResponse.getResult()); - } - - @Test - public void testDoExecuteWithException() { - Map dataSourceData = new HashMap<>(); - dataSourceData.put(NAME_FIELD, "test_datasource"); - dataSourceData.put(DESCRIPTION_FIELD, "test"); - doThrow(new RuntimeException("Error")).when(dataSourceService).patchDataSource(dataSourceData); - PatchDataSourceActionRequest request = new PatchDataSourceActionRequest(dataSourceData); - action.doExecute(task, request, actionListener); - verify(dataSourceService, times(1)).patchDataSource(dataSourceData); - Mockito.verify(actionListener).onFailure(exceptionArgumentCaptor.capture()); - Exception exception = exceptionArgumentCaptor.getValue(); - Assertions.assertTrue(exception instanceof RuntimeException); - Assertions.assertEquals("Error", exception.getMessage()); - } -} diff --git a/datasources/src/test/java/org/opensearch/sql/datasources/utils/XContentParserUtilsTest.java b/datasources/src/test/java/org/opensearch/sql/datasources/utils/XContentParserUtilsTest.java index e1e442d12b..d134293456 100644 --- a/datasources/src/test/java/org/opensearch/sql/datasources/utils/XContentParserUtilsTest.java +++ b/datasources/src/test/java/org/opensearch/sql/datasources/utils/XContentParserUtilsTest.java @@ -1,7 +1,6 @@ package org.opensearch.sql.datasources.utils; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.*; import com.google.gson.Gson; import java.util.HashMap; @@ -53,46 +52,6 @@ public void testToDataSourceMetadataFromJson() { Assertions.assertEquals("prometheus_access", retrievedMetadata.getAllowedRoles().get(0)); } - @SneakyThrows - @Test - public void testToMapFromJson() { - Map dataSourceData = - Map.of( - NAME_FIELD, - "test_DS", - DESCRIPTION_FIELD, - "test", - ALLOWED_ROLES_FIELD, - List.of("all_access"), - PROPERTIES_FIELD, - Map.of("prometheus.uri", "localhost:9090"), - CONNECTOR_FIELD, - "PROMETHEUS", - RESULT_INDEX_FIELD, - ""); - - Map dataSourceDataConnectorRemoved = - Map.of( - NAME_FIELD, - "test_DS", - DESCRIPTION_FIELD, - "test", - ALLOWED_ROLES_FIELD, - List.of("all_access"), - PROPERTIES_FIELD, - Map.of("prometheus.uri", "localhost:9090"), - RESULT_INDEX_FIELD, - ""); - - Gson gson = new Gson(); - String json = gson.toJson(dataSourceData); - - Map parsedData = XContentParserUtils.toMap(json); - - Assertions.assertEquals(parsedData, dataSourceDataConnectorRemoved); - Assertions.assertEquals("test", parsedData.get(DESCRIPTION_FIELD)); - } - @SneakyThrows @Test public void testToDataSourceMetadataFromJsonWithoutName() { @@ -112,22 +71,6 @@ public void testToDataSourceMetadataFromJsonWithoutName() { Assertions.assertEquals("name and connector are required fields.", exception.getMessage()); } - @SneakyThrows - @Test - public void testToMapFromJsonWithoutName() { - Map dataSourceData = new HashMap<>(Map.of(DESCRIPTION_FIELD, "test")); - Gson gson = new Gson(); - String json = gson.toJson(dataSourceData); - - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> { - XContentParserUtils.toMap(json); - }); - Assertions.assertEquals("Name is a required field.", exception.getMessage()); - } - @SneakyThrows @Test public void testToDataSourceMetadataFromJsonWithoutConnector() { @@ -163,21 +106,4 @@ public void testToDataSourceMetadataFromJsonUsingUnknownObject() { }); Assertions.assertEquals("Unknown field: test", exception.getMessage()); } - - @SneakyThrows - @Test - public void testToMapFromJsonUsingUnknownObject() { - HashMap hashMap = new HashMap<>(); - hashMap.put("test", "test"); - Gson gson = new Gson(); - String json = gson.toJson(hashMap); - - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> { - XContentParserUtils.toMap(json); - }); - Assertions.assertEquals("Unknown field: test", exception.getMessage()); - } } diff --git a/docs/user/ppl/admin/datasources.rst b/docs/user/ppl/admin/datasources.rst index 31378f6cc4..3682153b9d 100644 --- a/docs/user/ppl/admin/datasources.rst +++ b/docs/user/ppl/admin/datasources.rst @@ -93,19 +93,6 @@ we can remove authorization and other details in case of security disabled domai "allowedRoles" : ["prometheus_access"] } -* Datasource modification PATCH API ("_plugins/_query/_datasources") :: - - PATCH https://localhost:9200/_plugins/_query/_datasources - content-type: application/json - Authorization: Basic {{username}} {{password}} - - { - "name" : "my_prometheus", - "allowedRoles" : ["all_access"] - } - - **Name is required and must exist. Connector cannot be modified and will be ignored.** - * Datasource Read GET API("_plugins/_query/_datasources/{{dataSourceName}}" :: GET https://localhost:9200/_plugins/_query/_datasources/my_prometheus @@ -127,7 +114,6 @@ Each of the datasource configuration management apis are controlled by following * cluster:admin/opensearch/datasources/create [Create POST API] * cluster:admin/opensearch/datasources/read [Get GET API] * cluster:admin/opensearch/datasources/update [Update PUT API] -* cluster:admin/opensearch/datasources/patch [Update PATCH API] * cluster:admin/opensearch/datasources/delete [Delete DELETE API] Only users mapped with roles having above actions are authorized to execute datasource management apis. diff --git a/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java b/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java index ff36d2a887..8623b9fa6f 100644 --- a/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java @@ -5,8 +5,6 @@ package org.opensearch.sql.datasource; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.DESCRIPTION_FIELD; -import static org.opensearch.sql.datasources.utils.XContentParserUtils.NAME_FIELD; import static org.opensearch.sql.legacy.TestUtils.getResponseBody; import com.google.common.collect.ImmutableList; @@ -17,9 +15,7 @@ import java.io.IOException; import java.lang.reflect.Type; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; import lombok.SneakyThrows; import org.junit.AfterClass; import org.junit.Assert; @@ -140,31 +136,6 @@ public void updateDataSourceAPITest() { Assert.assertEquals( "https://randomtest.com:9090", dataSourceMetadata.getProperties().get("prometheus.uri")); Assert.assertEquals("", dataSourceMetadata.getDescription()); - - // patch datasource - Map updateDS = - new HashMap<>(Map.of(NAME_FIELD, "update_prometheus", DESCRIPTION_FIELD, "test")); - Request patchRequest = getPatchDataSourceRequest(updateDS); - Response patchResponse = client().performRequest(patchRequest); - Assert.assertEquals(200, patchResponse.getStatusLine().getStatusCode()); - String patchResponseString = getResponseBody(patchResponse); - Assert.assertEquals("\"Updated DataSource with name update_prometheus\"", patchResponseString); - - // Datasource is not immediately updated. so introducing a sleep of 2s. - Thread.sleep(2000); - - // get datasource to validate the modification. - // get datasource - Request getRequestAfterPatch = getFetchDataSourceRequest("update_prometheus"); - Response getResponseAfterPatch = client().performRequest(getRequestAfterPatch); - Assert.assertEquals(200, getResponseAfterPatch.getStatusLine().getStatusCode()); - String getResponseStringAfterPatch = getResponseBody(getResponseAfterPatch); - DataSourceMetadata dataSourceMetadataAfterPatch = - new Gson().fromJson(getResponseStringAfterPatch, DataSourceMetadata.class); - Assert.assertEquals( - "https://randomtest.com:9090", - dataSourceMetadataAfterPatch.getProperties().get("prometheus.uri")); - Assert.assertEquals("test", dataSourceMetadataAfterPatch.getDescription()); } @SneakyThrows diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index 303654ea37..e0d04c55b8 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -49,7 +49,6 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.Locale; -import java.util.Map; import javax.management.MBeanServerInvocationHandler; import javax.management.ObjectName; import javax.management.remote.JMXConnector; @@ -495,15 +494,6 @@ protected static Request getUpdateDataSourceRequest(DataSourceMetadata dataSourc return request; } - protected static Request getPatchDataSourceRequest(Map dataSourceData) { - Request request = new Request("PATCH", "/_plugins/_query/_datasources"); - request.setJsonEntity(new Gson().toJson(dataSourceData)); - RequestOptions.Builder restOptionsBuilder = RequestOptions.DEFAULT.toBuilder(); - restOptionsBuilder.addHeader("Content-Type", "application/json"); - request.setOptions(restOptionsBuilder); - return request; - } - protected static Request getFetchDataSourceRequest(String name) { Request request = new Request("GET", "/_plugins/_query/_datasources" + "/" + name); if (StringUtils.isEmpty(name)) { diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index eb6eabf988..a9a35f6318 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -59,12 +59,18 @@ import org.opensearch.sql.datasources.auth.DataSourceUserAuthorizationHelperImpl; import org.opensearch.sql.datasources.encryptor.EncryptorImpl; import org.opensearch.sql.datasources.glue.GlueDataSourceFactory; -import org.opensearch.sql.datasources.model.transport.*; +import org.opensearch.sql.datasources.model.transport.CreateDataSourceActionResponse; +import org.opensearch.sql.datasources.model.transport.DeleteDataSourceActionResponse; +import org.opensearch.sql.datasources.model.transport.GetDataSourceActionResponse; +import org.opensearch.sql.datasources.model.transport.UpdateDataSourceActionResponse; import org.opensearch.sql.datasources.rest.RestDataSourceQueryAction; import org.opensearch.sql.datasources.service.DataSourceMetadataStorage; import org.opensearch.sql.datasources.service.DataSourceServiceImpl; import org.opensearch.sql.datasources.storage.OpenSearchDataSourceMetadataStorage; -import org.opensearch.sql.datasources.transport.*; +import org.opensearch.sql.datasources.transport.TransportCreateDataSourceAction; +import org.opensearch.sql.datasources.transport.TransportDeleteDataSourceAction; +import org.opensearch.sql.datasources.transport.TransportGetDataSourceAction; +import org.opensearch.sql.datasources.transport.TransportUpdateDataSourceAction; import org.opensearch.sql.legacy.esdomain.LocalClusterState; import org.opensearch.sql.legacy.executor.AsyncRestExecutor; import org.opensearch.sql.legacy.metrics.Metrics; @@ -177,10 +183,6 @@ public List getRestHandlers( new ActionType<>( TransportUpdateDataSourceAction.NAME, UpdateDataSourceActionResponse::new), TransportUpdateDataSourceAction.class), - new ActionHandler<>( - new ActionType<>( - TransportPatchDataSourceAction.NAME, PatchDataSourceActionResponse::new), - TransportPatchDataSourceAction.class), new ActionHandler<>( new ActionType<>( TransportDeleteDataSourceAction.NAME, DeleteDataSourceActionResponse::new),