nspcc-dev / neofs-sdk-go

Go implementation of NeoFS SDK
Apache License 2.0
6 stars 14 forks source link

Drop ResolveNeoFSFailures #406

Closed roman-khimov closed 1 year ago

roman-khimov commented 1 year ago

Do we have anyone using SDK client without ResolveNeoFSFailures setting? Why anyone needs it? We already have these protocol errors represented as proper Go errors, they should be usable via a standard errors.Is() to check for something specific and I don't see any other useful use case for them. They're just code/message/details message as per API definition, there can't be any additional redirect data or some other fancy headers to look for. Even if they're to be added, specific errors can have specific methods to get this data. But we can greatly simplify our library interfaces if we're to always represent errors as errors and not some random structured types.

Related to #405.

cthulhu-rider commented 1 year ago

Do we have anyone using SDK client without ResolveNeoFSFailures setting?

i guess no. Even if so, it would be much more convenient to switch to Go-style error handling. In particular, if necessary, to get the transport status again (e.g. response relay), you can use the apistatus package functionality

notimetoname commented 1 year ago

Do we have anyone using SDK client without ResolveNeoFSFailures setting?

@roman-khimov, do you mean not calling ResolveNeoFSFailures at all? Well, i guess every client that is not a cli (in the current neofs code base)?

As i remember, we wanted to separate network errors and the "logical" errors on API layer. It can be used in the code like that (well, it is arguable but that is how it works now: any error is non-logical and should be handled inside mutli cache (for reconnection cooldown)). Also, i can remember arguments like the following: an API response is a response and it is not fully clear how to decide if it is an error or not so let a client decide according to his desires.

My opinion: if it is possible to make it easy and clear without ResolveNeoFSFailures but saving at the same time every possibility that we have been providing until now, it must be done.

P.S.: original discussion is somewhere near #91. I also try to remember that the ResolveNeoFSFailures was requested by the services but i can be wrong.

roman-khimov commented 1 year ago

do you mean not calling ResolveNeoFSFailures at all

Yes.

It can be used in the code like that

Yep, something like that can easily cover all the possibilities. errors.Is() to check for some specific error if it should be treated in some specific way, errors.As() for getting error-specific API if it's available for some cases.

The implementation pattern we have now is somewhat reminiscent of http.Do, but HTTP is a much more complex beast with a lot of not-really-an-error cases like redirects and a set of different successful codes that can have some meaning to the client. We have OK and a set of "sorry, you can't do anything about it" things. The only exception maybe is NODE_UNDER_MAINTENANCE (switch to another one!), but errors.Is can handle it.

What people really want is more like https://pkg.go.dev/github.com/emersion/go-webdav#Client or https://pkg.go.dev/github.com/studio-b12/gowebdav@v0.0.0-20230203202212-3282f94193f2#Client

notimetoname commented 1 year ago

@roman-khimov, can you, please, provide an example of separating any network and API error? I mean one-line code as you see it. Will it be possible to indicate all of them with errors.As or Is (some internal wrapper?)?

We have OK and a set of "sorry, you can't do anything about it" things.

I would also add here that we might have some "intermediate" errors in the future. Even now i can't say for sure that "incomplete object put" is a "true" error (it is not an API status now but there were some discussions about it).

roman-khimov commented 1 year ago

separating any network and API error

Likely not possible now, but can be done if we're to provide generic apistatus.Error to match against (Is() implementations of specific errors can then handle it).

notimetoname commented 1 year ago

Likely not possible now, but can be done if we're to provide generic apistatus.Error to match against

OK, my vote is "for" dropping ResolveNeoFSFailures and providing the NetworkError in any convenient way.

roman-khimov commented 1 year ago