[Connector API] Support updating configuration values only #105249
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jedrazb
added
>enhancement
:EnterpriseSearch/Application
|
Hi @jedrazb, I've created a changelog YAML for you. |
|
Pinging @elastic/ent-search-eng (Team:Enterprise Search) |
timgrein
approved these changes
Feb 8, 2024
...arch/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorConfiguration.java
Outdated
Show resolved
Hide resolved
...arch/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorConfiguration.java
Outdated
Show resolved
Hide resolved
...earch/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java
Outdated
Show resolved
Hide resolved
Comment on lines
+409
to
+430
| 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() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Is this needed, because we don't know the configuration value field names in advance? Looks a bit intimidating at a first glance 😁
There was a problem hiding this comment.
This is needed because currently the update ES method is additive, it will merge two dictionaries (configurations): existing with provided one. This is problematic when updating service type where you need to override the config completely, doing it via script means you end up with only new config values (the old one is discarded).
| * This method supports full configuration replacement or individual configuration value updates. | ||
| * If a full configuration is provided, it overwrites all existing configurations in non-additive way. | ||
| * If only configuration values are provided, existing configuration object is updated with new values | ||
| * provided in the request. | ||
| * | ||
| * @param request Request for updating connector configuration property. | ||
| * @param listener Listener to respond to a successful response or an error. | ||
| */ | ||
| public void updateConnectorConfiguration(UpdateConnectorConfigurationAction.Request request, ActionListener<UpdateResponse> listener) { |
There was a problem hiding this comment.
nit: The method body is quite long, maybe it's worth to refactor 2-3 blocks of logic out into private methods?
...org/elasticsearch/xpack/application/connector/action/UpdateConnectorConfigurationAction.java
Show resolved
Hide resolved
timgrein
reviewed
Feb 8, 2024
...c/yamlRestTest/resources/rest-api-spec/test/entsearch/335_connector_update_configuration.yml
Show resolved
Hide resolved
…/application/connector/ConnectorIndexService.java Co-authored-by: Tim Grein <tim@4greins.de>
…/application/connector/ConnectorConfiguration.java Co-authored-by: Tim Grein <tim@4greins.de>
…/application/connector/ConnectorConfiguration.java Co-authored-by: Tim Grein <tim@4greins.de>
martijnvg
pushed a commit
to martijnvg/elasticsearch
that referenced
this pull request
Feb 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
{"configuration": {... full config obj..}}as request payload to do itconfigurationin request payload overrides config fullyvaluesuses default ES additive update of an objectValidations
valuesandconfigurationcannot both be present and both be nullvalueskeys have to be subset of existing configuration schema. If we have connector with configuration with fields:field_a,field_b,field_c, then thevaluescannot contain mapping to unknown key, e.g.field_dbut can contain subset, e.g. only update value for fields:field_a,field_b. If this is violated we throw informative error message and a 400 status code e.g.:"Unknown [configuration] fields in the request payload: [xd]. Remove them from request or register their schema first."Testing