fix(github): avoid caching empty release assets

src/cache.rs

@@ -163,6 +163,53 @@ where
         Ok(val)
     }
 
+    /// Like [`Self::get_or_try_init_async`], but values rejected by `should_cache`
+    /// are returned without populating the in-memory or on-disk cache.
+    pub async fn get_or_try_init_async_if<F, Fut, P>(&self, fetch: F, should_cache: P) -> Result<T>
+    where
+        F: FnOnce() -> Fut,
+        Fut: Future<Output = Result<T>>,
+        P: Fn(&T) -> bool,
+        T: Clone,
+    {
+        if let Some(val) = self.cache_async.get()
+            && should_cache(val)
+        {
+            return Ok(val.clone());
+        }
+        if let Some(val) = self.cache.get()
+            && should_cache(val)
+        {
+            return Ok(val.clone());
+        }
+
+        let path = &self.cache_file_path;
+        if self.is_fresh() {
+            match self.parse() {
+                Ok(val) => {
+                    if should_cache(&val) {
+                        let _ = self.cache.set(val.clone());
+                        let _ = self.cache_async.set(val.clone());
+                        return Ok(val);
+                    }
+                }
+                Err(err) => {
+                    warn!("failed to parse cache file: {} {:#}", path.display(), err);
+                }
+            }
+        }
+
+        let val = fetch().await?;
+        if should_cache(&val) {
+            if let Err(err) = self.write(&val) {
+                warn!("failed to write cache file: {} {:#}", path.display(), err);
+            }
+            let _ = self.cache.set(val.clone());
+            let _ = self.cache_async.set(val.clone());
+        }
+        Ok(val)
+    }
+
     /// Fetch fresh data, write it to disk, and return it without consulting
     /// any cache. The in-memory cache cells are replaced with the fresh value
     /// so future non-refresh reads observe it instead of a stale previously-
@@ -399,4 +446,30 @@ mod tests {
         let val = cache.get_or_try_init(|| Ok(4)).unwrap();
         assert_eq!(val, &2);
     }
+
+    #[tokio::test]
+    async fn test_get_or_try_init_async_if_does_not_cache_rejected_values() {
+        let _config = Config::get().await.unwrap();
+        let mut cache: CacheManager<i32> =
+            CacheManagerBuilder::new(dirs::CACHE.join("test-cache-if")).build();
+        cache.clear().unwrap();
+
+        let val = cache
+            .get_or_try_init_async_if(|| async { Ok(1) }, |v| *v > 1)
+            .await
+            .unwrap();
+        assert_eq!(val, 1);
+
+        let val = cache
+            .get_or_try_init_async_if(|| async { Ok(2) }, |v| *v > 1)
+            .await
+            .unwrap();
+        assert_eq!(val, 2);
+
+        let val = cache
+            .get_or_try_init_async_if(|| async { Ok(3) }, |v| *v > 1)
+            .await
+            .unwrap();
+        assert_eq!(val, 2);
+    }
 }

src/github.rs

@@ -287,19 +287,27 @@ pub async fn get_release(repo: &str, tag: &str) -> Result<GithubRelease> {
     let cache = get_release_cache(&key).await;
     let cache = cache.get(&key).unwrap();
     Ok(cache
-        .get_or_try_init_async(async || get_release_(API_URL, repo, tag).await)
-        .await?
-        .clone())
+        .get_or_try_init_async_if(
+            async || get_release_(API_URL, repo, tag).await,
+            should_cache_release,
+        )
+        .await?)
 }
 
 pub async fn get_release_for_url(api_url: &str, repo: &str, tag: &str) -> Result<GithubRelease> {
     let key = format!("{api_url}-{repo}-{tag}").to_kebab_case();
     let cache = get_release_cache(&key).await;
     let cache = cache.get(&key).unwrap();
     Ok(cache
-        .get_or_try_init_async(async || get_release_(api_url, repo, tag).await)
-        .await?
-        .clone())
+        .get_or_try_init_async_if(
+            async || get_release_(api_url, repo, tag).await,
+            should_cache_release,
+        )
+        .await?)
+}
+
+fn should_cache_release(release: &GithubRelease) -> bool {
+    !release.assets.is_empty()
 }
 
 /// Find the latest build revision for a version in a GitHub repo.
@@ -864,4 +872,65 @@ something_else = "value"
         let best = pick_best_build_revision(releases, "3.3.11").unwrap();
         assert_eq!(best.tag_name, "3.3.11-1");
     }
+
+    fn make_asset(name: &str) -> GithubAsset {
+        GithubAsset {
+            name: name.to_string(),
+            browser_download_url: format!("https://github.com/owner/repo/releases/download/{name}"),
+            url: format!("https://api.github.com/repos/owner/repo/releases/assets/{name}"),
+            digest: None,
+        }
+    }
+
+    #[tokio::test]
+    async fn test_empty_release_assets_are_not_cached() {
+        let _config = crate::config::Config::get().await.unwrap();
+        let mut server = mockito::Server::new_async().await;
+        let repo = "owner/empty-assets-cache-test";
+        let tag = "v1.0.0";
+        let path = format!("/repos/{repo}/releases/tags/{tag}");
+        let key = format!("{}-{repo}-{tag}", server.url()).to_kebab_case();
+
+        let cached_empty_release = make_release(tag);
+        {
+            let cache_group = get_release_cache(&key).await;
+            let cache = cache_group.get(&key).unwrap();
+            cache.write(&cached_empty_release).unwrap();
+        }
+
+        let empty_mock = server
+            .mock("GET", path.as_str())
+            .with_status(200)
+            .with_header("content-type", "application/json")
+            .with_body(serde_json::to_string(&cached_empty_release).unwrap())
+            .expect(1)
+            .create_async()
+            .await;
+
+        let release = get_release_for_url(&server.url(), repo, tag).await.unwrap();
+        assert!(release.assets.is_empty());
+        empty_mock.assert_async().await;
+        empty_mock.remove_async().await;
+
+        let populated_release = GithubRelease {
+            assets: vec![make_asset("tool-v1.0.0-linux-x86_64.tar.gz")],
+            ..make_release(tag)
+        };
+        let mock = server
+            .mock("GET", path.as_str())
+            .with_status(200)
+            .with_header("content-type", "application/json")
+            .with_body(serde_json::to_string(&populated_release).unwrap())
+            .expect(1)
+            .create_async()
+            .await;
+
+        let release = get_release_for_url(&server.url(), repo, tag).await.unwrap();
+        assert_eq!(release.assets.len(), 1);
+        assert_eq!(release.assets[0].name, "tool-v1.0.0-linux-x86_64.tar.gz");
+
+        let release = get_release_for_url(&server.url(), repo, tag).await.unwrap();
+        assert_eq!(release.assets.len(), 1);
+        mock.assert_async().await;
+    }
 }