-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat(server): Add batch endpoint and extractor #244
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
base: main
Are you sure you want to change the base?
Conversation
| .get(CONTENT_TYPE) | ||
| .and_then(|ct| ct.to_str().ok()) | ||
| else { | ||
| return Err((StatusCode::BAD_REQUEST, "expected valid Content-Type") |
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've tried my best to distinguish between client and server error instead of just ?ing every Result.
As a consequence, the error handling became a bit verbose in this function. Maybe there's a better way?
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.
We'll refactor the errors anyway so this will use thiserror in the future.
557301e to
e01966b
Compare
| } | ||
|
|
||
| pub struct BatchRequest { | ||
| pub operations: BoxStream<'static, anyhow::Result<Operation>>, |
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.
could you replace this anyhow::Result<Operation> with Result<Operation, BatchError> where BatchError (or whatever it should be named) is a new type that derives thiserror::Error? example from auth https://github.com/getsentry/objectstore/blob/main/objectstore-server/src/auth/error.rs
anyhow doesn't really let us differentiate between an error that should return a 400 BAD REQUEST and an error that should return a 500. i'm not sure we should be using it at all, or at least we should push our usage of it as high as we can imo
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.
Yeah this is something I noticed as well and that's why I wanted to move to using structured errors with thiserror instead of using anyhow https://linear.app/getsentry/issue/FS-220/improve-error-handling
It might be convenient for me to do that first, and then rebase this PR on top.
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 don't think the two have to be coupled. i think you can just return Result<Operation, BatchError> and the error can be converted to anyhow::Error which can be converted to AnyhowResponse in the endpoint implementation. then this file can be left alone when doing FS-220. but up to you, it'll be done at some point either way
| let Some(content_type) = request | ||
| .headers() | ||
| .get(CONTENT_TYPE) | ||
| .and_then(|ct| ct.to_str().ok()) | ||
| else { | ||
| return Err((StatusCode::BAD_REQUEST, "expected valid Content-Type").into_response()); | ||
| }; |
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.
consider replacing let ... else {} with ok_or / map_err
let content_type = request
.headers()
.get(CONTENT_TYPE)
.and_then(|ct| ct.to_str())
.map_err(|_| (StatusCode::BAD_REQUEST, "expected_valid Content-Type"))?;
similar feedback below
| where | ||
| S: Send + Sync, | ||
| { | ||
| type Rejection = Response; |
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.
FromRequest requires that your Rejection impl IntoResponse, i think you can use (StatusCode, &'static str) as your Rejection and skip calling .into_response() every time?
| payload: field.bytes().await?, | ||
| }), | ||
| "delete" => { | ||
| let key = key.context("missing object key for delet operation")?; |
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.
nitpick but typo: delet lol
| merni = { workspace = true } | ||
| mimalloc = { workspace = true } | ||
| mime = "0.3.17" | ||
| multer = "3.1.0" |
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.
why multier over https://docs.rs/axum/latest/axum/extract/struct.Multipart.html?
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.
Good catch, I think we can use axum::extract::Multipart directly now.
With the old design, which included a manifest of operations as the first part, the correct content-type would've been multipart/mixed and therefore I had to use multer (which axum::extract::Multipart uses under the hood) directly because axum::extract::Multipart only works with multipart/form-data 😅.
Now multipart/form-data is correct, so we can move to using axum::extract::Multipart directly.
This starts the work on implementing Batch Operations by introducing a batch endpoint and an extractor to validate and parse our batch request format.
The implementation roughly follows the spec linked above, tomorrow I will make sure to sync up the two.