[BUG] opensearchapi.Client.Document.Delete bugged error handling (v4)

[yeswecanfixit]

What is the bug?

The function opensearchapi.Client.Document.Delete has bugged error handling. When it returns an error, for example when attempting to delete a non-existing document, the function rightly returns an error, but this error, instead of being of type *opensearch.StructError, is a *fmt.wrapError with message msg: "opensearch error response could not be parsed as error: {\"_index\":\"my-index\",\"_id\":\"my-id\",\"version\":4,\"result:\":\"not_found\", <ET CETERA . . . > }"
The "inner error" at least has the correct "result": "not_found", but this inner error has been wrapped with this parsing error.

How can one reproduce the bug?

This bug can be reproduced by calling opensearchapi.Client.Document.Delete function on any non-existing document ID.

What is the expected behavior?

The expectation is to receive an error that can be cast as *opensearch.StructError using errors.As(err, &myStructErr), or at the very least, the error should not be wrapped around this parsing error saying the error response could not be parsed as error.

What is your host/environment?

MacOS Sonoma 14.5

[dblock]

Thanks for reporting it. Want to try to write a (failing) unit test for this and maybe a fix?

[imvtsl]

Hi @yeswecanfixit
In case you don't want to work on the fix, I would be happy to pitch in and contribute to the fix. Please let us know if you are working on the fix.

[dblock]

@imvtsl Go for it!

[yeswecanfixit]

Hi @yeswecanfixit In case you don't want to work on the fix, I would be happy to pitch in and contribute to the fix. Please let us know if you are working on the fix.

Thank you! That would be much appreciated

[imvtsl]

@dblock @yeswecanfixit
I am now working on it. Thank you!!

[imvtsl]

I reproduced the issue here.

When there is error returned from the server, Go client in parseError, tries to parse the response body as testResp struct

This works well when there are failure while creating index, deleting index, creating document with existing id, etc. This is because all these responses are in same format. See all responses at the end.

However, in case of failure in delete document API, the response body received from the server is completely different. I believe the best practice is for error responses to be consistent across all APIs.

Therefore, in an ideal case we should get the error response for delete API changed at the server side. I will create an issue with OpenSearch and update issue number here.

Sample Responses

Index create failed

{
"error": {
"root_cause": [
{
"type": "resource_already_exists_exception",
"reason": "index [movies/Um9KkSRHQlyaVClh-O6GRw] already exists",
"index": "movies",
"index_uuid": "Um9KkSRHQlyaVClh-O6GRw"
}
],
"type": "resource_already_exists_exception",
"reason": "index [movies/Um9KkSRHQlyaVClh-O6GRw] already exists",
"index": "movies",
"index_uuid": "Um9KkSRHQlyaVClh-O6GRw"
},
"status": 400
}

Index delete failed

{
"error": {
"root_cause": [
{
"type": "index_not_found_exception",
"reason": "no such index [games]",
"index": "games",
"resource.id": "games",
"resource.type": "index_or_alias",
"index_uuid": "na"
}
],
"type": "index_not_found_exception",
"reason": "no such index [games]",
"index": "games",
"resource.id": "games",
"resource.type": "index_or_alias",
"index_uuid": "na"
},
"status": 404
}

Document create failed

{
"error": {
"root_cause": [
{
"type": "version_conflict_engine_exception",
"reason": "[2]: version conflict, document already exists (current version [1])",
"index": "movies",
"shard": "0",
"index_uuid": "Um9KkSRHQlyaVClh-O6GRw"
}
],
"type": "version_conflict_engine_exception",
"reason": "[2]: version conflict, document already exists (current version [1])",
"index": "movies",
"shard": "0",
"index_uuid": "Um9KkSRHQlyaVClh-O6GRw"
},
"status": 409
}

Document delete failed

{
"_index": "movies",
"_id": "3",
"_version": 1,
"result": "not_found",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 36,
"_primary_term": 2
}

[imvtsl]

Created this issue with Opensearch.

[Jakob3xD]

Created this issue with Opensearch.

Thank you for investigating this issue.
When re-writing the lib I already encountered this behavior several times. Therefore I opened opensearch-project/OpenSearch#9988 as a more general issue for 404 responses.

I am thinking if we maybe want to return the reponse body to the client as error if we can't parse the returned error into any struct. On the other hand we are returning the response even on error, so the client can access the reponse body already by calling Inspect().
Therefor the workaround would be to check if the response is not nil, inspect it and check the response body or status code.

Something like this:

resp, err := client.Document.Delete(ctx, <someRequest>)
if err != nil {
    if !errors.Is(err, opensearch.ErrJSONUnmarshalBody) || (errors.Is(err, opensearch.ErrJSONUnmarshalBody) && resp != nil && resp.Inspect().Response.StatusCode != 404) {
        return err
    }
}

[imvtsl]

Therefore I opened opensearch-project/OpenSearch#9988 as a more general issue for 404 responses.

Thanks for pointing this out. The discussion thread of this issue doesn't seem active. I am willing to pitch in and contribute to opensearch server to make error responses consistent for all APIs.

I am thinking if we maybe want to return the reponse body to the client as error if we can't parse the returned error into any struct. On the other hand we are returning the response even on error, so the client can access the reponse body already by calling Inspect(). Therefor the workaround would be to check if the response is not nil, inspect it and check the response body or status code.

Something like this:

resp, err := client.Document.Delete(ctx, <someRequest>)
if err != nil {
    if !errors.Is(err, opensearch.ErrJSONUnmarshalBody) || (errors.Is(err, opensearch.ErrJSONUnmarshalBody) && resp != nil && resp.Inspect().Response.StatusCode != 404) {
        return err
    }
}

This way the error will be of type *fmt.wrapError.

For the workaround, until the issue is fixed at Opensearch, I was thinking of parsing it into opensearch.StructError{} in api_document.delete():

if data.response, err = c.apiClient.do(ctx, req, &data); err != nil {
		// err type is *fmt.wrapError, so manually converting to type opensearch.StructError
		if data.response.IsError() {
			body, _ := io.ReadAll(data.response.Body)
			bodyString := string(body[:])
			errorStruct := &opensearch.StructError{
				Status: data.response.StatusCode,
				Err: opensearch.Err{
					Reason: bodyString,
				},
			}
			return &data, errorStruct
		}
		return &data, err
	}

The advantage here is that it is generic for all status code in delete API. The users of the library get one type of response (opensearch.StructError) for all failure in delete API. Although the users would still have to manually parse errorStruct.Err.Reason. Also, the client can access the entire response body in errorStruct.Err.Reason.

Thoughts?

[dblock]

Thanks for pointing this out. The discussion thread of this issue doesn't seem active. I am willing to pitch in and contribute to opensearch server to make error responses consistent for all APIs.

That would be much appreciated but note that for backwards compat it will only go into 3.0.

[Jakob3xD]

The advantage here is that it is generic for all status code in delete API.

I would not see this as an advantage as all other valid errors can't be parse normally. The users would need to do the error handling we are already doing with the ParseError() by them self and I don't see the benefit in it.

I would rather adjust the ParseError() function to return the generic StringError when it can't parse the response.

	if err = json.Unmarshal(body, &testResp); err != nil {
		return StringError{Status: resp.StatusCode, Err: string(body)}
	}

[imvtsl]

The advantage here is that it is generic for all status code in delete API.

I would not see this as an advantage as all other valid errors can't be parse normally. The users would need to do the error handling we are already doing with the ParseError() by them self and I don't see the benefit in it.

I am not sure if I understand what you are trying to convey here. I meant this would be generic only for all errors in delete API. Isn't getting an error struct (opensearch.StructError or StringError) better than getting a string (*fmt.wrapError) from the usability perspective for the client?

I would rather adjust the ParseError() function to return the generic StringError when it can't parse the response.

	if err = json.Unmarshal(body, &testResp); err != nil {
		return StringError{Status: resp.StatusCode, Err: string(body)}
	}

Anyways, this seems to be the best idea so far. It will work not only for delete API but for all APIs when it can't parse the response. I will raise a PR with this.

[imvtsl]

I raised this PR for the fix.