-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOR: Add attr override option for update
and bulkUpdate
#183267
Conversation
/ci |
/ci |
Pinging @elastic/kibana-core (Team:Core) |
* a "full" update instead, where the provided attributes will fully override the existing ones. | ||
* Defaults to `false`. | ||
*/ | ||
overrideAttributes?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, mergeAttributes: true
seems more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5857447
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]History
To update your PR or re-run it, just comment with: |
), | ||
typeMappings: typeDefinition.mappings, | ||
}); | ||
const mergeAttributes = options.mergeAttributes ?? true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT consistency: All other options are extracted on lines 87-93, and the optional ones have default values that are stored in ../constants.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one NIT (which can be addressed on a separate PR)
…183267) ## Summary Fix elastic#183112 Add a new `mergeAttributes` option for both `update` and `bulkUpdate`, which, when set to `false`, allows to perform a "full" update (fully replacing the current attributes of the document) instead of a "partial" one (merging the attributes). Technically, ES doesn't really support it, so it was only made possible due to the "client-side" update we implemented for ZDT / SO version BWC. (cherry picked from commit 61e408c)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…;bulkUpdate` (#183267) (#183476) # Backport This will backport the following commits from `main` to `8.14`: - [SOR: Add attr override option for `update` and `bulkUpdate` (#183267)](#183267) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Pierre Gayvallet","email":"pierre.gayvallet@elastic.co"},"sourceCommit":{"committedDate":"2024-05-15T09:02:32Z","message":"SOR: Add attr override option for `update` and `bulkUpdate` (#183267)\n\n## Summary\r\n\r\nFix #183112 a new `mergeAttributes` option for both `update` and `bulkUpdate`,\r\nwhich, when set to `false`, allows to perform a \"full\" update (fully\r\nreplacing the current attributes of the document) instead of a \"partial\"\r\none (merging the attributes).\r\n\r\nTechnically, ES doesn't really support it, so it was only made possible\r\ndue to the \"client-side\" update we implemented for ZDT / SO version BWC.","sha":"61e408c963c3bc0a26375ccb3007c0701a3240fa","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","Feature:Saved Objects","release_note:skip","v8.14.0","v8.15.0"],"title":"SOR: Add attr override option for `update` and `bulkUpdate`","number":183267,"url":"#183267: Add attr override option for `update` and `bulkUpdate` (#183267)\n\n## Summary\r\n\r\nFix #183112 a new `mergeAttributes` option for both `update` and `bulkUpdate`,\r\nwhich, when set to `false`, allows to perform a \"full\" update (fully\r\nreplacing the current attributes of the document) instead of a \"partial\"\r\none (merging the attributes).\r\n\r\nTechnically, ES doesn't really support it, so it was only made possible\r\ndue to the \"client-side\" update we implemented for ZDT / SO version BWC.","sha":"61e408c963c3bc0a26375ccb3007c0701a3240fa"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#183267: Add attr override option for `update` and `bulkUpdate` (#183267)\n\n## Summary\r\n\r\nFix #183112 a new `mergeAttributes` option for both `update` and `bulkUpdate`,\r\nwhich, when set to `false`, allows to perform a \"full\" update (fully\r\nreplacing the current attributes of the document) instead of a \"partial\"\r\none (merging the attributes).\r\n\r\nTechnically, ES doesn't really support it, so it was only made possible\r\ndue to the \"client-side\" update we implemented for ZDT / SO version BWC.","sha":"61e408c963c3bc0a26375ccb3007c0701a3240fa"}}]}] BACKPORT--> Co-authored-by: Pierre Gayvallet <pierre.gayvallet@elastic.co>
Summary
Fix #183112
Add a new
mergeAttributes
option for bothupdate
andbulkUpdate
, which, when set tofalse
, allows to perform a "full" update (fully replacing the current attributes of the document) instead of a "partial" one (merging the attributes).Technically, ES doesn't really support it, so it was only made possible due to the "client-side" update we implemented for ZDT / SO version BWC.