Support for Version=0 in bulk_delete_request.go

garrison-stauffer commented 1 year ago

Which version of Elastic are you using?

[X] elastic.v7 (for Elasticsearch 7.x)
[ ] elastic.v6 (for Elasticsearch 6.x)
[ ] elastic.v5 (for Elasticsearch 5.x)
[ ] elastic.v3 (for Elasticsearch 2.x)
[ ] elastic.v2 (for Elasticsearch 1.x)

Please describe the expected behavior

When we are using the Bulk API to issue delete requests, we should be able to use a version_type=EXTERNAL_GTE with version=0

Please describe the actual behavior

When we use this in the Bulk API, the version is not being forwarded to ES and is having -3 as its default (the MATCH_ANY behavior). This leads to an error being returned by ElasticSearch:

elastic: Error 400 (Bad Request): Validation Failed: 1: illegal version value [-3] for version type [EXTERNAL_GTE]; [type=action_request_validation_exception]

When I curl elasticsearch directly, it is able to support Version=0:

$ curl https://my-elasticsearch-cluster.com/_bulk \
-H 'content-type: application/json' \
-X POST \
-d '{"delete": {"_index": "my-index", "_id": "my_id", "version": 0, "version_type": "external_gte"} }
'
{"took":13,"errors":false,"items":[{"delete":{"_index":"my-index","_type":"_doc","_id":"my_id","_version":0,"result":"deleted","_shards":{"total":4,"successful":3,"failed":0},"_seq_no":1,"_primary_term":3,"status":200}}]}

Any steps to reproduce the behavior?

The code is pretty messy, but we're basically just sending bulk requests to create + delete a record. Example code for the request:

var bulkService *elastic.BulkService // defined elsewhere

// Add a new document at Version=0 and VersionType=External
bulkService.Add(
    elastic.NewBulkIndexRequest().
        Index("some-index").
        Id("some-id").
        Doc(stuff).
        Version(0).
        VersionType("external").
        RetryOnConflict(0)
).Do(ctx)

// Now delete that document referencing Version=0 and VersionType=External_GTE
bulkService.Add(
        elastic.NewBulkDeleteRequest().
        Index("some-index").
        Id("some-id").
        Version(0).
        VersionType("external_gte")
).Do(ctx)

I noticed that this issue has been resolved for the Bulk Indexing requests, which is why we've been able to do the first half of the reproduction steps. I'm wondering if there's any opposition to me opening a PR that adds the same behavior for the bulk delete api?

garrison-stauffer commented 1 year ago

I understand this repo is deprecated, but if possible I'd like to add this behavior in the short term while we look towards upgrading to the official client in the future. If that isn't possible, then I'll start looking at the migration path in the short term.

Thanks in advance for the help!

garrison-stauffer commented 1 year ago

@olivere apologies for the direct tag; I've been running with this change set on a forked version for a little over a month now with no issues. We've looked towards upgrading to OpenSearch but the client support (at least in its current state) leaves.. quite a lot to be desired. For the forseeable future it looks like we will be staying on ElasticSearch 7 with olivere/elastic, and if possible I'd like to remove our forked changes.

Would you have any concerns if I open a PR with the changes?

olivere / elastic