Add Err() function to Response for detailed errors #246

Jakob3xD · 2023-03-03T13:34:44Z

Description

Add Err() function to Response and a new Error Type for a better and more detailed error handling.

Issues Resolved

Related #220

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.

dblock

I like this a lot! Let's add something to USER_GUIDE too?

opensearchapi/opensearchapi.response.go

opensearchapi/opensearchapi_response_internal_test.go

USER_GUIDE.md

dblock

I think we're close, but something feels wrong about having to now check for error twice. WDYT?

dblock · 2023-03-16T04:40:41Z

USER_GUIDE.md

 		os.Exit(1)
 	}
+	if err := searchResponse.Err(); err != nil {
+		fmt.Println(fmt.Sprintf("failed to search document: %s", err))
+	}


I think this is net worse than what we had - we now have to check for error twice? Shouldn't err the first time be what we put into searchResponse.Err()?

Yes and nope.
When doing the request the .Do function returns the response and an error. For example this Cat request. The returned error from .Do is most likely a network error or something with the request it self.

Compared to the Err() function which checks the actual returned response for errors. If you wanted do this in an single error check, you would need to use the .Err() function in each .Do() function, as right now the .Do functions does not care about the actual response.

That's what I am saying, shouldn't we make .Do care about the actual response?

As a user I want to write this:

createIndexResponse, err := createIndex.Do(context.Background(), client) if err != nil { // err here is either a network error, or the error from the response fmt.Println(fmt.Sprintf("failed to create index: %s", err)) os.Exit(1) }

WDYT?

Depends on what the lib maintainers wants.
Pro:

The lib has a standard way on handling response errors, which prevent other users from doing it by them self

You only need to check one error

Adds the possibility to have a custom error type for some requests

Would make it easier for beginners to use the lib as the don´t need to handle the errors by them self

Cons:

Existing users with their own implementation needs to check the returned error. For example if the status code 404 is accepted. Currently this would be an error. Otherwise the question turns up, if the accepted response code should be a config setting to each Do request?

main...Jakob3xD:opensearch-go:opensearchapi-response-error

Still missing some stuff but this is how it could look like.

I like the pros and I like the implementation. The fact that it's easier for beginners is huge.

Is this (the con) a breaking change for existing users? If so we'll need to major increment the version of the client.

IMO this would be a braking change as users that did not care about the response code will now get errors they need to handle.

I will drop the current commits and cherry-pick the commits of my other branch into this after some more USER_GUIDE improvements. I want/need to add an example of the error handling.

dblock

This is so much better!

Let's add an UPGRADING.md like in #237?

opensearchapi/opensearchapi.error.go

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

dblock · 2023-03-23T15:22:13Z

Reading https://github.com/opensearch-project/opensearch-go/blob/main/UPGRADING.md#opensearchapi-error-handling I think we didn't do the best job explaining how before/after for error handling changed. You had to do isError() before and now you don't. Care to add something to that @Jakob3xD?

dblock · 2023-03-29T13:54:03Z

@Jakob3xD I tried sending you an e-mail about something, maybe it went to spam? :) mind dropping me a note to dblock[at]dblock[dot]org or dblock[at]amazon[dot]com? Thaaanks!

Jakob3xD · 2023-04-06T16:42:18Z

Closes #220

wbeckler · 2023-05-09T15:09:54Z

There might have been something amiss in the way we updated the user guide. A user has run into a compilation issue with the Error type: https://forum.opensearch.org/t/ide-doesnt-see-error-type-in-opensearch-go-example/14224

dblock · 2023-05-09T17:47:53Z

Might just be an older client version @wbeckler.

…ensearch-project#246)" This reverts commit 8d3cf4d.

…ensearch-project#246)" This reverts commit 8d3cf4d. Signed-off-by: Vacha Shah <vachshah@amazon.com>

#322) * Revert "Draft: Add Err() function to Response for detailed errors (#246)" This reverts commit 8d3cf4d. Signed-off-by: Vacha Shah <vachshah@amazon.com> * Fixing PIT APIs introduced in PR #253 since they use response.Err() Signed-off-by: Vacha Shah <vachshah@amazon.com> * Removing opensearchapi.Error usage from snapshot tests introduced in PR #237 Signed-off-by: Vacha Shah <vachshah@amazon.com> * Resolve Changelog conflicts Signed-off-by: Vacha Shah <vachshah@amazon.com> --------- Signed-off-by: Vacha Shah <vachshah@amazon.com>

dblock · 2023-06-12T18:12:05Z

@Xe ranted about the ES client in https://xeiaso.net/blog/elasticsearch - I think we fixed the error handling part fully in this PR, @Jakob3xD care to take a look at the blog post? anything I've missed?

Jakob3xD · 2023-06-12T20:09:55Z

I wouldn't say we cover it fully. In Go it is common to have pre-defined error types that you can match on. So for example you have the error type ErrIndexNotFound which the user can then use to match against his error. But as this is hard to maintain our implementation is more generic but at least existing 😅. We do the json to go part for the user and return one error so you don't need to check the response for errors your self.

Beside that I can understand his anger. That's why I want to rework the lib to make it more idiomatic.

dblock · 2023-06-12T20:11:06Z

Beside that I can understand his anger. That's why I want to rework the lib to make it more idiomatic.

Please!

MaxKsyunz · 2023-07-26T18:41:06Z

Was this PR reverted? Not seeing it in main or the 2.3 release.

dblock · 2023-07-26T19:11:41Z

Yeah, but I don't know what happened here, @VachaShah?

VachaShah · 2023-07-26T19:24:41Z

Yeah, but I don't know what happened here, @VachaShah?

Thats weird, I don't see this commit but no revert happened for this as well. Taking a deeper look.

VachaShah · 2023-07-26T19:25:27Z

Was this supposed to be a breaking change?

dblock · 2023-07-26T19:28:09Z

2f58bf6 but that's on 2.3 - I guess because it was a breaking change

dblock · 2023-07-26T19:30:36Z

Was this PR reverted? Not seeing it in main or the 2.3 release.

It's on main though, but was reverted from 2.3 as breaking?

VachaShah · 2023-07-26T21:12:07Z

Yeah I reverted before 2.3 release since this was a breaking change to do the next 2.x release.

opensearch-project#322) * Revert "Draft: Add Err() function to Response for detailed errors (opensearch-project#246)" This reverts commit 8d3cf4d. Signed-off-by: Vacha Shah <vachshah@amazon.com> * Fixing PIT APIs introduced in PR opensearch-project#253 since they use response.Err() Signed-off-by: Vacha Shah <vachshah@amazon.com> * Removing opensearchapi.Error usage from snapshot tests introduced in PR opensearch-project#237 Signed-off-by: Vacha Shah <vachshah@amazon.com> * Resolve Changelog conflicts Signed-off-by: Vacha Shah <vachshah@amazon.com> --------- Signed-off-by: Vacha Shah <vachshah@amazon.com>

#322) (#384) * Revert "Draft: Add Err() function to Response for detailed errors (#246)" This reverts commit 8d3cf4d. * Fixing PIT APIs introduced in PR #253 since they use response.Err() * Removing opensearchapi.Error usage from snapshot tests introduced in PR #237 * Resolve Changelog conflicts --------- Signed-off-by: Vacha Shah <vachshah@amazon.com>

Jakob3xD force-pushed the api-response-error branch 5 times, most recently from 0a39d49 to 1a28767 Compare March 3, 2023 15:10

dblock reviewed Mar 3, 2023

View reviewed changes

opensearchapi/opensearchapi.response.go Outdated Show resolved Hide resolved

Jakob3xD force-pushed the api-response-error branch 2 times, most recently from 6ada302 to a9137e0 Compare March 3, 2023 17:37

dblock reviewed Mar 6, 2023

View reviewed changes

opensearchapi/opensearchapi_response_internal_test.go Show resolved Hide resolved

dblock reviewed Mar 6, 2023

View reviewed changes

USER_GUIDE.md Show resolved Hide resolved

Jakob3xD force-pushed the api-response-error branch from a9137e0 to 531d416 Compare March 15, 2023 08:53

dblock reviewed Mar 16, 2023

View reviewed changes

dblock mentioned this pull request Mar 17, 2023

SnapshotDeleteRequest use []string instead of string #237

Merged

Jakob3xD force-pushed the api-response-error branch 2 times, most recently from 835b0dc to b028a9a Compare March 17, 2023 22:01

Jakob3xD marked this pull request as ready for review March 20, 2023 17:58

Jakob3xD requested review from VijayanB and VachaShah as code owners March 20, 2023 17:58

dblock previously approved these changes Mar 20, 2023

View reviewed changes

opensearchapi/opensearchapi.error.go Outdated Show resolved Hide resolved

Jakob3xD added 10 commits March 21, 2023 11:00

opensearchapi: Add Error type

a4257ef

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

opensearchapi: Add Err() function to response

fbb209d

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

opensearchapi: Remove unneded IsError check

fa6cc9b

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

opensearchapi: Add tests for reponse.Err() function

b8d0e02

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

opensearchutil: Remove unneded IsError() check

62928f4

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

USER_GUIDE: Remove IsError check, improve example, add error.As example

23e3f57

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

opensearchapi: Check reponse body for errors in all api types

0245908

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Update CHANGELOG.md

2d9bfcc

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

opensearchapi: Adjust benchmarks to not brake on wanted Response Errors

acbf0c1

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Add UPGRADING.md

0d6a52f

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Jakob3xD dismissed dblock’s stale review via 0d6a52f March 21, 2023 10:17

dblock approved these changes Mar 22, 2023

View reviewed changes

dblock merged commit 8d3cf4d into opensearch-project:main Mar 22, 2023

Jakob3xD deleted the api-response-error branch March 23, 2023 08:41

dblock mentioned this pull request Mar 23, 2023

[FEATURE] Release 3.0 #264

Closed

Jakob3xD mentioned this pull request Mar 23, 2023

Upgrade.md: Add before example for error handling #265

Merged

This was referenced Mar 29, 2023

[BUG] Unclear how to get a detailed error #220

Closed

Added @Jakob3xD as a co-maintainer. #270

Merged

Jakob3xD changed the title ~~Draft: Add Err() function to Response for detailed errors~~ Add Err() function to Response for detailed errors Apr 6, 2023

VachaShah added a commit to VachaShah/opensearch-go that referenced this pull request May 18, 2023

Revert "Draft: Add Err() function to Response for detailed errors (op…

73a0221

…ensearch-project#246)" This reverts commit 8d3cf4d.

VachaShah added a commit to VachaShah/opensearch-go that referenced this pull request May 18, 2023

Revert "Draft: Add Err() function to Response for detailed errors (op…

ffed0df

…ensearch-project#246)" This reverts commit 8d3cf4d. Signed-off-by: Vacha Shah <vachshah@amazon.com>

This was referenced Oct 3, 2023

Revert "Draft: Add Err() function to Response for detailed errors (#2… #383

Closed

Revert "Draft: Add Err() function to Response for detailed errors (#2… #384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Err() function to Response for detailed errors #246

Add Err() function to Response for detailed errors #246

Jakob3xD commented Mar 3, 2023

dblock left a comment

dblock left a comment

dblock Mar 16, 2023

Jakob3xD Mar 16, 2023

dblock Mar 16, 2023

Jakob3xD Mar 16, 2023 •

edited

Loading

Jakob3xD Mar 17, 2023

dblock Mar 17, 2023

Jakob3xD Mar 17, 2023

dblock left a comment

dblock commented Mar 23, 2023

dblock commented Mar 29, 2023

Jakob3xD commented Apr 6, 2023

wbeckler commented May 9, 2023

dblock commented May 9, 2023

dblock commented Jun 12, 2023

Jakob3xD commented Jun 12, 2023

dblock commented Jun 12, 2023

MaxKsyunz commented Jul 26, 2023

dblock commented Jul 26, 2023

VachaShah commented Jul 26, 2023

VachaShah commented Jul 26, 2023

dblock commented Jul 26, 2023 •

edited

Loading

dblock commented Jul 26, 2023

VachaShah commented Jul 26, 2023

Add Err() function to Response for detailed errors #246

Add Err() function to Response for detailed errors #246

Conversation

Jakob3xD commented Mar 3, 2023

Description

Issues Resolved

dblock left a comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock Mar 16, 2023

Choose a reason for hiding this comment

Jakob3xD Mar 16, 2023

Choose a reason for hiding this comment

dblock Mar 16, 2023

Choose a reason for hiding this comment

Jakob3xD Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Jakob3xD Mar 17, 2023

Choose a reason for hiding this comment

dblock Mar 17, 2023

Choose a reason for hiding this comment

Jakob3xD Mar 17, 2023

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock commented Mar 23, 2023

dblock commented Mar 29, 2023

Jakob3xD commented Apr 6, 2023

wbeckler commented May 9, 2023

dblock commented May 9, 2023

dblock commented Jun 12, 2023

Jakob3xD commented Jun 12, 2023

dblock commented Jun 12, 2023

MaxKsyunz commented Jul 26, 2023

dblock commented Jul 26, 2023

VachaShah commented Jul 26, 2023

VachaShah commented Jul 26, 2023

dblock commented Jul 26, 2023 • edited Loading

dblock commented Jul 26, 2023

VachaShah commented Jul 26, 2023

Jakob3xD Mar 16, 2023 •

edited

Loading

dblock commented Jul 26, 2023 •

edited

Loading