Skip to content
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

refactor(error): improve error messages and file handling #334

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

simonsan
Copy link
Contributor

@simonsan simonsan commented Oct 14, 2024

Do not merge: will be rebased after #321 lands

  • Update error messages for file operations in the LocalBackendErrorKind enum.
  • Refactor the ReadBackend and WriteBackend implementations in the LocalBackend module to handle file opening errors more accurately.
  • Add error variants OpeningFileForPartialReadingFailed and OpeningFileForWritingFailed to provide specific information about file opening failures.
  • Create parent directory if it does not exist before opening the file for writing.

Fixes rustic-rs/rustic#1315
Fixes #310

- Update error messages for file operations in the `LocalBackendErrorKind` enum.
- Refactor the `ReadBackend` and `WriteBackend` implementations in the `LocalBackend` module to handle file opening errors more accurately.
- Add error variants `OpeningFileForPartialReadingFailed` and `OpeningFileForWritingFailed` to provide specific information about file opening failures.
- Create parent directory if it does not exist before opening the file for writing.

Fixes #rustic-rs/rustic#1315

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan simonsan added A-errors Area: error handling needs improvement C-refactor Category: Refactoring of already existing code labels Oct 14, 2024
@simonsan simonsan requested a review from aawsome October 14, 2024 16:03
@simonsan simonsan added the S-waiting-for-review Status: PRs waiting for review label Oct 14, 2024
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan
Copy link
Contributor Author

I kept:

        for i in 0u8..=255 {
            let filename = self.path.join("data").join(hex::encode([i]));
            fs::create_dir_all(&filename).map_err(|err| {
                LocalBackendErrorKind::DirectoryCreationFailed {
                    path: filename.clone(),
                    source: err,
                }
            })?;
        }

as that impacts compatibility with restic, and a repository without that, wouldn't be able to be read by it. Hence, I thought, we keep that, but we make sure, that also without these directories (in case someone deletes them), we are able to create and write to these directories. Not sure, how else we could handle that.

Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
I have just found a small thing I would change in this PR...

fn write_bytes(&self, tpe: FileType, id: &Id, _cacheable: bool, buf: Bytes) -> Result<()> {
trace!("writing tpe: {:?}, id: {}", &tpe, &id);
let filename = self.path(tpe, id);

// create parent directory if it does not exist
if let Some(parent) = filename.parent() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make a method parent_path and then just use self.parent_path(tpe,id) here.

Copy link
Contributor Author

@simonsan simonsan Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Not sure how we would handle a non-existing parent? e.g. in a root path, that should never happen right? Or do you have another idea?

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 46.5%. Comparing base (996a10f) to head (6dda9a7).

Files with missing lines Patch % Lines
crates/core/src/commands/check.rs 40.0% 3 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/backend/src/local.rs 0.0% <ø> (ø)
crates/core/src/commands/check.rs 66.0% <40.0%> (-0.7%) ⬇️

... and 5 files with indirect coverage changes

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan simonsan requested a review from aawsome October 16, 2024 13:30
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan simonsan added S-blocked Status: Blocked from merging/working on due to some issue and removed S-waiting-for-review Status: PRs waiting for review labels Oct 24, 2024
@simonsan simonsan marked this pull request as draft October 24, 2024 01:19
@simonsan simonsan added this to the NEXT milestone Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-errors Area: error handling needs improvement C-refactor Category: Refactoring of already existing code S-blocked Status: Blocked from merging/working on due to some issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we avoid creating empty folders when initializing a repo? Uncaught rclone errors causing rustic panic
2 participants