-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Adding rest actions for getting and updating data stream mappings #130241
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
Adding rest actions for getting and updating data stream mappings #130241
Conversation
Pinging @elastic/es-data-management (Team:Data Management) |
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.
Pull Request Overview
This PR adds REST endpoints and support for retrieving and updating data stream mappings in Elasticsearch, gated behind the logs_stream
feature flag.
- Introduces
GetDataStreamMappingsAction
andUpdateDataStreamMappingsAction
server actions and their REST handlers - Adds corresponding REST API specifications (
indices.get_data_stream_mappings.json
,indices.put_data_stream_mappings.json
) - Implements YAML integration tests and registers handlers in the plugin under the feature flag
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
server/src/main/java/org/elasticsearch/action/datastreams/GetDataStreamMappingsAction.java | Fixes mapping conversion to always use JSON |
rest-api-spec/src/main/resources/rest-api-spec/api/indices.put_data_stream_mappings.json | Adds PUT API spec for updating data stream mappings |
rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_data_stream_mappings.json | Adds GET API spec for retrieving data stream mappings |
modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/250_data_stream_mappings.yml | New YAML tests covering get/put data stream mappings |
modules/data-streams/src/yamlRestTest/java/org/elasticsearch/datastreams/DataStreamsClientYamlTestSuiteIT.java | Enables logs_stream feature flag in test cluster setup |
modules/data-streams/src/main/java/org/elasticsearch/datastreams/rest/RestUpdateDataStreamMappingsAction.java | Implements REST handler for updating mappings |
modules/data-streams/src/main/java/org/elasticsearch/datastreams/rest/RestGetDataStreamMappingsAction.java | Implements REST handler for retrieving mappings |
modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamsPlugin.java | Registers new REST handlers under the logs_stream flag |
Comments suppressed due to low confidence (3)
modules/data-streams/src/main/java/org/elasticsearch/datastreams/rest/RestGetDataStreamMappingsAction.java:32
- There's a typo in the handler name. It should return "get_data_stream_mappings_action" to match naming conventions and avoid confusion.
return "gett_data_stream_mappings_action";
rest-api-spec/src/main/resources/rest-api-spec/api/indices.put_data_stream_mappings.json:44
- [nitpick] The
body
section currently only has a description; adding a schema (e.g., expectedproperties
structure) would improve API spec clarity for consumers.
"body":{
server/src/main/java/org/elasticsearch/action/datastreams/GetDataStreamMappingsAction.java:132
- [nitpick] Hardcoding JSON here may lead to mismatches if the response builder uses a different content type (e.g., YAML or SMILE). Consider reverting to
builder.contentType()
or documenting why JSON-only is intended.
XContentType.JSON
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.
One small typo and a question more than a change request so overall LGTM 👍
public class RestGetDataStreamMappingsAction extends BaseRestHandler { | ||
@Override | ||
public String getName() { | ||
return "gett_data_stream_mappings_action"; |
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.
Typo?
return "gett_data_stream_mappings_action"; | |
return "get_data_stream_mappings_action"; |
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.
Oops!
try (XContentParser parser = request.contentParser()) { | ||
XContentParser.Token token = parser.nextToken(); | ||
if (token == XContentParser.Token.VALUE_STRING) { | ||
mappings = new CompressedXContent(Base64.getDecoder().decode(parser.text())); | ||
} else if (token == XContentParser.Token.VALUE_EMBEDDED_OBJECT) { | ||
mappings = new CompressedXContent(parser.binaryValue()); | ||
} else if (token == XContentParser.Token.START_OBJECT) { | ||
mappings = new CompressedXContent(Strings.toString(XContentFactory.jsonBuilder().map(parser.mapOrdered()))); | ||
} else { | ||
throw new IllegalArgumentException("Unexpected token: " + token); | ||
} | ||
} |
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.
This seems to be a pretty common pattern. Any reason a utility method hasn't been created for it?
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.
Yeah good point. I moved it into a method in Template.
This builds on #129787 and #130042 and by adding the REST actions to allow users to get and update data stream mappings.