Hirevo / alexandrie

An alternative crate registry, implemented in Rust.
https://hirevo.github.io/alexandrie/
Apache License 2.0
491 stars 55 forks source link

Revision of the `Indexer` trait #60

Open Hirevo opened 4 years ago

Hirevo commented 4 years ago

The Indexer trait, in its current state has a number of problems that needlessly restricts what its implementors can do.

Here is a initial and non-exhaustive list of problems we might want to solve:

  1. Missing &mut self:
    This forces implementors to resort to interior mutability to workaround this constraint and it should be the responsibility of the registry itself to take care of the synchronization part (that it should be doing anyway, to avoid concurrent and racy crate publishes), so we should just allow them to take an &mut self and open the doors for fancier index managers.
  2. Error handling:
    The return types of the current trait are all just Result<_, Error> (_ being the type of successful value).
    So, in the case of fn latest_record(&self, name: &str) -> Result<CrateVersion, Error>, for instance, distinguishing between whether the crate was not found (something that can happen and not a real failure of the system) or something more critical is harder than needed (we must match and cannot just use ? to handle the critical ones).
    Taking inspiration of the error-handling design of Crate-Index, we could change some of these return type into Result<Option<_>, Error> or something like Result<Result<_, RetrievalError>, Error>.
    This way, the ? operator could take care of the outer, more critical, error and we can easily know if the lookup found anything or not.

Feel free to propose designs or share another problem that you think should be addressed by the new revision.

Hirevo commented 4 years ago

Here is an initial draft for a revision (just attempting to address the points above, with no real semantic change to the API):

/// The required trait that any crate index management type must implement.
pub trait Indexer {
    /// Gives back the URL of the managed crate index.
    fn url(&self) -> Result<String, Error>;

    /// Refreshes the managed crate index (in case another instance made modification to it).
    fn refresh(&mut self) -> Result<(), Error>;

    /// Retrieves all the version records of a crate.
    fn all_records(&self, name: &str) -> Result<Option<Vec<CrateVersion>>, Error>;

    /// Retrieves the latest version record of a crate.
    fn latest_record(&self, name: &str) -> Result<Option<CrateVersion>, Error>;

    /// Retrieves the latest crate version record that matches the given name and version requirement.
    fn match_record(&self, name: &str, req: VersionReq) -> Result<Option<CrateVersion>, Error>;

    /// Commits and pushes changes upstream.
    fn commit_and_push(&self, msg: &str) -> Result<(), Error>;

    /// Adds a new crate record into the index.
    fn insert_record(&mut self, record: CrateVersion) -> Result<(), Error>;

    /// Alters an index's crate version record with the passed-in function.
    fn alter_record<F>(&mut self, name: &str, version: Version, func: F) -> Result<Option<CrateVersion>, Error>
    where
        F: FnOnce(&mut CrateVersion);

    /// Yanks a crate version.
    fn yank_record(&mut self, name: &str, version: Version) -> Result<Option<()>, Error>;

    /// Un-yanks a crate version.
    fn unyank_record(&mut self, name: &str, version: Version) -> Result<Option<()>, Error>;
}
Hirevo commented 4 years ago

Another interesting question:
Currently, the trait is not object-safe due the generics used in fn alter_records(...). Are we interested in making the new revised trait object-safe ? (aka. do we intend to ever use a dyn Indexer ?)