ipfs / pinning-services-api-spec

Standalone, vendor-agnostic Pinning Service API for IPFS ecosystem
https://ipfs.github.io/pinning-services-api-spec/
Creative Commons Zero v1.0 Universal
100 stars 27 forks source link

Suggestion for error code to be switch to a string #57

Closed obo20 closed 3 years ago

obo20 commented 3 years ago

I wanted to check and see if people would be open to switching the error code returned as part of the spec to a string instead of an integer. So things would like like this:

{
    code: 'ITEM_NOT_PINNED',  (short error code)
    message: `Current user does not have this item pinned` (more detailed message)
}

From my experience, strings are a little easier to manage in the codebase and can be easily understood better by the consumer.

I'm completely fine implementing the spec as is, I just wanted to throw this up for discussion to see if people cared one way or the other.

lidel commented 3 years ago

Listing the most common error types in the spec is a good idea. While we do it, we may change the format if there is a good reason. Let's discuss both.


@obo20 Good catch regarding code.

I believe current version with numeric code and text message came with v0.0.1 and is solely inspired by IPFS Cluster API where HTTP status code in duplicated in JSON as code:

{
  code: 400
  message: 'ITEM_NOT_PINNED'
}

The problem with this approach is that multiple errors will be returned with HTTP status 400, so it becomes mostly informational, and the real "error code" needs to be returned in message.

Right now, the spec does not specify details of error response. If we decide to change this, we should make it really intuitive.

Perhaps something along these lines (details being optional):

{
  error: {
      reason: 'ITEM_NOT_PINNED'
      details: 'Current user does not have this item pinned'
  }
}

cc @andrew @aschmahmann @ipfs/wg-pinning-services

Thoughts on how to proceed?


For reference, afaik there is no standard convention, everyone returns whatever stuff they want:

aschmahmann commented 3 years ago

I don't have a particularly strong opinion here other than wanting to make sure we all agree and it's in the yaml spec.

Since it seems like the errors are service specific as opposed to shared across all implementations I feel like the question is what type of behavior do we want to encourage:

1) Having the error code be an integer basically requires that any reasonable developer would add a useful details message. Developers might also choose to have more unique error codes based on their situation. 2) Having the error code be a string essentially requires developers to have reasonable short error codes, but they might choose not to put much deatils into the details section (e.g. Current user does not have this item pinned adds minimal information over ITEM_NOT_PINNED, but perhaps there was more context to be added).

I feel like, while integer codes might encourage more flexibility for developer debugging, string codes might be more friendly and so we should go with them. However, I'm persuadable :smile:

andrew commented 3 years ago

Whilst putting together my basic pinning service api, I wasn't really sure what kinds of errors to expect to be sending back to the client, so having a list of standard(ish) ones to expect in the spec would be great.

I'm currently bubbling up any errors I get from the go-ipfs HTTP API to the client which is pretty nasty as the error code is usually 0 or 1 which I believe is intended for the CLI to return as the exit code, in other words, 0 is an ignorable error and 1 is more serious?

lidel commented 3 years ago

Good point about bubbling up codes. Personally I don't like the idea of "external" numeric code being shared by multiple error types. Switching to string makes it harder to do the wrong thing.

I took a stab at string codes in https://github.com/ipfs/pinning-services-api-spec/pull/59 – please review.