[BUG] 429 error does not follow standard error format

[martingrzzler]

What is the bug?

opensearchapi.parseError does not handle 429 correctly and instead returns failed to json unmarshal body as the body does not follow the opensearchapi.Error format.

What is the expected behavior?

I ran into this by calling Bulk. A 429 Error is expected instead of the unmarshalling error.

// praseError tries to parse the opensearch api error into an custom error
func parseError(resp *opensearch.Response) error {
	if resp.Body == nil {
		return fmt.Errorf("%w, status: %s", ErrUnexpectedEmptyBody, resp.Status())
	}

	body, err := io.ReadAll(resp.Body)
	if err != nil {
		return fmt.Errorf("%w: %w", ErrReadBody, err)
	}

	if resp.StatusCode == http.StatusMethodNotAllowed {
		var apiError StringError
		if err = json.Unmarshal(body, &apiError); err != nil {
			return fmt.Errorf("%w: %w", ErrJSONUnmarshalBody, err)
		}
		return apiError
	}

	// ToDo: Parse 404 errors separate as they are not in one standard format
	// https://github.com/opensearch-project/OpenSearch/issues/9988
        // ToDo: Parse 429 errors separate as they are not in one standard format


	var apiError Error
	if err = json.Unmarshal(body, &apiError); err != nil {
                 // will be returned if status code is 429
		return fmt.Errorf("%w: %w", ErrJSONUnmarshalBody, err)
	}

	return apiError
}

[Jakob3xD]

@martingrzzler thanks for the report.
It is possible for you to test this with the main branch.
I did some changes to the error handling and it should now cover more cases. See error.go.
If this does not help, then please provide the 429 reponse body you get so we can adjust or add some error struct.

[martingrzzler]

@Jakob3xD

If I'm not mistaken the 429 Error will be mapped to ErrNonJSONError. I've tried the following test:

{
				Name: "Too many requests",
				Resp: &opensearch.Response{
					StatusCode: http.StatusTooManyRequests,
					Body:       io.NopCloser(strings.NewReader("429 Too Many Requests /testindex/_bulk")),
				},
				WantedErrors: []error{opensearch.ErrNonJSONError},
},

Perhaps a ErrTooManyRequests should be added to the sentinels?

	ErrUnauthorized           = errors.New(http.StatusText(http.StatusUnauthorized))
        ErrTooManyRequests    = errors.New(http.StatusText(http.TooManyRequests))

[dblock]

@martingrzzler Yes, it should. Want to contribute a test + fix?

[Jakob3xD]

Adding this for now is okay but maybe instead of doing:

	if !json.Valid(body) {
		if resp.StatusCode == http.StatusUnauthorized {
			return ErrUnauthorized
		}
		return ErrNonJSONError
	}

We can do:

	if !json.Valid(body) {
		return fmt.Errorf("%w, %s",ErrNonJSONError, body )
	}

Or even omit the ErrNonJSONError from the error. I am not sure how good this will scale if we have some more error codes that have a non json response on error.

[martingrzzler]

@Jakob3xD I've went with the approach you suggested without the ErrNonJSONError.