From ef052d0b3493c534e5b153cd95c19b03e314a2db Mon Sep 17 00:00:00 2001 From: Jedr Blaszyk Date: Mon, 22 Jan 2024 13:19:38 +0100 Subject: [PATCH] [Connector API] Make update configuration action non-additive --- .../335_connector_update_configuration.yml | 30 ++++---- .../connector/ConnectorIndexService.java | 42 +++++++---- .../connector/ConnectorIndexServiceTests.java | 70 ++++++++++++++++--- 3 files changed, 107 insertions(+), 35 deletions(-) diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/335_connector_update_configuration.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/335_connector_update_configuration.yml index df4a640a0495d..5a7ab14dc6386 100644 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/335_connector_update_configuration.yml +++ b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/335_connector_update_configuration.yml @@ -57,7 +57,7 @@ setup: connector_id: test-connector body: configuration: - some_field: + some_new_field: default_value: null depends_on: - field: some_field @@ -92,20 +92,22 @@ setup: connector.get: connector_id: test-connector - - match: { configuration.some_field.value: 456 } + - is_false: configuration.some_field # configuration.some_field doesn't exist + + - match: { configuration.some_new_field.value: 456 } - match: { status: configured } - - match: { configuration.some_field.validations.0.constraint: [123, 456, 789] } - - match: { configuration.some_field.validations.0.type: included_in } - - match: { configuration.some_field.validations.1.constraint: ["string 1", "string 2", "string 3"] } - - match: { configuration.some_field.validations.1.type: included_in } - - match: { configuration.some_field.validations.2.constraint: 0 } - - match: { configuration.some_field.validations.2.type: greater_than } - - match: { configuration.some_field.validations.3.constraint: 42 } - - match: { configuration.some_field.validations.3.type: less_than } - - match: { configuration.some_field.validations.4.constraint: int } - - match: { configuration.some_field.validations.4.type: list_type } - - match: { configuration.some_field.validations.5.constraint: "\\d+" } - - match: { configuration.some_field.validations.5.type: regex } + - match: { configuration.some_new_field.validations.0.constraint: [123, 456, 789] } + - match: { configuration.some_new_field.validations.0.type: included_in } + - match: { configuration.some_new_field.validations.1.constraint: ["string 1", "string 2", "string 3"] } + - match: { configuration.some_new_field.validations.1.type: included_in } + - match: { configuration.some_new_field.validations.2.constraint: 0 } + - match: { configuration.some_new_field.validations.2.type: greater_than } + - match: { configuration.some_new_field.validations.3.constraint: 42 } + - match: { configuration.some_new_field.validations.3.type: less_than } + - match: { configuration.some_new_field.validations.4.constraint: int } + - match: { configuration.some_new_field.validations.4.type: list_type } + - match: { configuration.some_new_field.validations.5.constraint: "\\d+" } + - match: { configuration.some_new_field.validations.5.type: regex } --- "Update Connector Configuration with null tooltip": diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java index 41451c76b90f8..86127b7680e23 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java @@ -26,6 +26,8 @@ import org.elasticsearch.client.internal.OriginSettingClient; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.query.MatchAllQueryBuilder; +import org.elasticsearch.script.Script; +import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.search.sort.SortOrder; @@ -45,6 +47,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.function.BiConsumer; @@ -267,6 +270,8 @@ public void onFailure(Exception e) { /** * Updates the {@link ConnectorConfiguration} property of a {@link Connector}. + * The update process is non-additive; it completely replaces all existing configuration fields with the new configuration mapping, + * thereby deleting any old configurations. * * @param request Request for updating connector configuration property. * @param listener Listener to respond to a successful response or an error. @@ -274,19 +279,32 @@ public void onFailure(Exception e) { public void updateConnectorConfiguration(UpdateConnectorConfigurationAction.Request request, ActionListener listener) { try { String connectorId = request.getConnectorId(); - final UpdateRequest updateRequest = new UpdateRequest(CONNECTOR_INDEX_NAME, connectorId).doc( - new IndexRequest(CONNECTOR_INDEX_NAME).opType(DocWriteRequest.OpType.INDEX) - .id(connectorId) - .setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE) - .source( - Map.of( - Connector.CONFIGURATION_FIELD.getPreferredName(), - request.getConfiguration(), - Connector.STATUS_FIELD.getPreferredName(), - ConnectorStatus.CONFIGURED.toString() - ) - ) + + String updateConfigurationScript = String.format( + Locale.ROOT, + """ + ctx._source.%s = params.%s; + ctx._source.%s = params.%s; + """, + Connector.CONFIGURATION_FIELD.getPreferredName(), + Connector.CONFIGURATION_FIELD.getPreferredName(), + Connector.STATUS_FIELD.getPreferredName(), + Connector.STATUS_FIELD.getPreferredName() + ); + Script script = new Script( + ScriptType.INLINE, + "painless", + updateConfigurationScript, + Map.of( + Connector.CONFIGURATION_FIELD.getPreferredName(), + request.getConfiguration(), + Connector.STATUS_FIELD.getPreferredName(), + ConnectorStatus.CONFIGURED.toString() + ) ); + final UpdateRequest updateRequest = new UpdateRequest(CONNECTOR_INDEX_NAME, connectorId).script(script) + .setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); + clientWithOrigin.update( updateRequest, new DelegatingIndexNotFoundActionListener<>(connectorId, listener, (l, updateResponse) -> { diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorIndexServiceTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorIndexServiceTests.java index 9a2e0a82895fc..4183b192ac78e 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorIndexServiceTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorIndexServiceTests.java @@ -12,7 +12,14 @@ import org.elasticsearch.action.DocWriteResponse; import org.elasticsearch.action.delete.DeleteResponse; import org.elasticsearch.action.update.UpdateResponse; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.plugins.Plugin; import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.script.MockScriptEngine; +import org.elasticsearch.script.MockScriptPlugin; +import org.elasticsearch.script.ScriptContext; +import org.elasticsearch.script.ScriptEngine; +import org.elasticsearch.script.UpdateScript; import org.elasticsearch.test.ESSingleNodeTestCase; import org.elasticsearch.xpack.application.connector.action.PostConnectorAction; import org.elasticsearch.xpack.application.connector.action.PutConnectorAction; @@ -27,11 +34,14 @@ import org.junit.Before; import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -49,6 +59,13 @@ public void setup() { this.connectorIndexService = new ConnectorIndexService(client()); } + @Override + protected Collection> getPlugins() { + List> plugins = new ArrayList<>(super.getPlugins()); + plugins.add(MockPainlessScriptEngine.TestPlugin.class); + return plugins; + } + public void testPutConnector() throws Exception { Connector connector = ConnectorTestUtils.getRandomConnector(); String connectorId = randomUUID(); @@ -90,21 +107,16 @@ public void testUpdateConnectorConfiguration() throws Exception { DocWriteResponse resp = buildRequestAndAwaitPutConnector(connectorId, connector); assertThat(resp.status(), anyOf(equalTo(RestStatus.CREATED), equalTo(RestStatus.OK))); - Map connectorConfiguration = connector.getConfiguration() - .entrySet() - .stream() - .collect(Collectors.toMap(Map.Entry::getKey, entry -> ConnectorTestUtils.getRandomConnectorConfigurationField())); - UpdateConnectorConfigurationAction.Request updateConfigurationRequest = new UpdateConnectorConfigurationAction.Request( connectorId, - connectorConfiguration + connector.getConfiguration() ); DocWriteResponse updateResponse = awaitUpdateConnectorConfiguration(updateConfigurationRequest); assertThat(updateResponse.status(), equalTo(RestStatus.OK)); - Connector indexedConnector = awaitGetConnector(connectorId); - assertThat(connectorConfiguration, equalTo(indexedConnector.getConfiguration())); - assertThat(indexedConnector.getStatus(), equalTo(ConnectorStatus.CONFIGURED)); + + // Configuration update is handled via painless script. ScriptEngine is mocked for unit tests. + // More comprehensive tests are defined in yamlRestTest. } public void testUpdateConnectorPipeline() throws Exception { @@ -608,4 +620,44 @@ public void onFailure(Exception e) { return resp.get(); } + /** + * Update configuration action is handled via painless script. This implementation mocks the painless script engine + * for unit tests. + */ + private static class MockPainlessScriptEngine extends MockScriptEngine { + + public static final String NAME = "painless"; + + public static class TestPlugin extends MockScriptPlugin { + @Override + public ScriptEngine getScriptEngine(Settings settings, Collection> contexts) { + return new ConnectorIndexServiceTests.MockPainlessScriptEngine(); + } + + @Override + protected Map, Object>> pluginScripts() { + return Collections.emptyMap(); + } + } + + @Override + public String getType() { + return NAME; + } + + @Override + public T compile(String name, String script, ScriptContext context, Map options) { + if (context.instanceClazz.equals(UpdateScript.class)) { + UpdateScript.Factory factory = (params, ctx) -> new UpdateScript(params, ctx) { + @Override + public void execute() { + + } + }; + return context.factoryClazz.cast(factory); + } + throw new IllegalArgumentException("mock painless does not know how to handle context [" + context.name + "]"); + } + } + }