-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
✨ Add support for uploading books #492
Conversation
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'm approving now so you aren't blocked by me being away, however there are a couple of frontend issues that will have to be addressed before merge (my fault). Edit to add they should be resolved, but I haven't had time to fully test it
This otherwise looks great! Thanks for knocking it out!
for i in 0..zip_archive.len() { | ||
let mut zip_file = zip_archive.by_index(i).map_err(|e| { | ||
APIError::InternalServerError(format!( | ||
"Error accessing file in series zip: {e}" | ||
)) | ||
})?; | ||
|
||
// Skip directories | ||
if zip_file.is_dir() { | ||
continue; | ||
} | ||
|
||
// Using `enclosed_name` also validates against path traversal attacks: | ||
// https://docs.rs/zip/1.1.3/zip/read/struct.ZipFile.html#method.enclosed_name | ||
let Some(enclosed_path) = zip_file.enclosed_name() else { | ||
return Err(APIError::InternalServerError( | ||
"Series zip contained a malformed path".to_string(), | ||
)); | ||
}; | ||
// Get the path that the archive file will be extracted to | ||
let extraction_path = series_path.join(enclosed_path); | ||
|
||
// Error if the file already exists and we aren't allowing overwrites | ||
if extraction_path.exists() && !allow_overwrite { | ||
return Err(APIError::InternalServerError(format!( | ||
"Unable to extract zip contents to {extraction_path:?}, overwrites are disabled" | ||
))); | ||
} | ||
|
||
validate_zip_file(&mut zip_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.
We should make a note to async-ify this down the road, the zip
crate doesn't have an async API via a feature flag or anything so it would just be spawning a task to handle the blocking IO (e.g. here). This isn't critical though, so I think it is more than acceptable as-is
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.
https://crates.io/crates/async_zip
I used this once in the past, but I recall that I ran into some difficulties as well. Might have been user error, however.
packages/browser/src/components/explorer/upload/UploadModal.tsx
Outdated
Show resolved
Hide resolved
packages/browser/src/scenes/library/tabs/files/LibraryExplorerScene.tsx
Outdated
Show resolved
Hide resolved
packages/browser/src/scenes/series/tabs/files/SeriesExplorerScene.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Aaron Leopold <36278431+aaronleopold@users.noreply.github.com>
Just tested the changes it looks to be working on my end. I believe this is ready to merge and will do so later this evening. |
This pull request addresses #133. It adds support for an in-app upload interface by which books or a zip archive of books can be added to an existing library.
The
enable_upload
configuration variable must be set totrue
to enable this feature, by default it isfalse
. Unless enabled, the upload routes will not be present on the server, and the web application will not display the upload button.Testing is needed to improve this feature and get it ready to be added to develop.