-
Notifications
You must be signed in to change notification settings - Fork 16
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
chore: cleanup core api default body limit #336
Conversation
Docker tags |
2ba0021
to
0d0135d
Compare
Locally, I have set on
and here:
... so the limit works, but the error is surfaced rather differently (out of which 413 would be my preferred). |
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.
Nice. Thanks for updating things. A few minor tweaks needed before merging 👍
async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> { | ||
match axum::Json::<T>::from_request_parts(parts, state).await { | ||
Ok(value) => Ok(Self(value.0)), | ||
Err(rejection) => Err(client_error(format!("{:?}", rejection))), |
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.
Looks like this is now throwing the error for request size, so we will probably want to handle it properly and turn it into a 403.
Similarly on line 47? Perhaps this logic can be unified into a method?
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.
Fixed. Removed the unused from_request_parts
.
@@ -12,18 +14,35 @@ use super::{client_error, ResponseError}; | |||
|
|||
#[derive(Debug)] | |||
pub(crate) struct Json<T>(pub T); | |||
pub use axum::Extension; // Re-export Extension so that it can be used easily | |||
pub use axum::extract::State; // Re-export Extension so that it can be used easily |
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.
Comment needs updating :)
@@ -93,8 +92,6 @@ pub async fn create_server<F>( | |||
{ | |||
let core_api_state = CoreApiState { state_manager }; | |||
|
|||
// TODO - Change to remove the Tower RequestBodyLimitLayer middleware and use DefaultBodyLimit::max |
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.
Can we remove the tower dependency now? Or is it still used elsewhere?
Also could we update LARGE_REQUEST_MAX_BYTES now? 👍 I imagine it can be more like 3 MB now that the max transaction size is about 1MB?
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.
Done.
Signed-off-by: Alexandru Murtaza <alexandru.murtaza@rdx.works>
… max size Signed-off-by: Alexandru Murtaza <alexandru.murtaza@rdx.works>
0d0135d
to
d6e58f1
Compare
Kudos, SonarCloud Quality Gate passed! |
Update axum's version. Check breaking changelog.
Remove Tower
RequestBodyLimitLayer
middleware and useDefaultBodyLimit::max
(see TODO and tokio-rs/axum#1397).