[Connector API] Support soft-deletes of connectors #118669

jedrazb · 2024-12-13T14:23:43Z

Soft deletes for connectors

Add support for soft-deletes of connectors. Why?

In order to improve UX on connectors on agentless we need to track what connectors were deleted vs. what connector records are not yet created

Changes

introduce deleted flag in the connector index mappings, indicates whether connector has been soft deleted
adapt delete, get and list operations logic to support this feature
- if we pass deleted=true flag we return also deleted connectors in the response
update docs
add yaml tests and unit tests

Related work

In a follow-up PR I will address adding index mapping migrations for connector index. I'm coordinating here with @navarone-feekery as he might be adding SystemIndexDecriptor for connector indices soon, this will help updating mappings a lot as there is SystemIndexMappingUpdateService
follow-up: support force deletes, e.g pass force=true to delete endpoint to completely remove the connector

github-actions · 2024-12-13T14:23:55Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-12-13T14:40:31Z

Hi @jedrazb, I've created a changelog YAML for you.

…ub.com:jedrazb/elasticsearch into support-soft-deletes-connectors-change-mapping

jedrazb · 2024-12-17T15:59:26Z

@elasticmachine merge upstream

elasticsearchmachine · 2024-12-17T16:02:47Z

Pinging @elastic/search-eng (Team:SearchOrg)

elasticsearchmachine · 2024-12-17T16:02:47Z

Pinging @elastic/search-extract-and-transform (Team:Search - Extract & Transform)

jedrazb · 2024-12-18T12:16:00Z

@elasticmachine merge upstream

navarone-feekery

Looks good! Just one comment about a possible test case.

Also, I'm curious about unique validations. Do we allow shared values across soft-deleted and live Connectors? E.g. can a new Connector have an index_name value identical to a soft-deleted Connector?

navarone-feekery · 2024-12-19T09:52:15Z

...rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/connector/20_connector_list.yml

@@ -279,4 +278,145 @@ setup:
      connector.list: { }


+---


I think this could benefit from a test case covering when a connector was soft deleted and we don't want to see soft-deleted connectors in the list response.

so like

Soft delete 1 connector

Fetch with deleted: false

Ensure there are 2 results

This exact scenario is covered here "List Connectors - Single soft deleted connector":

Be default the deleted flag is false when not defined

jedrazb · 2024-12-19T14:08:10Z

Also, I'm curious about unique validations. Do we allow shared values across soft-deleted and live Connectors?

Good point. By default, the logic stays the same, meaning deleted connectors remain invisible to any logic associated with existing connectors.

navarone-feekery

LGTM!

…n use

jedrazb · 2024-12-19T16:03:10Z

can a new Connector have an index_name value identical to a soft-deleted Connector

That was the single edge case I didn't cover because we have custom check (with query) for index names already in use ... fixed in 7525c99

timgrein

Left some comments, but I think nothing preventing this PR from being merged

timgrein · 2024-12-19T16:55:41Z

docs/reference/connector/apis/get-connector-api.asciidoc

@@ -27,6 +27,9 @@ To get started with Connector APIs, check out <<es-connectors-tutorial-api, our
 `<connector_id>`::
 (Required, string)

+`deleted`::
+(Optional, boolean) A flag indicating whether to also include connectors that have been soft-deleted. Defaults to `false`.


Suggested change

(Optional, boolean) A flag indicating whether to also include connectors that have been soft-deleted. Defaults to `false`.

(Optional, boolean) A flag indicating whether to also return connectors that have been soft-deleted. Defaults to `false`.

consistency nit

timgrein · 2024-12-19T16:56:20Z

rest-api-spec/src/main/resources/rest-api-spec/api/connector.list.json

+      "deleted": {
+        "type": "boolean",
+        "default": false,
+        "description": "A flag indicating whether to list connectors that have been soft-deleted."


Suggested change

"description": "A flag indicating whether to list connectors that have been soft-deleted."

"description": "A flag indicating whether to return connectors that have been soft-deleted."

consistency nit, feel free to ignore if it makes sense to use "list" here

timgrein · 2024-12-19T17:02:02Z

...earch/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java

@@ -1138,6 +1159,11 @@ private Map<String, Object> getConnectorConfigurationFromSearchResult(ConnectorS
        return (Map<String, Object>) searchResult.getResultMap().get(Connector.CONFIGURATION_FIELD.getPreferredName());
    }

+    private boolean getIsDeletedFromSearchResult(ConnectorSearchResult searchResult) {


Suggested change

private boolean getIsDeletedFromSearchResult(ConnectorSearchResult searchResult) {

private boolean isConnectorDeleted(ConnectorSearchResult searchResult) {

I was a bit confused by the name in the beginning. Would isConnectorDeleted make it more obvious?

timgrein · 2024-12-19T17:04:22Z

...earch/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java

@@ -212,6 +214,10 @@ public void getConnector(String connectorId, ActionListener<ConnectorSearchResul
                        .setResultMap(getResponse.getSourceAsMap())
                        .build();

+                    if (isDeleted == false && getIsDeletedFromSearchResult(connector)) {


Suggested change

if (isDeleted == false && getIsDeletedFromSearchResult(connector)) {

boolean didNotFindNonDeletedConnector = isDeleted == false && getIsDeletedFromSearchResult(connector)

if (didNotFindNonDeletedConnector) {

timgrein · 2024-12-19T17:06:03Z

...earch/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java

@@ -1156,8 +1182,8 @@ private static ConnectorSearchResult hitToConnector(SearchHit searchHit) {
    }

    /**
-     * This method determines if any documents in the connector index have the same index name as the one specified,
-     * excluding the document with the given _id if it is provided.
+     * This method determines if any records in the connector index have the same index name as the one specified,


Should it stay documents here, if the next line uses docs? Or should both be records?

timgrein · 2024-12-19T17:08:28Z

...plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/Connector.java

@@ -469,6 +479,7 @@ public void writeTo(StreamOutput out) throws IOException {
        out.writeEnum(status);
        out.writeGenericValue(syncCursor);
        out.writeBoolean(syncNow);
+        out.writeBoolean(isDeleted);


Curious: do we need a new transport version, if we add something at the end of an entity, which is serialized over the wire? Does a potentially old node simply discard everything after syncNow? Never tested that (I assume it works without a transport version check, but just to double-check)

Should ConnectorTests extend AbstractWireSerializingTestCase btw as it can be serialized over the wire?

jedrazb and others added 12 commits December 9, 2024 17:02

[Connector API] Add interface for soft-deletes

93ac07c

Define connector deleted system index

12567b2

Got soft-delete logic working

4fdc2ca

Add unit tests

d9be216

Add yaml e2e test and attempt to update permissions

183ed7a

Fix permissions

397032b

Update docs

d8523e0

Merge branch 'main' into support-soft-deletes-to-connectors

2640383

Fix docs

6e1197c

Update docs/changelog/118282.yaml

3d77bb2

Change logic

bf98ea8

Fix tests

b4eb411

jedrazb added :SearchOrg/Extract&Transform Label for the Search E&T team Team:Search - Extract & Transform v9.0.0 labels Dec 13, 2024

jedrazb added 2 commits December 13, 2024 15:24

Remove unnecessary privilege from yaml rest test

1b0845f

Update changelog

b51848e

jedrazb added the >feature label Dec 13, 2024

Update docs/changelog/118669.yaml

b206844

Merge branch 'main' into support-soft-deletes-connectors-change-mapping

3c0c5eb

jedrazb mentioned this pull request Dec 13, 2024

[Connector API] Support soft deletes of connectors #118282

Closed

jedrazb added 3 commits December 16, 2024 10:26

Merge branch 'main' into support-soft-deletes-connectors-change-mapping

5e02200

Adapt yaml tests

6ff5a8d

Merge branch 'support-soft-deletes-connectors-change-mapping' of gith…

8557414

…ub.com:jedrazb/elasticsearch into support-soft-deletes-connectors-change-mapping

elasticmachine and others added 2 commits December 17, 2024 15:59

Merge branch 'main' into support-soft-deletes-connectors-change-mapping

471643f

Undo changes to muted-tests.yml

e697134

jedrazb marked this pull request as ready for review December 17, 2024 16:02

elasticsearchmachine added the Team:SearchOrg Meta label for the Search Org (Enterprise Search) label Dec 17, 2024

elasticmachine and others added 2 commits December 18, 2024 12:16

Merge branch 'main' into support-soft-deletes-connectors-change-mapping

51ab020

Fix compilation issue after other PR got merged

4eb2a88

navarone-feekery reviewed Dec 19, 2024

View reviewed changes

navarone-feekery approved these changes Dec 19, 2024

View reviewed changes

Exclude soft-deleted connector from checks about index_name already i…

7525c99

…n use

timgrein reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connector API] Support soft-deletes of connectors #118669

[Connector API] Support soft-deletes of connectors #118669

jedrazb commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024

elasticsearchmachine commented Dec 13, 2024

jedrazb commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

jedrazb commented Dec 18, 2024

navarone-feekery left a comment

navarone-feekery Dec 19, 2024

jedrazb Dec 19, 2024 •

edited

Loading

jedrazb commented Dec 19, 2024

navarone-feekery left a comment

jedrazb commented Dec 19, 2024

timgrein left a comment

timgrein Dec 19, 2024

timgrein Dec 19, 2024

timgrein Dec 19, 2024

timgrein Dec 19, 2024

timgrein Dec 19, 2024

timgrein Dec 19, 2024

timgrein Dec 19, 2024

	(Optional, boolean) A flag indicating whether to also include connectors that have been soft-deleted. Defaults to `false`.
	(Optional, boolean) A flag indicating whether to also return connectors that have been soft-deleted. Defaults to `false`.

	"description": "A flag indicating whether to list connectors that have been soft-deleted."
	"description": "A flag indicating whether to return connectors that have been soft-deleted."

	private boolean getIsDeletedFromSearchResult(ConnectorSearchResult searchResult) {
	private boolean isConnectorDeleted(ConnectorSearchResult searchResult) {

-                    if (isDeleted == false && getIsDeletedFromSearchResult(connector)) {
+                   boolean didNotFindNonDeletedConnector = isDeleted == false && getIsDeletedFromSearchResult(connector)
+                    if (didNotFindNonDeletedConnector) {

[Connector API] Support soft-deletes of connectors #118669

Are you sure you want to change the base?

[Connector API] Support soft-deletes of connectors #118669

Conversation

jedrazb commented Dec 13, 2024 • edited Loading

Soft deletes for connectors

Add support for soft-deletes of connectors. Why?

Changes

Related work

github-actions bot commented Dec 13, 2024

elasticsearchmachine commented Dec 13, 2024

jedrazb commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

jedrazb commented Dec 18, 2024

navarone-feekery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedrazb Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

jedrazb commented Dec 19, 2024

navarone-feekery left a comment

Choose a reason for hiding this comment

jedrazb commented Dec 19, 2024

timgrein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedrazb commented Dec 13, 2024 •

edited

Loading

jedrazb Dec 19, 2024 •

edited

Loading