-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Ingest: Enforce _version metadata not null in sourceAndMetadata map #88102
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
Ingest: Enforce _version metadata not null in sourceAndMetadata map #88102
Conversation
The `_version` metadata field should always exist in the sourceAndMetadata map, this change enforces that invariant but allows tests to avoid it if necessary. Refs: elastic#87309
|
@elasticsearch run elasticsearch-ci/part-1 |
…gest_required-version-pr
…_required-version-pr
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/es-data-management (Team:Data Management) |
rjernst
left a comment
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.
A couple naming suggestions
| /** | ||
| * Create an {@link IngestDocument} with {@link #DEFAULT_VERSION} as the _version metadata, if _version is not already present. | ||
| */ | ||
| public static IngestDocument ofSourceAndMetadataWithDefaultVersion(Map<String, Object> sourceAndMetadata) { |
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.
If this is the common case, can we shorten the method name to the original name? These are extremely long...
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.
ofSourceAndMetadataWithDefaultVersion -> withDefaultVersion
ofSourceAndMetadataWithNullableVersion -> withNullableVersion
ofSourceAndIngestWithNullableVersion -> ofIngestWithNullableVersion
emptyIngestDocumentWithDefaultVersion -> emptyIngestDocument
| */ | ||
| public static IngestDocument emptyIngestDocument() { | ||
| return new IngestDocument(new HashMap<>(), new HashMap<>()); | ||
| public static IngestDocument emptyIngestDocumentWithDefaultVersion() { |
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.
I think it would be fine to leave the original name and just add to the javadoc that the default version is added.
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.
Kept original, added javadoc comment.
rjernst
left a comment
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.
Much better names, thanks! LGTM
|
The discussions of the BWC issues happened in this thread: #87309 (comment), leading to this change. |
…ersion (#88183) When `RandomDocumentPicks.randomExistingFieldName` selects a field, the caller expects to be able to manipulate that field in any manner. `_version` has special validation, must fit in an `long` and may not be `null`. This change no longer returns `_version` from `randomExistingFieldName` unless it is the only field in the document. Related to change: #88102
The
_versionmetadata field should always exist in the sourceAndMetadatamap, this change enforces that invariant but allows tests to avoid it
if necessary.
Refs: #87309