opensearch-rs icon indicating copy to clipboard operation
opensearch-rs copied to clipboard

[BUG] BulkCreate operation should support optional _id

Open sharp-pixel opened this issue 1 year ago • 2 comments

What is the bug?

BulkCreate operation requires an id 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 a create 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 the id.

What is the expected behavior?

new should have a variant pub 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

sharp-pixel avatar Jan 29 '24 15:01 sharp-pixel

Thanks for the report @sharp-pixel, would you be interested in making a PR to add this optional constructor?

Xtansia avatar Jan 29 '24 22:01 Xtansia

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?

  1. new(source: B) + id(id: S)
  2. new(id: Option<S>, source: B)

Different Handling of Optional _id Field

  • 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 API
  • BulkUpdateOperation::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

camerondurham avatar Feb 29 '24 05:02 camerondurham