Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

imvtsl
Copy link
Contributor

@imvtsl imvtsl commented Jul 20, 2024

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.

Copy link

codecov bot commented Jul 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@             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 Coverage Δ
integration 60.35% <0.00%> (+9.51%) ⬆️
unit 15.37% <100.00%> (+2.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
error.go 100.00% <100.00%> (ø)

... and 273 files with indirect coverage changes

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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?

CHANGELOG.md Outdated Show resolved Hide resolved
@imvtsl imvtsl marked this pull request as draft July 22, 2024 03:57
@imvtsl
Copy link
Contributor Author

imvtsl commented Jul 22, 2024

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
Copy link
Contributor Author

imvtsl commented Jul 22, 2024

@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
Copy link
Contributor Author

imvtsl commented Jul 23, 2024

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.

@imvtsl imvtsl requested a review from dblock July 23, 2024 08:48
@imvtsl imvtsl marked this pull request as ready for review July 23, 2024 08:49
@dblock
Copy link
Member

dblock commented Jul 23, 2024

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)?

@dblock dblock merged commit d3e1f10 into opensearch-project:main Jul 23, 2024
59 checks passed
@imvtsl
Copy link
Contributor Author

imvtsl commented Jul 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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