Skip to content
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

ResourceExhausted error code to HTTP 429 status code #270

Merged
merged 2 commits into from
Sep 16, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clientcompat/internal/clientcompat/clientcompat.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions docs/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Twirp server implementations can return regular errors too, but those
will be wrapped with `twirp.InternalErrorWith(err)`, so they are also
`twirp.Error` values when received by the clients.

Check the [Errors Spec](spec_v5.md) for more information on error
Check the [Errors Spec](spec_v7.md) for more information on error
codes and the wire protocol.

Also don't be afraid to open the [source code](https://github.com/twitchtv/twirp/blob/master/errors.go)
Expand Down Expand Up @@ -52,7 +52,7 @@ Each error code is defined by a constant in the `twirp` package:
| AlreadyExists | already_exists | 409 Conflict
| PermissionDenied | permission_denied | 403 Forbidden
| Unauthenticated | unauthenticated | 401 Unauthorized
| ResourceExhausted | resource_exhausted | 403 Forbidden
| ResourceExhausted | resource_exhausted | 429 Too Many Requests
| FailedPrecondition | failed_precondition | 412 Precondition Failed
| Aborted | aborted | 409 Conflict
| OutOfRange | out_of_range | 400 Bad Request
Expand All @@ -61,7 +61,7 @@ Each error code is defined by a constant in the `twirp` package:
| Unavailable | unavailable | 503 Service Unavailable
| DataLoss | dataloss | 500 Internal Server Error

For more information on each code, see the [Errors Spec](spec_v5.md).
For more information on each code, see the [Errors Spec](spec_v7.md).

### HTTP Errors from Intermediary Proxies

Expand Down Expand Up @@ -123,8 +123,8 @@ part of the Protobuf messages (add an error field to proto messages).

### Writing HTTP Errors outside Twirp services

Twirp services can be [muxed with other HTTP services](mux.md). For consistent responses
and error codes _outside_ Twirp servers, such as http middlewares, you can call `twirp.WriteError`.
Twirp services can be [muxed with other HTTP services](mux.md). For consistent responses
and error codes _outside_ Twirp servers, such as http middlewares, you can call `twirp.WriteError`.

The error is expected to satisfy a `twirp.Error`, otherwise it is wrapped with `twirp.InternalError`.

Expand All @@ -134,7 +134,7 @@ Usage:
rpc.WriteError(w, twirp.NewError(twirp.Unauthenticated, "invalid token"))
```

To simplify `twirp.Error` composition, a few constructors are available, such as `NotFoundError`
To simplify `twirp.Error` composition, a few constructors are available, such as `NotFoundError`
and `RequiredArgumentError`. See [docs](https://godoc.org/github.com/twitchtv/twirp#Error).

With constructor:
Expand Down
4 changes: 2 additions & 2 deletions docs/routing.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ For example, to make a hat with the [Haberdasher service](example.md):
POST /twirp/twirp.example.haberdasher.Haberdasher/MakeHat
```

More details on the [protocol specification](spec_v5.md).
More details on the [protocol specification](spec_v7.md).

#### Naming Style

Please follow the [Protocol Buffers Style Guide](https://developers.google.com/protocol-buffers/docs/style#services). In particular, the `<Service>` and `<Method>` names should be CamelCased (with an initial capital).

The [official Go implementation](https://github.com/twitchtv/twirp) differs in behavior from what is described in the [protocol specification](https://twitchtv.github.io/twirp/docs/spec_v5.html). The routes are modified to be CamelCase instead of using the exact from the proto file definition, so URL paths generated by Go clients may differ from paths generated by other languages. This issue is discussed in [#244](https://github.com/twitchtv/twirp/issues/244), and is easily avoided by following the Protocol Buffers Style Guide on your proto files.
The [official Go implementation](https://github.com/twitchtv/twirp) differs in behavior from what is described in the [protocol specification](spec_v7.md). The routes are modified to be CamelCase instead of using the exact from the proto file definition, so URL paths generated by Go clients may differ from paths generated by other languages. This issue is discussed in [#244](https://github.com/twitchtv/twirp/issues/244), and is easily avoided by following the Protocol Buffers Style Guide on your proto files.

### Content-Type Header (json or protobuf)

Expand Down
2 changes: 1 addition & 1 deletion docs/spec_v7.md
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ corresponding HTTP Status Code for the response.
| already_exists | 409 | An attempt to create an entity failed because one already exists.
| permission_denied | 403 | The caller does not have permission to execute the specified operation. It must not be used if the caller cannot be identified (use "unauthenticated" instead).
| unauthenticated | 401 | The request does not have valid authentication credentials for the operation.
| resource_exhausted | 403 | Some resource has been exhausted, perhaps a per-user quota, or perhaps the entire file system is out of space.
| resource_exhausted | 429 | Some resource has been exhausted, perhaps a per-user quota, or perhaps the entire file system is out of space.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth mentioning rate-limiting here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding "exhausted or rate-limited"?

Some resource has been exhausted or rate-limited, perhaps a per-user quota, or perhaps the entire file system is out of space

| failed_precondition | 412 | The operation was rejected because the system is not in a state required for the operation's execution. For example, doing an rmdir operation on a directory that is non-empty, or on a non-directory object, or when having conflicting read-modify-write on the same resource.
| aborted | 409 | The operation was aborted, typically due to a concurrency issue like sequencer check failures, transaction aborts, etc.
| out_of_range | 400 | The operation was attempted past the valid range. For example, seeking or reading past end of a paginated collection. Unlike "invalid_argument", this error indicates a problem that may be fixed if the system state changes (i.e. adding more items to the collection). There is a fair bit of overlap between "failed_precondition" and "out_of_range". We recommend using "out_of_range" (the more specific error) when it applies so that callers who are iterating through a space can easily look for an "out_of_range" error to detect when they are done.
Expand Down
2 changes: 1 addition & 1 deletion errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ func ServerHTTPStatusFromErrorCode(code ErrorCode) int {
case Unauthenticated:
return 401 // Unauthorized
case ResourceExhausted:
return 403 // Forbidden
return 429 // Too Many Requests
case FailedPrecondition:
return 412 // Precondition Failed
case Aborted:
Expand Down
4 changes: 3 additions & 1 deletion example/service.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion internal/twirptest/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ func TestClientIntermediaryErrors(t *testing.T) {
401: twirp.Unauthenticated,
403: twirp.PermissionDenied,
404: twirp.BadRoute,
429: twirp.Unavailable,
429: twirp.ResourceExhausted,
502: twirp.Unavailable,
503: twirp.Unavailable,
504: twirp.Unavailable,
Expand Down
4 changes: 3 additions & 1 deletion internal/twirptest/empty_service/empty_service.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/gogo_compat/service.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/google_protobuf_imports/service.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/importable/importable.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/importer/importer.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/importer_local/importer_local.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/importmapping/x/x.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/multiple/multiple1.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/no_package_name/no_package_name.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/proto/proto.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/service.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion internal/twirptest/source_relative/source_relative.twirp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion protoc-gen-twirp/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,9 @@ func (t *twirp) generateUtils() {
t.P(` code = `, t.pkgs["twirp"], `.PermissionDenied`)
t.P(` case 404: // Not Found`)
t.P(` code = `, t.pkgs["twirp"], `.BadRoute`)
t.P(` case 429, 502, 503, 504: // Too Many Requests, Bad Gateway, Service Unavailable, Gateway Timeout`)
t.P(` case 429: // Too Many Requests`)
t.P(` code = `, t.pkgs["twirp"], `.ResourceExhausted`)
t.P(` case 502, 503, 504: // Too Many Requests, Bad Gateway, Service Unavailable, Gateway Timeout`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "Too Many Requests" from this line

t.P(` code = `, t.pkgs["twirp"], `.Unavailable`)
t.P(` default: // All other codes`)
t.P(` code = `, t.pkgs["twirp"], `.Unknown`)
Expand Down