opensearch-project / opensearch-go

Go Client for OpenSearch
https://opensearch.org/docs/latest/clients/go/
Apache License 2.0
197 stars 102 forks source link

Improves ParseError response when server response is an unknown json #592

Closed imvtsl closed 2 months ago

imvtsl commented 2 months ago

Description

In ParseError, when json response from server is unknown, it used to return a string response (*fmt.wrapError). This is now changed to return a StringError struct (*opensearch.StringError).

Issues Resolved

Closes #582

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.99%. Comparing base (06a6dc8) to head (f8cfc01). Report is 53 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #592 +/- ## =========================================== + Coverage 57.29% 67.99% +10.70% =========================================== Files 315 376 +61 Lines 9823 8862 -961 =========================================== + Hits 5628 6026 +398 + Misses 2902 1555 -1347 + Partials 1293 1281 -12 ``` | [Flag](https://app.codecov.io/gh/opensearch-project/opensearch-go/pull/592/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) | Coverage Δ | | |---|---|---| | [integration](https://app.codecov.io/gh/opensearch-project/opensearch-go/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) | `60.35% <0.00%> (+9.51%)` | :arrow_up: | | [unit](https://app.codecov.io/gh/opensearch-project/opensearch-go/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) | `15.37% <100.00%> (+2.53%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/opensearch-project/opensearch-go/pull/592?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) | Coverage Δ | | |---|---|---| | [error.go](https://app.codecov.io/gh/opensearch-project/opensearch-go/pull/592?src=pr&el=tree&filepath=error.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-ZXJyb3IuZ28=) | `100.00% <100.00%> (ø)` | | ... and [273 files with indirect coverage changes](https://app.codecov.io/gh/opensearch-project/opensearch-go/pull/592/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project)
imvtsl commented 2 months ago

Is this a breaking change for users or can we make this backwards compatible? If yes, it's likely a major version bump that would need to be done in the same PR, and I definitely think it needs an entry in https://github.com/opensearch-project/opensearch-go/blob/main/UPGRADING.md.

I think we need to document error handling somewhere in USER_GUIDE, including this behavior, maybe you want to start a guide on that or add to existing ones?

I am attending a triage meeting with opensearch indexing team on Monday to get more insights into error handling. We can work on a guide for error handling after that.

imvtsl commented 2 months ago

@dblock I attended the triage meeting with indexing team today. You can find the summary of the discussion here.

The fix with Opensearch will likely go in 3.0. Also, we don't have documentation for error responses. I created issues with documentation after discussion in triage meeting today.

For now, I will get started with USER_GUIDE with the changes in this PR and prepare for a release.

imvtsl commented 2 months ago

I added documentation in UPGRADING.md.

I read RELEASING.md. I believe release manager is authorized to perform the release process. Please let me know if any further action is required from my end for the release.

dblock commented 2 months ago

I didn't see an answer on whether there is any way to make this backwards compatible, so I presume not and I am good with this - @Jakob3xD could you also please take a look (merge if it's good with you)?

imvtsl commented 2 months ago

@dblock Apologies for the confusion. I did look into making it backward compatible but couldn't find a reasonable way for doing so.