-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Fix saved objects client upsert functionality #114929
Fix saved objects client upsert functionality #114929
Conversation
Reduce from 2 preflight checks to 1 preflight check. Also combined two internal functions into a single one.
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.
Author's notes for reviewers
/** What namespace(s) the object should exist in, if it needs to be created; practically speaking this will never be undefined */ | ||
savedObjectNamespaces: string[] | undefined; |
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.
practically speaking this will never be undefined
What I mean by this is:
If the object's namespaces
field was undefined, then it would not have been found in the first place. But the only way this would be undefined is if the object was found and its source did not include a namespaces
field.
At any rate, I thought it would be better not to try to change existing types even though that is the behavior we expect.
* @returns Array of namespaces that this saved object currently includes, or (if the object does not exist yet) the namespaces that a | ||
* newly-created object will include. Value may be undefined if an existing saved object has no namespaces attribute; this should not | ||
* happen in normal operations, but it is possible if the Elasticsearch document is manually modified. | ||
* @throws Will throw an error if the saved object exists and it does not include the target namespace. | ||
*/ | ||
private async preflightGetNamespaces( |
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.
preflightGetNamespaces
and preflightCheckIncludesNamespace
were very similar, so I combined them into a single preflightCheckNamespaces
function.
Testing process: I pulled down #114620 and merged this branch on top. Then I ran the functional tests for SOC.update. The tests passed 🎉
|
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.
Thanks for fixing this. Got a few remarks and questions before definitive approval:
errorType: 'conflict', | ||
}); | ||
savedObjectNamespaces = preflightResult.savedObjectNamespaces; |
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.
incrementCounterInternal
also relies on upsert
:
upsert: raw._source, |
I think we need to add the same !preflightResult?.found
condition you did in update
?
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.
No, that was only necessary because update
will use the version properties of the existing object (for optimistic concurrency control). If you make an ES update call with the version properties and the upsert
field in your request, ES responds with an error.
incrementCounterInternal
doesn't need to include the version properties at all because it only updates object attributes using a script.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Thanks for making the checkResult
changes. I agree this is more verbose for the callers, but just re-reviewing the code, it felt way easier to understand the logic this way.
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Resolves #114918.
Reduce from 2 preflight checks to 1 preflight check.
Also combined two internal functions into a single one.
As far as I can tell, no consumers of multi-namespace objects are actually utilizing upsert yet (the only multi-namespace object at the moment is ML jobs), so I'm just backporting this to 7.16 and leaving it without a release note.