BulkResponse with IsValid = false does not throw exception with ThrowExceptions(true)

[JulianRooze]

NEST/Elasticsearch.Net version:

2.3.1:

Description of the problem including expected versus actual behavior:

NEST ConnectionSettings.ThrowExceptions(true) is documented as this:

//
// Summary:
//     Instead of following a c/go like error checking on response.IsValid always throw
//     an exception on the client when a call resulted in an exception on either the
//     client or the Elasticsearch server.
//     Reasons for such exceptions could be search parser errors, index missing exceptions,
//     etc...

However, no exception is raised for a BulkResponse that has IsValid = false (and Errors = true), which I would expect is the expected behavior according to the above documentation.

The HttpStatusCode from Elasticsearch (version 2.3.3) is however a 200 which is the cause of this behavior, as the overall Success from the response is true and because of that it doesn't enter the BadResponse code in Transport.cs.

As far as I can tell, every call to _bulk returns with a 200 OK from Elasticsearch, even if the request contains things like invalid or unparseable JSON.

I don't know whether this is by design or if this is a bug in Elasticsearch. It's possible that in a bulk insert, some items will succeed while others will fail and so there isn't necessarily one status code as a response. I would expect however Elastic to return the most pessimistic result as the response (500 > 40x > 20x), rather than a 200. If there is a technical reason why Elasticsearch must return a 200 always then it would be nice if NEST took this in consideration and threw an exception for invalid bulk responses if ThrowExceptions is set to true.

I removed all our tedious C/Go-like IsValid error checking in favor of that setting, but I have to add it again for bulk responses and potentially other operations (because I don't know what other operations behave this way now).

Steps to reproduce:

1. Create an index with dynamic mapping set to strict to not allow any dynamic fields.
2. Set the ThrowExceptions setting to true.
3. Do a bulk insert to the index with a field that is not part of the current mapping. You'll receive a BulkResponse with IsValid = false because an exception is raised on the server, but no exception will be thrown.

[gmarz]

I don't know whether this is by design or if this is a bug in Elasticsearch.

It's by design. The 200 indicates that the request itself was successful, even though there may be some individual failures.

response.IsValid is a NEST abstraction, and doesn't always map 1-to-1 with the response code from ES. There are some exceptions (for instance with _bulk, as you've discovered) where IsValid also checks additional conditions on the response.

response.ApiCall.Success on the other hand does map 1-to-1 with the ES response.

Sorry if I'm stating what you already know, but figured I should clarify this here for the next reader.

(because I don't know what other operations behave this way now).

We should definitely document all the instances where IsValid has extra logic baked in, but I'll list them all here now FWIW:

TasksCancel
TasksList
ReindexOnServer
UpdateByQuery
MultiSearch
MultiPercolate

With all that said, the actual issue/debate here is whether or not ThrowExceptions() should align with IsValid. Currently it doesn't, and instead aligns more with ApiCall.Success.

There is a bit of a technical challenge to get that to work though, as Tranport would need to be aware of what the IsValid condition on the response, as right now it's totally agnostic and only raises an exception based on the response code.

Thoughts @Mpdreamz @russcam ?

[Mpdreamz]

Discussed this today:

ThrowExceptions is really only for those cases where elasticsearch indicates an error through http status codes and returns a structured error response. All of this data is encapsulated in the existing exceptions.

Its expected behavior to inspect certain API's valid responses further e.g bulk's error property or making sure a search result was successful on all the shards. IsValid can help here. We should not throw however in these cases.

[nicholasyin]

At least this should be mentioned in the code comment.