6.x sets IsValid=false for 404 response

[keithpecka3]

NEST: 6.x >
Client: 6.x >

We are updating from 5.6 to 6.6 and have found that GetAsync sets IsValid=false when the document is not found. This is change from 5.6 where IsValid=true and Found=false. This is on .NET Core.

var response = await client.GetAsync (new DocumentPath<Person>("invalid"));

Response data:
IsValid: false
Found: false
DebugInformation: Invalid NEST response built from a unsuccessful low level call on GET: /people/person/invalid Audit trail of this API call: [1] BadResponse: Node: http://127.0.0.1:9200/ Took: 00:00:00.1815146 OriginalException: Elasticsearch.Net.ElasticsearchClientException: The remote server returned an error: (404) Not Found

I can't find any information stating this change. And can't see how the exception could be used to infer it's valid but not found without doing something like if(response.DebugInformation.Contains("(404) Not Found")) which is not ideal.

Please advise if this is expected behaviour. Thanks.

[ejsmith]

Yeah, I noticed this too and it seems wrong to me. 404 is a perfectly valid response for missing documents.

[russcam]

Thanks for opening @keithpecka3.

I think that the behaviour of this may have changed between 5.6.x and 6.x due to the way that responses are handled; We'll need to dig in further.

[Mpdreamz]

This was indeed changed between 5.x and 6.x one of the changes in 6.x is that in case of a bad response (as deemed by the low level client) it will still flow through the deserializer of NEST.

In 5.x if a bad response happens the low level client would shortcircuit and read the deserialized error from elasticsearch. This pattern worked until 6.x when various API's started returning more data on bad responses too. Also 5.5 introduces a changed whereby if some aliases are not found it would return a 404 but still return data for the aliases it did find.

in 5.x we signaled to the low level client 404 is not a bad response and in 6.x we rely on the fact we always deserialize and never bless a particular response code.

In 6.x you could use response.ApiCall.HttpStatusCode or exception.Response.HttpStatusCode to validate.

The rule for .Success in the low level client currently (7.x) is

				success = statusCode >= 200 && statusCode < 300
					|| requestData.Method == HttpMethod.HEAD && statusCode == 404
					|| allowedStatusCodes.Contains(statusCode.Value)
					|| allowedStatusCodes.Contains(-1);

I think we need to remove

					|| requestData.Method == HttpMethod.HEAD && statusCode == 404

And make the default for NEST IsValid return true if statusCode == 404 and no error is returned by Elasticsearch.

[Mpdreamz]

#3873 addresses this somewhat but won't change .IsValid, i think false is the correct response here having thought about it some more. .IsValid signals you can't proceed without further inspecting the response.

#3873 won't make 404 something that throws if you specify .ThrowExceptions() on .GetAsync() though

[codebrain]

Closing this ticket, hopefully @Mpdreamz has advised sufficiently as to the expected behaviour in .6.x.
The two PRs mentioned have been merged into 7.x and will be available on the next release.
Thanks @keithpecka3 - any problems please open another issue.