-
Notifications
You must be signed in to change notification settings - Fork 95
[5/n] [dropshot] add support for per-endpoint size limits #1171
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
Changes from all commits
b97472e
a8cfa4d
bd693e0
d33aff7
633d208
9b02b18
45e00a5
09cb551
2ec7ac4
6fb01a4
65d0e69
55fc9ad
088aa70
176b43d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,8 +174,14 @@ impl<Context: ServerContext> RequestContext<Context> { | |
| } | ||
|
|
||
| /// Returns the maximum request body size. | ||
| /// | ||
| /// This is typically the same as | ||
| /// `self.server.config.request_body_max_bytes`, but can be overridden on a | ||
| /// per-endpoint basis. | ||
| pub fn request_body_max_bytes(&self) -> usize { | ||
| self.server.config.default_request_body_max_bytes | ||
| self.endpoint | ||
| .request_body_max_bytes | ||
| .unwrap_or(self.server.config.default_request_body_max_bytes) | ||
| } | ||
|
|
||
| /// Returns the appropriate count of items to return for a paginated request | ||
|
|
@@ -218,6 +224,9 @@ pub struct RequestEndpointMetadata { | |
|
|
||
| /// The expected request body MIME type. | ||
| pub body_content_type: ApiEndpointBodyContentType, | ||
|
|
||
| /// The maximum number of bytes allowed in the request body, if overridden. | ||
| pub request_body_max_bytes: Option<usize>, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of calling this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, not sure -- personally I feel like if you're using this you're likely also aware of the default in the config, maybe? I generally lean towards shorter names over spelling things out completely, especially when there's a clear chain of implication. |
||
| } | ||
|
|
||
| /// Helper trait for extracting the underlying Context type from the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -707,7 +707,7 @@ | |
| //! | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment belongs at L309, but, you know, thanks GitHub. I think you want to add this to the synopsis at L309 and also add a note about it in that section (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, the synopsis doesn't cover all the arguments today so it would be a bigger change. I can make this change but it would be a substantial one that I would do separately. (This is where I feel like not having everything in the lib.rs documentation, but instead having an external documentation site, might be useful.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, so I just realized that Yikes -- I think the list of arguments should only be specified in one spot!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #1181 does that. I'd like to not block on it if that's ok, I think that's a bigger fix that should be done in a followup. |
||
| //! ```text | ||
| //! // introduced in 1.0.0, present in all subsequent versions | ||
| //! versions = "1.0.0".. | ||
| //! versions = "1.0.0".. | ||
| //! | ||
| //! // removed in 2.0.0, present in all previous versions | ||
| //! // (not present in 2.0.0 itself) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Copyright 2024 Oxide Computer Company | ||
|
|
||
| #![allow(unused_imports)] | ||
|
|
||
| use dropshot::channel; | ||
| use dropshot::RequestContext; | ||
| use dropshot::WebsocketUpgrade; | ||
|
|
||
| // Test: request_body_max_bytes specified for channel (this parameter is only | ||
| // accepted for endpoints, not channels) | ||
|
|
||
| #[channel { | ||
| protocol = WEBSOCKETS, | ||
| path = "/test", | ||
| request_body_max_bytes = 1024, | ||
| }] | ||
| async fn bad_channel( | ||
| _rqctx: RequestContext<()>, | ||
| _upgraded: WebsocketUpgrade, | ||
| ) -> dropshot::WebsocketChannelResult { | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| error: extraneous member `request_body_max_bytes` | ||
| --> tests/fail/bad_channel28.rs:15:5 | ||
| | | ||
| 15 | request_body_max_bytes = 1024, | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // Copyright 2024 Oxide Computer Company | ||
|
|
||
| #![allow(unused_imports)] | ||
|
|
||
| use dropshot::endpoint; | ||
| use dropshot::HttpError; | ||
| use dropshot::HttpResponseUpdatedNoContent; | ||
| use dropshot::RequestContext; | ||
|
|
||
| // Test: incorrect type for request_body_max_bytes. | ||
|
|
||
| #[endpoint { | ||
| method = GET, | ||
| path = "/test", | ||
| request_body_max_bytes = "not_a_number" | ||
| }] | ||
| async fn bad_endpoint( | ||
| _rqctx: RequestContext<()>, | ||
| ) -> Result<HttpResponseUpdatedNoContent, HttpError> { | ||
| Ok(HttpResponseUpdatedNoContent()) | ||
| } | ||
|
|
||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| error[E0308]: mismatched types | ||
| --> tests/fail/bad_endpoint28.rs:15:30 | ||
| | | ||
| 15 | request_body_max_bytes = "not_a_number" | ||
| | ^^^^^^^^^^^^^^ expected `usize`, found `&str` | ||
| 16 | }] | ||
| 17 | async fn bad_endpoint( | ||
| | ------------ arguments to this method are incorrect | ||
| | | ||
| note: method defined here | ||
| --> src/api_description.rs | ||
| | | ||
| | pub fn request_body_max_bytes(mut self, max_bytes: usize) -> Self { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // Copyright 2024 Oxide Computer Company | ||
|
|
||
| #![allow(unused_imports)] | ||
|
|
||
| use dropshot::channel; | ||
| use dropshot::RequestContext; | ||
| use dropshot::WebsocketUpgrade; | ||
|
|
||
| #[dropshot::api_description] | ||
| trait MyServer { | ||
| type Context; | ||
|
|
||
| // Test: request_body_max_bytes specified for channel (this parameter is only | ||
| // accepted for endpoints, not channels) | ||
| #[channel { | ||
| protocol = WEBSOCKETS, | ||
| path = "/test", | ||
| request_body_max_bytes = 1024, | ||
| }] | ||
| async fn bad_channel( | ||
| _rqctx: RequestContext<Self::Context>, | ||
| _upgraded: WebsocketUpgrade, | ||
| ) -> dropshot::WebsocketChannelResult; | ||
| } | ||
|
|
||
| enum MyImpl {} | ||
|
|
||
| // This should not produce errors about items being missing. | ||
| impl MyServer for MyImpl { | ||
| type Context = (); | ||
|
|
||
| async fn bad_channel( | ||
| _rqctx: RequestContext<Self::Context>, | ||
| _upgraded: WebsocketUpgrade, | ||
| ) -> dropshot::WebsocketChannelResult { | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| // These items should be generated and accessible. | ||
| my_server_mod::api_description::<MyImpl>(); | ||
| my_server_mod::stub_api_description(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| error: endpoint `bad_channel` has invalid attributes: extraneous member `request_body_max_bytes` | ||
| --> tests/fail/bad_trait_channel28.rs:18:9 | ||
| | | ||
| 18 | request_body_max_bytes = 1024, | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| // Copyright 2024 Oxide Computer Company | ||
|
|
||
| #![allow(unused_imports)] | ||
|
|
||
| use dropshot::endpoint; | ||
| use dropshot::HttpError; | ||
| use dropshot::HttpResponseUpdatedNoContent; | ||
| use dropshot::RequestContext; | ||
|
|
||
| // Test: incorrect type for request_body_max_bytes. | ||
|
|
||
| #[dropshot::api_description] | ||
| trait MyApi { | ||
| type Context; | ||
|
|
||
| #[endpoint { | ||
| method = GET, | ||
| path = "/test", | ||
| request_body_max_bytes = "not_a_number", | ||
| }] | ||
| async fn bad_endpoint( | ||
| _rqctx: RequestContext<Self::Context>, | ||
| ) -> Result<HttpResponseUpdatedNoContent, HttpError>; | ||
| } | ||
|
|
||
| enum MyImpl {} | ||
|
|
||
| // This should not produce errors about items being missing. | ||
|
|
||
| impl MyApi for MyImpl { | ||
| type Context = (); | ||
| async fn bad_endpoint( | ||
| _rqctx: RequestContext<Self::Context>, | ||
| ) -> Result<HttpResponseUpdatedNoContent, HttpError> { | ||
| Ok(HttpResponseUpdatedNoContent()) | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| // These items should be generated and accessible. | ||
| my_api_mod::api_description::<MyImpl>(); | ||
| my_api_mod::stub_api_description(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| error[E0308]: mismatched types | ||
| --> tests/fail/bad_trait_endpoint28.rs:19:34 | ||
| | | ||
| 16 | #[endpoint { | ||
| | - arguments to this method are incorrect | ||
| ... | ||
| 19 | request_body_max_bytes = "not_a_number", | ||
| | ^^^^^^^^^^^^^^ expected `usize`, found `&str` | ||
| | | ||
| note: method defined here | ||
| --> src/api_description.rs | ||
| | | ||
| | pub fn request_body_max_bytes(mut self, max_bytes: usize) -> Self { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ |
Uh oh!
There was an error while loading. Please reload this page.