Open sharp-pixel opened 7 months ago
Thanks for the report @sharp-pixel, would you be interested in making a PR to add this optional constructor?
What are your thoughts on adjusting the existing API for consistency with how "optional" _id
fields are handled?
This would be a breaking change, but could potentially make the API more intuitive/consistent if id was handled consistently per the API.
Instead of making a new constructor, what do you think about having a consistent handling of optional API fields?
new(source: B)
+ id(id: S)
new(id: Option<S>, source: B)
BulkCreateOperation::new(id, source)
requires ID although it's optional in the BulkCreate: https://github.com/opensearch-project/opensearch-rs/blob/41417d4ee7892213f570b072211f3cb87f0c113a/opensearch/src/root/bulk.rs#L179 APIBulkUpdateOperation::new(id, source)
requires an ID because the BulkUpdate API requires it https://github.com/opensearch-project/opensearch-rs/blob/41417d4ee7892213f570b072211f3cb87f0c113a/opensearch/src/root/bulk.rs#L200 BulkIndexOperation::new(source)
ID not required since it's optional per BulkIndex API, crate offers the id()
to add it if desired https://github.com/opensearch-project/opensearch-rs/blob/41417d4ee7892213f570b072211f3cb87f0c113a/opensearch/src/root/bulk.rs#L184
What is the bug?
BulkCreate
operation requires anid
to be specified, when it is optional in the original documentation: https://www.elastic.co/guide/en/elasticsearch/reference/7.10/docs-bulk.html#bulk-api-request-body. Note that Data Streams require the use of acreate
operation and the_id
should be auto-generated by OpenSearch as a best practice for log analytics.How can one reproduce the bug?
Look at https://github.com/opensearch-project/opensearch-rs/blob/main/opensearch/src/root/bulk.rs#L228 where the
new
method requires theid
.What is the expected behavior?
new
should have a variantpub fn new<S>(source: B) -> Self
(or some other name as we cannot overload in Rust)What is your host/environment?
any
Do you have any screenshots?
N/A
Do you have any additional context?
no