Do not install latest plugin if incompatible

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

crates/plugins/Cargo.toml

@@ -7,14 +7,14 @@ edition = { workspace = true }
 [dependencies]
 anyhow = "1.0"
 bytes = "1.1"
-chrono = "0.4"
+chrono = { version = "0.4", features = ["serde"] }
 dirs = "4.0"
 fd-lock = "3.0.12"
 flate2 = { version = "1.0.17", features = ["zlib-ng"], default-features = false }
 is-terminal = "0.4"
 path-absolutize = "3.0.11"
 reqwest = { version = "0.11", features = ["json"] }
-semver = "1.0"
+semver = { version = "1.0", features = ["serde"] }
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
 spin-common = { path = "../common" }
@@ -24,4 +24,4 @@ terminal = { path = "../terminal" }
 thiserror = "1"
 tokio = { version = "1.23", features = [ "fs", "process", "rt", "macros" ] }
 tracing = { workspace = true }
-url = "2.2.2"
+url = { version = "2.2.2", features = ["serde"] }

crates/plugins/src/badger/mod.rs

@@ -170,7 +170,7 @@ impl BadgerEvaluator {
         let latest_version = {
             let latest_lookup = crate::lookup::PluginLookup::new(&self.plugin_name, None);
             let latest_manifest = latest_lookup
-                .get_manifest_from_repository(store.get_plugins_directory())
+                .resolve_manifest_exact(store.get_plugins_directory())
                 .await
                 .ok();
             latest_manifest.and_then(|m| semver::Version::parse(m.version()).ok())

crates/plugins/src/error.rs

@@ -14,6 +14,9 @@ pub enum Error {
 
     #[error("URL parse error {0}")]
     UrlParseError(#[from] url::ParseError),
+
+    #[error("{0}")]
+    Other(#[from] anyhow::Error),
 }
 
 /// Contains error details for when a plugin resource cannot be found at expected location

crates/plugins/src/lookup.rs

@@ -30,7 +30,34 @@ impl PluginLookup {
         }
     }
 
-    pub async fn get_manifest_from_repository(
+    pub async fn resolve_manifest(
+        &self,
+        plugins_dir: &Path,
+        skip_compatibility_check: bool,
+        spin_version: &str,
+    ) -> PluginLookupResult<PluginManifest> {
+        let exact = self.resolve_manifest_exact(plugins_dir).await?;
+        if skip_compatibility_check
+            || self.version.is_some()
+            || exact.is_compatible_spin_version(spin_version)
+        {
+            return Ok(exact);
+        }
+
+        let store = crate::store::PluginStore::new(plugins_dir.to_owned());
+
+        // TODO: This is very similar to some logic in the badger module - look for consolidation opportunities.
+        let manifests = store.catalogue_manifests()?;
+        let relevant_manifests = manifests.into_iter().filter(|m| m.name() == self.name);
+        let compatible_manifests = relevant_manifests
+            .filter(|m| m.has_compatible_package() && m.is_compatible_spin_version(spin_version));
+        let highest_compatible_manifest =
+            compatible_manifests.max_by_key(|m| m.try_version().unwrap_or_else(|_| null_version()));
+
+        Ok(highest_compatible_manifest.unwrap_or(exact))
+    }
+
+    pub async fn resolve_manifest_exact(
         &self,
         plugins_dir: &Path,
     ) -> PluginLookupResult<PluginManifest> {
@@ -41,37 +68,73 @@ impl PluginLookup {
             .map_err(|e| {
                 Error::ConnectionFailed(ConnectionFailedError::new(url.to_string(), e.to_string()))
             })?;
+
+        self.resolve_manifest_exact_from_good_repo(plugins_dir)
+    }
+
+    // This is split from resolve_manifest_exact because it may recurse (once) and that makes
+    // Rust async sad. So we move the potential recursion to a sync helper.
+    #[allow(clippy::let_and_return)]
+    pub fn resolve_manifest_exact_from_good_repo(
+        &self,
+        plugins_dir: &Path,
+    ) -> PluginLookupResult<PluginManifest> {
         let expected_path = spin_plugins_repo_manifest_path(&self.name, &self.version, plugins_dir);
-        let file = File::open(&expected_path).map_err(|e| {
-            Error::NotFound(NotFoundError::new(
-                Some(self.name.clone()),
-                expected_path.display().to_string(),
-                e.to_string(),
-            ))
-        })?;
-        let manifest: PluginManifest = serde_json::from_reader(file).map_err(|e| {
-            Error::InvalidManifest(InvalidManifestError::new(
+
+        let not_found = |e: std::io::Error| {
+            Err(Error::NotFound(NotFoundError::new(
                 Some(self.name.clone()),
                 expected_path.display().to_string(),
                 e.to_string(),
-            ))
-        })?;
-        Ok(manifest)
+            )))
+        };
+
+        let manifest = match File::open(&expected_path) {
+            Ok(file) => serde_json::from_reader(file).map_err(|e| {
+                Error::InvalidManifest(InvalidManifestError::new(
+                    Some(self.name.clone()),
+                    expected_path.display().to_string(),
+                    e.to_string(),
+                ))
+            }),
+            Err(e) if e.kind() == std::io::ErrorKind::NotFound && self.version.is_some() => {
+                // If a user has asked for a version by number, and the path doesn't exist,
+                // it _might_ be because it's the latest version. This checks for that case.
+                let latest = Self::new(&self.name, None);
+                match latest.resolve_manifest_exact_from_good_repo(plugins_dir) {
+                    Ok(manifest) if manifest.try_version().ok() == self.version => Ok(manifest),
+                    _ => not_found(e),
+                }
+            }
+            Err(e) => not_found(e),
+        };
+
+        manifest
     }
 }
 
 pub fn plugins_repo_url() -> Result<Url, url::ParseError> {
     Url::parse(SPIN_PLUGINS_REPO)
 }
 
+#[cfg(not(test))]
+fn accept_as_repo(git_root: &Path) -> bool {
+    git_root.join(".git").exists()
+}
+
+#[cfg(test)]
+fn accept_as_repo(git_root: &Path) -> bool {
+    git_root.join(".git").exists() || git_root.join("_spin_test_dot_git").exists()
+}
+
 pub async fn fetch_plugins_repo(
     repo_url: &Url,
     plugins_dir: &Path,
     update: bool,
 ) -> anyhow::Result<()> {
     let git_root = plugin_manifests_repo_path(plugins_dir);
     let git_source = GitSource::new(repo_url, None, &git_root);
-    if git_root.join(".git").exists() {
+    if accept_as_repo(&git_root) {
         if update {
             git_source.pull().await?;
         }
@@ -110,3 +173,83 @@ pub fn spin_plugins_repo_manifest_dir(plugins_dir: &Path) -> PathBuf {
         .join(PLUGINS_REPO_LOCAL_DIRECTORY)
         .join(PLUGINS_REPO_MANIFESTS_DIRECTORY)
 }
+
+fn null_version() -> semver::Version {
+    semver::Version::new(0, 0, 0)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    const TEST_NAME: &str = "some-spin-ver-some-not";
+    const TESTS_STORE_DIR: &str = "tests";
+
+    fn tests_store_dir() -> PathBuf {
+        PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(TESTS_STORE_DIR)
+    }
+
+    #[tokio::test]
+    async fn if_no_version_given_and_latest_is_compatible_then_latest() -> PluginLookupResult<()> {
+        let lookup = PluginLookup::new(TEST_NAME, None);
+        let resolved = lookup
+            .resolve_manifest(&tests_store_dir(), false, "99.0.0")
+            .await?;
+        assert_eq!("99.0.1", resolved.version);
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn if_no_version_given_and_latest_is_not_compatible_then_highest_compatible(
+    ) -> PluginLookupResult<()> {
+        // NOTE: The setup assumes you are NOT running Windows on aarch64, so as to check 98.1.0 is not
+        // offered. If that assumption fails then this test will fail with actual version being 98.1.0.
+        // (We use this combination because the OS and architecture enums don't allow for fake operating systems!)
+        let lookup = PluginLookup::new(TEST_NAME, None);
+        let resolved = lookup
+            .resolve_manifest(&tests_store_dir(), false, "98.0.0")
+            .await?;
+        assert_eq!("98.0.0", resolved.version);
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn if_version_given_it_gets_used_regardless() -> PluginLookupResult<()> {
+        let lookup = PluginLookup::new(TEST_NAME, Some(semver::Version::parse("99.0.0").unwrap()));
+        let resolved = lookup
+            .resolve_manifest(&tests_store_dir(), false, "98.0.0")
+            .await?;
+        assert_eq!("99.0.0", resolved.version);
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn if_latest_version_given_it_gets_used_regardless() -> PluginLookupResult<()> {
+        let lookup = PluginLookup::new(TEST_NAME, Some(semver::Version::parse("99.0.1").unwrap()));
+        let resolved = lookup
+            .resolve_manifest(&tests_store_dir(), false, "98.0.0")
+            .await?;
+        assert_eq!("99.0.1", resolved.version);
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn if_no_version_given_but_skip_compat_then_highest() -> PluginLookupResult<()> {
+        let lookup = PluginLookup::new(TEST_NAME, None);
+        let resolved = lookup
+            .resolve_manifest(&tests_store_dir(), true, "98.0.0")
+            .await?;
+        assert_eq!("99.0.1", resolved.version);
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn if_non_existent_version_given_then_error() -> PluginLookupResult<()> {
+        let lookup = PluginLookup::new(TEST_NAME, Some(semver::Version::parse("177.7.7").unwrap()));
+        lookup
+            .resolve_manifest(&tests_store_dir(), true, "99.0.0")
+            .await
+            .expect_err("Should have errored because plugin v177.7.7 does not exist");
+        Ok(())
+    }
+}

crates/plugins/src/manager.rs

@@ -184,6 +184,8 @@ impl PluginManager {
     pub async fn get_manifest(
         &self,
         manifest_location: &ManifestLocation,
+        skip_compatibility_check: bool,
+        spin_version: &str,
     ) -> PluginLookupResult<PluginManifest> {
         let plugin_manifest = match manifest_location {
             ManifestLocation::Remote(url) => {
@@ -232,7 +234,11 @@ impl PluginManager {
             }
             ManifestLocation::PluginsRepository(lookup) => {
                 lookup
-                    .get_manifest_from_repository(self.store().get_plugins_directory())
+                    .resolve_manifest(
+                        self.store().get_plugins_directory(),
+                        skip_compatibility_check,
+                        spin_version,
+                    )
                     .await?
             }
         };

crates/plugins/src/manifest.rs

@@ -64,6 +64,10 @@ impl PluginManifest {
             Err(_) => false,
         }
     }
+
+    pub fn try_version(&self) -> Result<semver::Version, semver::Error> {
+        semver::Version::parse(&self.version)
+    }
 }
 
 /// Describes compatibility and location of a plugin source.

crates/plugins/tests/.spin-plugins/_spin_test_dot_git

@@ -0,0 +1 @@
+Fake file for preventing the plugin system from pulling from the production repo into this directory during tests

crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not.json

@@ -0,0 +1,27 @@
+{
+    "name": "some-spin-ver-some-not",
+    "description": "A plugin where only some versions work with the test harness version of Spin.",
+    "version": "99.0.1",
+    "spinCompatibility": ">=99.0",
+    "license": "Apache-2.0",
+    "packages": [
+        {
+            "os": "linux",
+            "arch": "amd64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        },
+        {
+            "os": "macos",
+            "arch": "aarch64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        },
+        {
+            "os": "macos",
+            "arch": "amd64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        }
+    ]
+}

crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json

@@ -0,0 +1,27 @@
+{
+    "name": "some-spin-ver-some-not",
+    "description": "A plugin where only some versions work with the test harness version of Spin.",
+    "version": "98.0.0",
+    "spinCompatibility": ">=98.0",
+    "license": "Apache-2.0",
+    "packages": [
+        {
+            "os": "linux",
+            "arch": "amd64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        },
+        {
+            "os": "macos",
+            "arch": "aarch64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        },
+        {
+            "os": "macos",
+            "arch": "amd64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        }
+    ]
+}

crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json

@@ -0,0 +1,15 @@
+{
+    "name": "some-spin-ver-some-not",
+    "description": "A plugin where only some versions work with the test harness version of Spin.",
+    "version": "98.1.0",
+    "spinCompatibility": ">=98.0",
+    "license": "Apache-2.0",
+    "packages": [
+        {
+            "os": "windows",
+            "arch": "aarch64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        }
+    ]
+}

crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json

@@ -0,0 +1,27 @@
+{
+    "name": "some-spin-ver-some-not",
+    "description": "A plugin where only some versions work with the test harness version of Spin.",
+    "version": "99.0.0",
+    "spinCompatibility": ">=99.0",
+    "license": "Apache-2.0",
+    "packages": [
+        {
+            "os": "linux",
+            "arch": "amd64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        },
+        {
+            "os": "macos",
+            "arch": "aarch64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        },
+        {
+            "os": "macos",
+            "arch": "amd64",
+            "url": "https://example.com/doesnt-exist",
+            "sha256": "11111111"
+        }
+    ]
+}