Skip to content

Commit

Permalink
refactor(error): more error handling stuff
Browse files Browse the repository at this point in the history 
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
  • Loading branch information
@simonsan
simonsan committed Jan 24, 2024
1 parent 287040d commit 2088ac7
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 53 deletions.
12 changes: 7 additions & 5 deletions src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ pub enum ErrorKind {
NotImplemented,
/// File not found: {0}
FileNotFound(String),
/// Fetting file metadata failed
GettingFileMetadataFailed,
/// Fetting file metadata failed: {0}
GettingFileMetadataFailed(String),
/// Range not valid
RangeNotValid,
/// Seeking file failed
Expand Down Expand Up @@ -85,7 +85,9 @@ impl IntoResponse for ErrorKind {
format!("path {path} is not valid unicode"),
)
.into_response(),
ErrorKind::InvalidPath(_) => todo!(),
ErrorKind::InvalidPath(path) => {
(StatusCode::BAD_REQUEST, format!("path {path} is not valid")).into_response()
}
ErrorKind::CreatingDirectoryFailed(err) => (
StatusCode::INTERNAL_SERVER_ERROR,
format!("error creating dir: {:?}", err),
Expand All @@ -99,9 +101,9 @@ impl IntoResponse for ErrorKind {
ErrorKind::FileNotFound(path) => {
(StatusCode::NOT_FOUND, format!("file not found: {path}")).into_response()
}
ErrorKind::GettingFileMetadataFailed => (
ErrorKind::GettingFileMetadataFailed(err) => (
StatusCode::INTERNAL_SERVER_ERROR,
"error getting file metadata".to_string(),
format!("error getting file metadata: {err}"),
)
.into_response(),
ErrorKind::RangeNotValid => {
Expand Down
30 changes: 15 additions & 15 deletions src/handlers/file_config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,16 @@ use crate::{
/// has_config
/// Interface: HEAD {path}/config
pub(crate) async fn has_config(
AxumPath((path, tpe, name)): AxumPath<(Option<String>, String, String)>,
AxumPath((path, tpe)): AxumPath<(Option<String>, String)>,
auth: AuthFromRequest,
) -> Result<impl IntoResponse> {
tracing::debug!("[has_config] path: {path:?}, tpe: {tpe}, name: {name}");
tracing::debug!("[has_config] path: {path:?}, tpe: {tpe}");
let path_str = path.unwrap_or_default();
let path = std::path::Path::new(&path_str);
check_auth_and_acl(auth.user, &tpe, path, AccessType::Read)?;

let storage = STORAGE.get().unwrap();
let file = storage.filename(path, &tpe, &name);
let file = storage.filename(path, &tpe, None);
if file.exists() {
Ok(())
} else {
Expand Down Expand Up @@ -68,17 +68,17 @@ pub(crate) async fn add_config(
/// Interface: DELETE {path}/config
/// FIXME: The original restic spec does not define delete_config --> but rustic did ??
pub(crate) async fn delete_config(
AxumPath((path, tpe, name)): AxumPath<(Option<String>, String, String)>,
AxumPath((path, tpe)): AxumPath<(Option<String>, String)>,
auth: AuthFromRequest,
) -> Result<impl IntoResponse> {
tracing::debug!("[delete_config] path: {path:?}, tpe: {tpe}, name: {name}");
check_name(&tpe, &name)?;
tracing::debug!("[delete_config] path: {path:?}, tpe: {tpe}");
check_name(&tpe, None)?;
let path_str = path.unwrap_or_default();
let path = Path::new(&path_str);
check_auth_and_acl(auth.user, &tpe, path, AccessType::Append)?;

let storage = STORAGE.get().unwrap();
if storage.remove_file(path, &tpe, &name).is_err() {
if storage.remove_file(path, &tpe, None).is_err() {
return Err(ErrorKind::RemovingFileFailed(path_str));
}
Ok(())
Expand Down Expand Up @@ -112,7 +112,7 @@ mod test {
// NOT CONFIG
// -----------------------
let app = Router::new()
.route("/*path", head(has_config))
.route("/:path/config", head(has_config))
.layer(middleware::from_fn(print_request_response));

let request = Request::builder()
Expand All @@ -133,7 +133,7 @@ mod test {
// HAS CONFIG
// -----------------------
let app = Router::new()
.route("/*path", head(has_config))
.route("/:path/config", head(has_config))
.layer(middleware::from_fn(print_request_response));

let request = Request::builder()
Expand Down Expand Up @@ -179,7 +179,7 @@ mod test {
// -----------------------
let repo_name_uri = ["/", &repo, "?create=true"].concat();
let app = Router::new()
.route("/*path", post(create_repository))
.route("/:path/config", post(create_repository))
.layer(middleware::from_fn(print_request_response));

let request = request_uri_for_test(&repo_name_uri, Method::POST);
Expand All @@ -195,7 +195,7 @@ mod test {
let body = Body::new(test_vec.clone());

let app = Router::new()
.route("/*path", post(add_config))
.route("/:path/config", post(add_config))
.layer(middleware::from_fn(print_request_response));

let request = Request::builder()
Expand All @@ -220,7 +220,7 @@ mod test {
// GET CONFIG
// -----------------------
let app = Router::new()
.route("/*path", get(get_config))
.route("/:path/config", get(get_config))
.layer(middleware::from_fn(print_request_response));

let request = request_uri_for_test(&uri, Method::GET);
Expand All @@ -237,7 +237,7 @@ mod test {
// - differs from tester_has_config() that we have a non empty path now
// -----------------------
let app = Router::new()
.route("/*path", head(has_config))
.route("/:path/config", head(has_config))
.layer(middleware::from_fn(print_request_response));

let request = request_uri_for_test(&uri, Method::HEAD);
Expand All @@ -249,7 +249,7 @@ mod test {
// DELETE CONFIG
// -----------------------
let app = Router::new()
.route("/*path", delete(delete_config))
.route("/:path/config", delete(delete_config))
.layer(middleware::from_fn(print_request_response));

let request = request_uri_for_test(&uri, Method::DELETE);
Expand All @@ -264,7 +264,7 @@ mod test {
// -----------------------
let repo_name_uri = ["/", &repo].concat();
let app = Router::new()
.route("/*path", post(delete_repository))
.route("/:path/config", post(delete_repository))
.layer(middleware::from_fn(print_request_response));

let request = request_uri_for_test(&repo_name_uri, Method::POST);
Expand Down
36 changes: 16 additions & 20 deletions src/handlers/file_exchange.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,13 @@ pub(crate) async fn delete_file(
let path_str = path.unwrap_or_default();
let path = Path::new(&path_str);

check_name(&tpe, &name)?;
check_name(&tpe, Some(&name))?;
check_auth_and_acl(auth.user, &tpe, path, AccessType::Append)?;

let storage = STORAGE.get().unwrap();

if let Err(e) = storage.remove_file(path, &tpe, &name) {
tracing::debug!("[delete_file] IO error: {e:?}");
return Err(ErrorKind::RemovingFileFailed(path_str));
}
storage.remove_file(path, &tpe, Some(&name))?;

Ok(())
}

Expand All @@ -78,21 +76,18 @@ pub(crate) async fn get_file(
) -> Result<impl IntoResponse> {
tracing::debug!("[get_file] path: {path:?}, tpe: {tpe}, name: {name}");

check_name(&tpe, &name)?;
check_name(&tpe, Some(&name))?;
let path_str = path.unwrap_or_default();
let path = Path::new(&path_str);

check_auth_and_acl(auth.user, &tpe, path, AccessType::Read)?;

let storage = STORAGE.get().unwrap();
let file = match storage.open_file(path, &tpe, &name).await {
Ok(file) => file,
Err(_) => {
return Err(ErrorKind::FileNotFound(path_str));
}
};
let file = storage.open_file(path, &tpe, &name).await?;

let body = KnownSize::file(file).await.unwrap();
let body = KnownSize::file(file)
.await
.map_err(|err| ErrorKind::GettingFileMetadataFailed(format!("{err:?}")))?;
let range = range.map(|TypedHeader(range)| range);
Ok(Ranged::new(range, body).into_response())
}
Expand All @@ -111,7 +106,7 @@ pub(crate) async fn get_save_file(
) -> Result<impl AsyncWrite + Unpin + Finalizer> {
tracing::debug!("[get_save_file] path: {path:?}, tpe: {tpe}, name: {name}");

check_name(tpe, &name)?;
check_name(tpe, Some(&name))?;
check_auth_and_acl(user, tpe, path.as_path(), AccessType::Append)?;

let storage = STORAGE.get().unwrap();
Expand Down Expand Up @@ -168,12 +163,13 @@ fn check_string_sha256(name: &str) -> bool {
true
}

///FIXME Move to support functoin file
pub(crate) fn check_name(tpe: &str, name: &str) -> Result<impl IntoResponse> {
match tpe {
"config" => Ok(()),
_ if check_string_sha256(name) => Ok(()),
_ => Err(ErrorKind::FilenameNotAllowed(name.to_string())),
pub(crate) fn check_name(tpe: &str, name: Option<&str>) -> Result<impl IntoResponse> {
match (tpe, name) {
("config", _) => Ok(()),
(_, Some(name)) if check_string_sha256(name) => Ok(()),
_ => Err(ErrorKind::FilenameNotAllowed(
name.unwrap_or_default().to_string(),
)),
}
}

Expand Down
8 changes: 5 additions & 3 deletions src/handlers/file_length.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub(crate) async fn file_length(
check_auth_and_acl(auth.user, &tpe, path, AccessType::Read)?;

let storage = STORAGE.get().unwrap();
let file = storage.filename(path, &tpe, &name);
let file = storage.filename(path, &tpe, Some(&name));
return if file.exists() {
let storage = STORAGE.get().unwrap();
let file = match storage.open_file(path, &tpe, &name).await {
Expand All @@ -34,8 +34,10 @@ pub(crate) async fn file_length(
};
let length = match file.metadata().await {
Ok(meta) => meta.len(),
Err(_) => {
return Err(ErrorKind::GettingFileMetadataFailed);
Err(err) => {
return Err(ErrorKind::GettingFileMetadataFailed(format!(
"path: {path:?}, tpe: {tpe}, name: {name}, err: {err}",
)));
}
};
let mut headers = HeaderMap::new();
Expand Down
21 changes: 11 additions & 10 deletions src/storage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ pub(crate) fn init_storage(storage: impl Storage) -> Result<()> {
pub trait Storage: Send + Sync + 'static {
fn create_dir(&self, path: &Path, tpe: &str) -> Result<()>;
fn read_dir(&self, path: &Path, tpe: &str) -> Box<dyn Iterator<Item = walkdir::DirEntry>>;
fn filename(&self, path: &Path, tpe: &str, name: &str) -> PathBuf;
fn filename(&self, path: &Path, tpe: &str, name: Option<&str>) -> PathBuf;
async fn open_file(&self, path: &Path, tpe: &str, name: &str) -> Result<File>;
async fn create_file(&self, path: &Path, tpe: &str, name: &str) -> Result<WriteOrDeleteFile>;
fn remove_file(&self, path: &Path, tpe: &str, name: &str) -> Result<()>;
fn remove_file(&self, path: &Path, tpe: &str, name: Option<&str>) -> Result<()>;
fn remove_repository(&self, path: &Path) -> Result<()>;
}

Expand Down Expand Up @@ -89,27 +89,28 @@ impl Storage for LocalStorage {
Box::new(walker)
}

fn filename(&self, path: &Path, tpe: &str, name: &str) -> PathBuf {
match tpe {
"config" => self.path.join(path).join("config"),
"data" => self.path.join(path).join(tpe).join(&name[0..2]).join(name),
_ => self.path.join(path).join(tpe).join(name),
fn filename(&self, path: &Path, tpe: &str, name: Option<&str>) -> PathBuf {
match (tpe, name) {
("config", _) => self.path.join(path).join("config"),
("data", Some(name)) => self.path.join(path).join(tpe).join(&name[0..2]).join(name),
(tpe, Some(name)) => self.path.join(path).join(tpe).join(name),
(path, None) => self.path.join(path),
}
}

async fn open_file(&self, path: &Path, tpe: &str, name: &str) -> Result<File> {
let file_path = self.filename(path, tpe, name);
let file_path = self.filename(path, tpe, Some(name));
Ok(File::open(file_path)
.await
.map_err(|err| ErrorKind::OpeningFileFailed(format!("Could not open file: {}", err)))?)
}

async fn create_file(&self, path: &Path, tpe: &str, name: &str) -> Result<WriteOrDeleteFile> {
let file_path = self.filename(path, tpe, name);
let file_path = self.filename(path, tpe, Some(name));
WriteOrDeleteFile::new(file_path).await
}

fn remove_file(&self, path: &Path, tpe: &str, name: &str) -> Result<()> {
fn remove_file(&self, path: &Path, tpe: &str, name: Option<&str>) -> Result<()> {
let file_path = self.filename(path, tpe, name);
fs::remove_file(file_path)
.map_err(|err| ErrorKind::RemovingFileFailed(format!("Could not remove file: {err}")))
Expand Down

0 comments on commit 2088ac7

Please sign in to comment.