nspcc-dev / neofs-node

NeoFS is a decentralized distributed object storage integrated with the Neo blockchain
https://fs.neo.org
GNU General Public License v3.0
31 stars 38 forks source link

Reconsider status responses #2744

Open carpawell opened 5 months ago

carpawell commented 5 months ago

Some status errors in NeoFS are known and self-describing. But there are also "general errors" with 1024 status codes and some "good" human-readable messages.

Expected Behavior

If I have a general error, I expect NOT a general message that can help me understand what is happening. E.g. session token does not relate to the container a request is trying to change:

status: code = 1024 message = could not execute SetEACL request: session token validation: wrong container: 

Current Behavior

Only the last error in the errors chain is attached:

status: code = 1024 message = wrong container: 

Possible Solution

  1. errors.Join for meaningful errros inside a package? (like session tokens errors can be wrapped with a common session token error with detailed errors so the Unwrap does not unwrap more than it is needed as the package thinks)
  2. Make all the low-level errors well described and have more context than they have now? (can be a huge work)
  3. Attach the full error chain if it is a 1024 error? (chain could be too long and too golang specific)
  4. Attach 1-2 more errors from the lowest one? (a strange rule can fix smth but not all the cases)

Steps to Reproduce (for bugs)

Any 1024 "general" error case (like incorrect session token; even though session token error may become another status case, there always be some "general" errors that should be described correctly).

Context

https://github.com/nspcc-dev/neofs-node/blob/996b1eae7bd1f1ad5693758b38c9c025523842ac/pkg/services/util/sign.go#L225-L229

Regression

No.

Your Environment

0.40.0

roman-khimov commented 5 months ago

Server should return as much data as it can (except when it somehow affects security), simple as that. Joining/chaining errors is fine for that and the protocol allows for it, message can be anything. Then this message should be a part of client error.

carpawell commented 5 months ago

@roman-khimov, so you are for the full error message?

roman-khimov commented 5 months ago

Absolutely. It's not that much different from https://github.com/neo-project/proposals/pull/156

carpawell commented 5 months ago

Well, I agree too, I think. I just try to understand why it was done like that. I think the idea was that "a user do not want to see Invalid table argument to Dexie.transaction(). Only Table or String are allowed [Caught in: [MWPTransactor] Error performing loadExpired transaction]" (nicely copied from the browser's console) errors in the response. Maybe.

roman-khimov commented 2 months ago

1024 in ContainerDelete delete: status: code = 1024 message = session was not issued by the container owner, issuer: NehJedoS24C7LkeJCzSFpApivRmHKJe1Q6 is rather strange. 2048 is almost perfect except it's defined for object operations.