refactor(error)!: handle errors more gracefully in rustic_core. #202

simonsan · 2024-03-13T10:21:32Z

Till now, rustic_core had a lot of unwraps and expects sprinkled over the code base. This made rustic_core and dependents panic in case of errors. This is unacceptable for a library, hence we fixed it now.

Fixes #140

TODO:

Implement unit tests for test coverage of some changes
Fix integration test
0 (*nix) errors left (due to unwrap/expect being denied)

Possible Panics left over/reintroduced:

HexId::as_str() / Id:to_hex(): Commit cc6430f4

codecov · 2024-03-13T11:32:24Z

Codecov Report

Attention: Patch coverage is 26.00000% with 444 lines in your changes are missing coverage. Please review.

Project coverage is 27.5%. Comparing base (fb291dc) to head (f1218c8).

Additional details and impacted files

Files	Coverage Δ
crates/backend/src/choose.rs	`0.0% <ø> (ø)`
crates/backend/src/opendal.rs	`0.0% <ø> (ø)`
crates/backend/src/rclone.rs	`87.5% <ø> (+9.7%)`	⬆️
crates/core/src/archiver/file_archiver.rs	`77.0% <ø> (ø)`
crates/core/src/cdc/polynom.rs	`75.0% <ø> (ø)`
crates/core/src/cdc/rolling_hash.rs	`54.3% <ø> (ø)`
crates/core/src/chunker.rs	`58.6% <ø> (+0.7%)`	⬆️
crates/core/src/crypto/aespoly1305.rs	`77.7% <ø> (-5.6%)`	⬇️
crates/core/src/error.rs	`25.0% <ø> (+10.7%)`	⬆️
crates/core/src/index/binarysorted.rs	`54.5% <ø> (-0.9%)`	⬇️
... and 36 more

... and 7 files with indirect coverage changes

simonsan

Some notes where help would be appreciated

crates/core/src/backend/local_destination.rs

crates/core/src/blob/packer.rs

crates/core/src/commands/backup.rs

simonsan · 2024-03-15T01:44:22Z

crates/core/src/commands/check.rs

+            // REVIEW: Behaviour could have changed here, check!
            index_be
-                .into_index()
+                .into_index()?
                .into_iter()
                .par_bridge()
-                .for_each(|pack| {
+                .map(|pack| -> RusticResult<()> {
                    let id = pack.id;
-                    let data = be.read_full(FileType::Pack, &id).unwrap();
-                    match check_pack(be, pack, data, &p) {
-                        Ok(()) => {}
-                        Err(err) => error!("Error reading pack {id} : {err}",),
-                    }
-                });
+                    let data = be
+                        .read_full(FileType::Pack, &id)
+                        .map_err(RusticErrorKind::Backend)?;
+
+                    check_pack(be, pack, data, &p)
+                })
+                .collect::<RusticResult<()>>()?;
+


due to replacing for_each with map (to be able to use results) behaviour could have changed, check again

crates/core/src/commands/forget.rs

crates/core/src/id.rs

crates/core/src/vfs.rs

simonsan · 2024-03-15T02:29:09Z

The CI fails for Test Coverage, I don't intend to write tests at this stage for the changes as I think we should focus on the other tests first, that we wanted to implement (e.g. path related stuff).

I don't want to implement these tests in this PR though, as it's already quite big.

simonsan · 2024-03-15T07:23:07Z

crates/core/src/blob/packer.rs

+    #[rstest]
+    #[case(0.5f32, false, "size is too small, should be 'false'")]
+    #[case(1.1f32, true, "size is ok, should be 'true'")]
+    #[case(1_000_000.0f32, false, "size is too huge: should be 'false'")]
+    #[allow(clippy::cast_possible_truncation)]
+    fn test_compute_pack_size_ok_passes(
+        pack_sizer: PackSizer,
+        #[case] input: f32,
+        #[case] expected: bool,
+        #[case] comment: &str,
+    ) -> RusticResult<()> {
+        let size_limit = pack_sizer.pack_size()? * 30 / 100;
+
+        let size = (input * size_limit as f32) as u32;
+
+        assert_eq!(pack_sizer.size_ok(size)?, expected, "{comment}");
+
+        Ok(())
+    }


Can you think of a better way to test the PackSizer::size_ok() + PackSizer::add_size()?

simonsan · 2024-03-15T13:02:31Z

OK, added some more testing, so the coverage test also runs through. 👍🏽

crates/core/src/id.rs

crates/backend/src/choose.rs

crates/backend/src/opendal.rs

crates/backend/src/local.rs

crates/backend/src/rclone.rs

aawsome · 2024-03-15T22:13:12Z

crates/backend/src/rest.rs

+    type Error;
+
+    /// Check a response for an error and treat them as permanent or transient
+    fn check_error(self) -> Result<Self, Self::Error>
+    where
+        Self: Sized;
 }

 impl CheckError for Response {
+    type Error = backoff::Error<reqwest::Error>;
+


What does this improve?

The latest version improves the name (imho) and this makes it possible to use it further down the line with other types. Also, people can use it easier for their own implementation, e.g. in case they want to use another library at a certain point in time handling the error of a response and backing off. So I like the idea of having a more generalized trait for that.

/// `ValidateResponse` to add user-defined method `validate` on a response /// /// This trait is used to add a method `validate` on a response to check for errors /// and treat them as permanent or transient based on the status code of the response. /// /// It returns a result with the response if it is not an error, otherwise it returns /// the associated error. pub trait ValidateResponse { /// The error type that is returned if the response is an error type Error; /// Check a response for an error and treat it as permanent or transient /// /// # Errors /// /// If the response is an error, it will return an error of type [`Self::Error`] /// /// # Returns /// /// The response if it is not an error or an error of type [`Self::Error`] if it is an error fn validate(self) -> Result<Self, Self::Error> where Self: Sized; }

crates/core/src/archiver.rs

crates/core/src/backend/cache.rs

aawsome · 2024-03-16T05:59:20Z

crates/core/src/archiver/parent.rs

@@ -296,3 +332,57 @@ impl Parent {
        Ok(result)
    }
 }
+


internal remark: Review until here.

Signed-off-by: simonsan <[email protected]>

…r the code base, also handle cli changes with alias Signed-off-by: simonsan <[email protected]>

Signed-off-by: simonsan <[email protected]>

…code Signed-off-by: simonsan <[email protected]>

Signed-off-by: simonsan <[email protected]>

…ar and keep good style Signed-off-by: simonsan <[email protected]>

Signed-off-by: simonsan <[email protected]>

…em and otherwise it can be confused with a pure getter Signed-off-by: simonsan <[email protected]>

Signed-off-by: simonsan <[email protected]>

…leanup, add cfg_if for tests and os dependent stuff Signed-off-by: simonsan <[email protected]>

Signed-off-by: simonsan <[email protected]>

simonsan added A-errors Area: error handling needs improvement C-enhancement Category: New feature or request C-refactor Category: Refactoring of already existing code labels Mar 13, 2024

simonsan marked this pull request as draft March 13, 2024 10:21

simonsan marked this pull request as ready for review March 14, 2024 14:41

simonsan requested a review from aawsome March 14, 2024 14:41

simonsan commented Mar 15, 2024

View reviewed changes

simonsan mentioned this pull request Mar 15, 2024

fix: Add warning about unsorted files and sort where neccessary #205

Merged

simonsan commented Mar 15, 2024

View reviewed changes

crates/core/src/id.rs Outdated Show resolved Hide resolved

aawsome reviewed Mar 16, 2024

View reviewed changes

simonsan added 16 commits March 17, 2024 11:35

test: implement more tests and rename tests for consistency

31b1b9f

Signed-off-by: simonsan <[email protected]>

rename trait for checking errors in rest backend

0ef7c22

Signed-off-by: simonsan <[email protected]>

implement transpose on ParentResult and some error handling fixes

76f2bed

Signed-off-by: simonsan <[email protected]>

fix doc test and export for overwrite zero duration

39089b1

Signed-off-by: simonsan <[email protected]>

rework Id/HexId

5ba3c02

Signed-off-by: simonsan <[email protected]>

use parking lot

ad9203b

Signed-off-by: simonsan <[email protected]>

more errors for error handling

bce58de

Signed-off-by: simonsan <[email protected]>

rename filename to file_name (and dir_name) to make it consistent ove…

ccca56d

…r the code base, also handle cli changes with alias Signed-off-by: simonsan <[email protected]>

rename new_cache to from_backend to make source/intent clearer

f24105e

Signed-off-by: simonsan <[email protected]>

handle fallible location() method

0a62645

Signed-off-by: simonsan <[email protected]>

fix non-self-describing variable names that make it hard to read the …

c205d45

…code Signed-off-by: simonsan <[email protected]>

add more documentation about errors etc

1ae7a82

Signed-off-by: simonsan <[email protected]>

rename new_node to from_type_and_metadata to make the intent more cle…

7e0f0ca

…ar and keep good style Signed-off-by: simonsan <[email protected]>

impl Display for Cache

0128764

Signed-off-by: simonsan <[email protected]>

remove default impl from nodetype as it can be derived

896a14a

Signed-off-by: simonsan <[email protected]>

impl Debug for Packer

5910c3b

Signed-off-by: simonsan <[email protected]>

simonsan added 28 commits March 17, 2024 13:55

handle error in streamlist

7ea2e37

Signed-off-by: simonsan <[email protected]>

handle option in gid and uid from name

efd19a4

Signed-off-by: simonsan <[email protected]>

rename path to path of to make intent clear, because we pass in an it…

19516aa

…em and otherwise it can be confused with a pure getter Signed-off-by: simonsan <[email protected]>

use from Impl for readability

01db571

Signed-off-by: simonsan <[email protected]>

add serde feature for enum-map for insta tests, uncomment lints and c…

d1896bd

…leanup, add cfg_if for tests and os dependent stuff Signed-off-by: simonsan <[email protected]>

handle err in ignore + node

37e72be

Signed-off-by: simonsan <[email protected]>

differentiate between seen and successfully setting attribs

3b93861

Signed-off-by: simonsan <[email protected]>

handle err in stdin

c082ea1

Signed-off-by: simonsan <[email protected]>

handle errors in size calc in packer + deal with overflows

ab99051

Signed-off-by: simonsan <[email protected]>

multiprocessing errs

3bae8c5

Signed-off-by: simonsan <[email protected]>

multiprocessing errs

2e1d12d

Signed-off-by: simonsan <[email protected]>

parking lot rwlock

3e2e3fe

Signed-off-by: simonsan <[email protected]>

handle unwrap in repair cmd

34b55e4

Signed-off-by: simonsan <[email protected]>

error collector

5c55690

Signed-off-by: simonsan <[email protected]>

handle get_tree errs

49a08ef

Signed-off-by: simonsan <[email protected]>

handle unwrap in loop

5e57105

Signed-off-by: simonsan <[email protected]>

handle errs in forget

c283f0b

Signed-off-by: simonsan <[email protected]>

handle errs in prune

c1f95b2

Signed-off-by: simonsan <[email protected]>

collect errs in set_metadata

55df35f

Signed-off-by: simonsan <[email protected]>

refactor match to if

8e01f89

Signed-off-by: simonsan <[email protected]>

multiprocessing errs in restore

cc82a9e

Signed-off-by: simonsan <[email protected]>

handling other errs in restore

84b459c

Signed-off-by: simonsan <[email protected]>

multiprocessing errs in archiver

7db2396

Signed-off-by: simonsan <[email protected]>

results for fallible location

b56487d

Signed-off-by: simonsan <[email protected]>

doc changes

35d585a

Signed-off-by: simonsan <[email protected]>

make from_dir_node return option to bubble up subtree optional

9362ab6

Signed-off-by: simonsan <[email protected]>

handle errs in from_node

c87e9f6

Signed-off-by: simonsan <[email protected]>

handle errs in dir_entries_from_path

f1218c8

Signed-off-by: simonsan <[email protected]>

simonsan force-pushed the feat/remove-unwrap-expect branch from 92818c4 to f1218c8 Compare March 17, 2024 13:16

simonsan changed the title ~~feat(error)!: handle errors more gracefully in rustic_core.~~ refactor(error)!: handle errors more gracefully in rustic_core. May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(error)!: handle errors more gracefully in rustic_core. #202

refactor(error)!: handle errors more gracefully in rustic_core. #202

simonsan commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading

simonsan left a comment

simonsan Mar 15, 2024

simonsan commented Mar 15, 2024

simonsan Mar 15, 2024 •

edited

Loading

simonsan commented Mar 15, 2024

aawsome Mar 15, 2024

simonsan Mar 16, 2024 •

edited

Loading

simonsan Mar 16, 2024 •

edited

Loading

aawsome Mar 16, 2024

refactor(error)!: handle errors more gracefully in rustic_core. #202

Are you sure you want to change the base?

refactor(error)!: handle errors more gracefully in rustic_core. #202

Conversation

simonsan commented Mar 13, 2024 • edited Loading

codecov bot commented Mar 13, 2024 • edited Loading

Codecov Report

simonsan left a comment

Choose a reason for hiding this comment

simonsan Mar 15, 2024

Choose a reason for hiding this comment

simonsan commented Mar 15, 2024

simonsan Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

simonsan commented Mar 15, 2024

aawsome Mar 15, 2024

Choose a reason for hiding this comment

simonsan Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

simonsan Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

aawsome Mar 16, 2024

Choose a reason for hiding this comment

simonsan commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading

simonsan Mar 15, 2024 •

edited

Loading

simonsan Mar 16, 2024 •

edited

Loading

simonsan Mar 16, 2024 •

edited

Loading