-
Notifications
You must be signed in to change notification settings - Fork 3
Add doc on error handling #19
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
Conversation
6136ab2
to
21f3980
Compare
Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
9b73ddf
to
a7bd90b
Compare
docs/errors.md
Outdated
```json | ||
HTTP/1.1 400 Bad Request | ||
Content-Type: application/json | ||
|
||
{ | ||
"code": "invalid_argument", | ||
"message": "name is required", | ||
"details": [ | ||
{ | ||
"type": "google.protobuf.Struct", | ||
"value": "base64-encoded-protobuf" | ||
} | ||
] | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure if http
is a valid markdown language tag, but could be more appropriate here if it is. (although then maybe the json
wouldn't be pretty 😞.)
can probably just ignore this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I changed json
to http
now
Select error codes that accurately reflect the situation: | ||
|
||
- Use `INVALID_ARGUMENT` for malformed requests that should never be retried | ||
- Use `FAILED_PRECONDITION` for requests that might succeed if the system state changes | ||
- Use `UNAVAILABLE` for transient failures that should be retried | ||
- Use `INTERNAL` sparingly - it indicates a bug in your code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might link to https://connectrpc.com/docs/protocol#error-codes; the discussion below the table about how to choose an error code is pretty useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks! I just wrote the link below.
violation = bad_request.field_violations.add() | ||
violation.field = field | ||
violation.description = error | ||
raise ConnectError(Code.INVALID_ARGUMENT, "Validation failed", details=[bad_request]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my only hesitation on recommending error details is that in practice, they can make it more difficult to debug if your client isn't already deserializing them. And there's no way to strongly enforce them in your API contract; the best you can really do is just document them.
I think this is fine to leave as-is, just wish error details were easier to use in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is trying to match the coverage of Go docs which similarly cover error details so is probably good as is. Agree that error details are a bit clunky but too useful for substituting for custom error codes to leave out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I agree with both opinions, so I just added this https://github.com/connectrpc/connect-python/pull/19/files#diff-72376d0b487d7231cf31959bf266f6aea098e899743bae02e36d176cef4db476R210
Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
This PR adds error handling documentation covering error codes, client/server examples, error details, and best practices.