Repository navigation
fix(es): key existing documents on _id instead of _source.id - #4523
Merged
Jan-Kazlouski-elastic merged 2 commits intoOct 6, 2026
Merged
Jan-Kazlouski-elastic merged 2 commits into
Jan-Kazlouski-elastic merged 2 commits into
Conversation
|
💚 CLA has been signed |
yield_existing_documents_metadata preferred _source.id over _id, but every consumer in the sink matches incoming docs and issues deletes by _id. When the two differ (e.g. an ingest pipeline rewrote id), existing docs were never matched and stale docs were never deleted. Closes elastic#2776 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ShivamSharma43
force-pushed
the
fix-2776-use-es-id-for-existing-docs
branch
from
September 27, 2026 12:43
1f9baf1 to
24c26c4
Compare
This was referenced Sep 27, 2026
Contributor
|
buildkite test this |
Contributor
|
Thank you for your contribution @ShivamSharma43 |
Jan-Kazlouski-elastic
added
v8.19.24
v9.5.6
v9.4.9
and removed
v9.4.8
v9.5.5
v8.19.23
labels
Jan-Kazlouski-elastic
enabled auto-merge (squash)
October 6, 2026 16:06
Contributor
|
buildkite test this |
Jan-Kazlouski-elastic
added a commit
that referenced
this pull request
Oct 7, 2026
…4523) (#4573) Backports the following commits to 9.5: - fix(es): key existing documents on _id instead of _source.id (#4523) Co-authored-by: Shivam Sharma <192242476+ShivamSharma43@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Jan-Kazlouski-elastic <jan.kazlouski@elastic.co>
Jan-Kazlouski-elastic
added a commit
that referenced
this pull request
Oct 7, 2026
…4523) (#4574) Backports the following commits to 9.4: - fix(es): key existing documents on _id instead of _source.id (#4523) Co-authored-by: Shivam Sharma <192242476+ShivamSharma43@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Jan-Kazlouski-elastic <jan.kazlouski@elastic.co>
Jan-Kazlouski-elastic
added a commit
that referenced
this pull request
Oct 7, 2026
…4523) (#4572) Backports the following commits to 8.19: - fix(es): key existing documents on _id instead of _source.id (#4523) Co-authored-by: Shivam Sharma <192242476+ShivamSharma43@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Jan-Kazlouski-elastic <jan.kazlouski@elastic.co>
Jan-Kazlouski-elastic
pushed a commit
that referenced
this pull request
Oct 7, 2026
## Part of #2990 #2989 removed `_update_native_connector_authentication()`, the only place the framework read a connector secret, and set `native_connector_api_keys_enabled` to False. Several pieces it relied on were left behind with no remaining callers. This PR removes those: - `ESManagementClient.get_connector_secret()` - the `Connector.api_key_secret_id` property - `ApiKeyNotFoundError` in `sync_job_runner.py` - `Features.native_connector_api_keys_enabled()` and the `Features.NATIVE_CONNECTOR_API_KEYS` constant in `connectors_sdk` Their tests are removed with them, along with a leftover `native_connector_api_keys_enabled` mock in `test_sync_job_runner.py`. Nothing here changes what is written to Elasticsearch or what Kibana sees. The following are deliberately left in place because removing them would be observable outside the framework, and are for a separate PR if that is wanted: - the `native_connector_api_keys` entry in `BaseDataSource.features()`, which is written into the connector doc - `create_connector_secret()`, which `connectors connector create --native` still uses to store an API key and set `api_key_secret_id` on the connector doc - `api_key_secret_id` in `docs/CONNECTOR_PROTOCOL.md` `Features.native_connector_api_keys_enabled()` and `NATIVE_CONNECTOR_API_KEYS` are public in the published `connectors_sdk` package. Nothing in this repo uses them, but external code calling them would break. Tests: `make lint` and `make test` pass in both `app/connectors_service` (2504 passed, 92.01% coverage) and `libs/connectors_sdk` (343 passed, 84.21% coverage). ## Checklists #### Pre-Review Checklist - [x] this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check `config.yml.example`) - [x] this PR has a meaningful title - [x] this PR links to all relevant github issues that it fixes or partially addresses - [x] Covered the changes with automated tests - [ ] Tested the changes locally (ran `make lint` and `make test`; not tested against a live cluster) ## Related Pull Requests - #2989, which removed the code path these were used by. - #4523 adds a test next to the ones removed here in `test_management_client.py`, so whichever merges second will need a small rebase. Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or 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
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.
Closes #2776
When the sink loads the documents already in an index,
yield_existing_documents_metadatakeyed each one on_source.idand only fell back to_idwhenidwas missing. Everything that consumes that map treats the key as the Elasticsearch_id:get_docsandget_access_control_docslook up each incoming doc by the_idthey are about to write, andenqueue_docs_to_deleteissues bulk deletes by_id.The connector itself always writes
idequal to_id, so the two only diverge when something else changesid, for example an ingest pipeline. When they do, no incoming doc matches an existing one: every doc is counted as a create,skip_unchanged_documentsnever skips anything, and the deletes at the end of a full sync target_idvalues that don't exist, so docs removed from the source are left in the index.This change keys the map on
_idand dropsidfrom the scan's_source, since it is no longer read. Theidfield the connector writes into documents is unchanged.This changes behaviour for indices already written where
iddiffers from_id. The first full or access control sync after this change will delete documents that were orphaned by the old behaviour, so document counts may drop. Existing docs will be counted as updates rather than creates, and unchanged docs will be skipped whenskip_unchanged_documentsis enabled. Indices whereidequals_id, or whereidis absent, are unaffected.Tests:
test_management_client.pygets a case where_source.iddiffers from_idand asserts the yielded keys are the_ids.test_sink.pygets a case that runsget_docsthrough the realyield_existing_documents_metadata(withasync_scanmocked) and asserts the matching doc is queued as an update and the stale doc is deleted by its_id. Both fail without the fix.Checklists
Pre-Review Checklist
config.yml.example)make lintandmake test; not tested against a live cluster)v7.13.2,v7.14.0,v8.0.0)Release Note
Fixes syncs failing to delete stale documents, and treating existing documents as new, when an ingest pipeline changes a document's
idfield so that it no longer matches its_id.Generated with Claude Code