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

Inability to fully specify type of Repository #227

Closed
TilBlechschmidt opened this issue Jun 8, 2024 · 5 comments
Closed

Inability to fully specify type of Repository #227

TilBlechschmidt opened this issue Jun 8, 2024 · 5 comments
Labels
A-api Area: Related to API design of rustic_core A-dx Area: Related to Developer Experience C-bug Category: Something isn't working as expected

Comments

@TilBlechschmidt
Copy link
Contributor

Hey there 👋

I was trying to use rustic_core to read files from an on-disk backup and wanted to dynamically create and cache repositories (i.e. store them in a collection). However, while designing my internal API, I ran into the issue that the type of an opened and fully indexed Repository can not be fully specified:

use rustic_core::{
    repository::{FullIndex, IndexedStatus}, // <-- ERROR: These are inaccessible
    NoProgressBars, OpenStatus
};

type RusticRepository = rustic_core::Repository<NoProgressBars, IndexedStatus<FullIndex, OpenStatus>>;

The problem is that while the repository::IndexedFull trait is re-exported, the concrete types repository::IndexedStatus and repository::FullIndex are not.

When directly using the return value of Repository::new(...).open().to_indexed() this does not matter, since the methods can be called regardless. Returning this value from a function is also possible using impl IndexedFull. However, when for example constructing a wrapping struct like so:

pub struct RepositoryWrapper(Repository<NoProgressBars, _>);

It becomes impossible to define the type since the previously mentioned concrete types are not public. It is possible to work around this by using generics and the IndexedFull trait.

However, this forces one to push the creation of the repository far up the call stack (i.e. passing it in as a parameter in the code below) and litters the entire codebase with generics that will only ever have one representation. It also becomes impossible to dynamically create and store Repositories in a collection.

pub struct RepositoryWrapper<S>(Repository<NoProgressBars, S>);

impl<S> RepositoryWrapper<S> {
    pub fn new() -> Self<S> {
        // ERROR: S does not match the concrete but opaque type returned by this since `S` needs to be specified by the callee!
        Self(Repository::new(...).open().to_indexed())
    }
}

I also tried using dynamic dispatch instead by wrapping the repository in a Box or Arc and using dyn IndexedFull. However, this runs into the same issue where the associated type parameter I needs to be specified which resolves to index::GlobalIndex which unfortunately is also private.


My suggestion would be to re-export the two types FullIndex and IndexedStatus in lib.rs#L151 as follows:

pub use crate::{
    ...
-   repository::{IndexedFull, OpenStatus, Repository, RepositoryOptions},
+   repository::{IndexedFull, OpenStatus, Repository, RepositoryOptions, FullIndex, IndexedStatus},
}

There might be a way to put dynamically created Repositories into a collection through some type magic, though I could not find one 🤷 😢

@github-actions github-actions bot added the S-triage Status: Waiting for a maintainer to triage this issue/PR label Jun 8, 2024
@TilBlechschmidt
Copy link
Contributor Author

In case the suggested change is acceptable, I've created TilBlechschmidt@7467cd7 where I tested whether it works (it does) and based on which I could create a PR 🙂

@aawsome
Copy link
Member

aawsome commented Jun 10, 2024

Thanks for opening this issue @TilBlechschmidt!
Yes, please create a PR for this! And maybe you might even want to add an integration test to ensure that your type of setting really works (and keeps working with future changes).

Note that the Repository type might change in future as we are not fully settled how to best model it. But anyway that would be a breaking change and hopefully not much work for you in an dependency update.

@aawsome aawsome added C-bug Category: Something isn't working as expected A-api Area: Related to API design of rustic_core A-dx Area: Related to Developer Experience and removed S-triage Status: Waiting for a maintainer to triage this issue/PR labels Jun 10, 2024
@TilBlechschmidt
Copy link
Contributor Author

TilBlechschmidt commented Jun 10, 2024

Hi @aawsome!

Will do. I had the same thought about the integration test but did not have the time to look into that the other day, a good idea for sure though. I'll put that on my list and send a PR your way soon™

Future breaking changes are totally fine, I am using a very small set of API functions anyway. Honestly, opening up these types muddies the current abstraction a little so it makes total sense that there are some changes in the pipeline 🤷‍♂️

Either way, thanks for working on and maintaining rustic! It really simplified my current project which is a website to browse+extract files/directories from a restic backup (in my case hosted off-site) and either download or share a temporary link to them 😊

@TilBlechschmidt
Copy link
Contributor Author

TilBlechschmidt commented Jun 14, 2024

I've opened a PR which includes an integration test, let me know if there is anything else 🙂

FYI It seems that the snapshot tests are semi-broken on macOS due to some OS-specific file attribute (related to sandboxing) that gets added to the restored files. I did run my test in isolation and it works just fine, so we will have to see what the pipeline has to say about that 🤷

┈┈┈┈┈┈┈┈┈┈┈┈┼┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
   48    60 │       "inode": "[inode]",
   49    61 │       "device_id": "[device_id]",
   50    62 │       "size": 21,
   51    63 │       "links": 2,
         64 │+      "extended_attributes": [
         65 │+        ExtendedAttribute(
         66 │+          name: "com.apple.provenance",
         67 │+          value: "AQAAiAZZUZMQbLQ=",
         68 │+        ),
         69 │+      ],
   52    70 │       "content": Some([
   53    71 │         Id("649b8b471e7d7bc175eec758a7006ac693c434c8297c07db15286788c837154a"),
   54    72 │       ]),
   55    73 │     },
┈┈┈┈┈┈┈┈┈┈┈┈┼┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

aawsome pushed a commit that referenced this issue Jun 17, 2024
See #227 for more details.

I've added a test-case which constructs a minimal wrapper type around a
Repository that implements `IndexedFull`, puts two instances into a
collection (the primary motivator behind this), and hands them to a
function that relies on the trait.
@aawsome
Copy link
Member

aawsome commented Jun 17, 2024

fixed in #229

@aawsome aawsome closed this as completed Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-api Area: Related to API design of rustic_core A-dx Area: Related to Developer Experience C-bug Category: Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

2 participants