-
Notifications
You must be signed in to change notification settings - Fork 472
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
feat(services/vercel_blob): support vercel blob #4103
Conversation
7c12aef
to
f11dc24
Compare
@Xuanwo blob fails to pass the test of test_write_with_special_chars, after debugging, i feel that it is a vercel error. Maybe you can help me check it |
This test is not mentioned in the documentation. Perhaps we can simply ignore it for the Vercel blob. |
Sorry for the mistake operation |
ea4f17d
to
410b9c0
Compare
Why does seafile container not start? |
Seems seafile pushed a new tag serval hours ago: Maybe we can change the tag in docker-compose file to use the old one until they addressed. Would you like to submit a new PR for this? |
Fixed in #4107, please try again after merge with |
5ba2571
to
c592c2c
Compare
2707f5c
to
5fd43df
Compare
return Err(Error::new(ErrorKind::NotFound, "Blob not found")); | ||
} | ||
|
||
// Use the first url to download the file |
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.
Seems pathname not checked?
return Err(Error::new(ErrorKind::NotFound, "Blob not found")); | ||
} | ||
|
||
let url = &blobs[0].url; |
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.
The same here. The first blob returned in list
might not be the blob we want.
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.
For example, we have files like aa
. If users trying to get file a
, they will got aa
.
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 bad. It should have matched pathname, fixed.
pub async fn copy(&self, from: &str, to: &str) -> Result<Response<IncomingAsyncBody>> { | ||
let from = build_abs_path(&self.root, from); | ||
|
||
let resp = self.list(&from, Some(1)).await?; |
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.
This pattern show a lot. Maybe we can add a function called resolve_blob
? We can implement the get blob logic and leave detailed comments there.
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
|
||
let status = resp.status(); | ||
|
||
match status { |
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 can write like this to improvie the readability
if status != StatusCode::OK {
return Err(parse_error(resp).await?);
}
// other logic
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
// In this case, we want to create a new MPU. | ||
req = req.header("x-mpu-action", "create"); | ||
|
||
req = req.header("x-add-random-suffix", "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.
These headers are used in multiple places. We can extract them as constants and add comments to explain their meaning and possible values.
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
5fd43df
to
d981c28
Compare
d981c28
to
b4dd17a
Compare
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 a lot!
fixes #2191