diff --git a/ci/Cargo.lock b/ci/Cargo.lock index b69c1deb6..dc2ece474 100644 --- a/ci/Cargo.lock +++ b/ci/Cargo.lock @@ -499,6 +499,9 @@ name = "github-repo" version = "0.1.0" dependencies = [ "anyhow", + "serde", + "serde-saphyr", + "serde_json", ] [[package]] diff --git a/ci/crates/github-repo/Cargo.toml b/ci/crates/github-repo/Cargo.toml index 908127508..8ed99d4c6 100644 --- a/ci/crates/github-repo/Cargo.toml +++ b/ci/crates/github-repo/Cargo.toml @@ -13,3 +13,8 @@ workspace = true [dependencies] anyhow.workspace = true +serde.workspace = true + +[dev-dependencies] +serde_json.workspace = true +serde-saphyr.workspace = true diff --git a/ci/crates/github-repo/src/lib.rs b/ci/crates/github-repo/src/lib.rs index c71bc4997..cc749de88 100644 --- a/ci/crates/github-repo/src/lib.rs +++ b/ci/crates/github-repo/src/lib.rs @@ -1,15 +1,25 @@ -//! Shared GitHub repository URL classification. +//! Shared tool source and GitHub repository URL classification. use anyhow::{Context, Result, ensure}; +use serde::{Deserialize, Serialize, Serializer}; -/// A repository parsed from a GitHub HTTP(S) URL, borrowing its owner and name. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct GithubRepo<'a> { - owner: &'a str, - name: &'a str, +/// A repository parsed from a GitHub HTTP(S) URL. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct GithubRepo { + owner: String, + name: String, + original_url: String, } -impl<'a> TryFrom<&'a str> for GithubRepo<'a> { +impl GithubRepo { + /// Returns the original URL without normalizing its spelling or trailing slashes. + #[must_use] + pub fn as_str(&self) -> &str { + &self.original_url + } +} + +impl TryFrom<&str> for GithubRepo { type Error = anyhow::Error; /// Parses an HTTP(S) GitHub repository URL, ignoring trailing slashes. @@ -18,8 +28,8 @@ impl<'a> TryFrom<&'a str> for GithubRepo<'a> { /// /// Returns an error for an unsupported prefix, missing owner or repository, /// or a repository path containing a subpath. - fn try_from(url: &'a str) -> Result { - let url = url.trim_end_matches('/'); + fn try_from(original_url: &str) -> Result { + let url = original_url.trim_end_matches('/'); let path = url .strip_prefix("https://github.com/") .or_else(|| url.strip_prefix("http://github.com/")) @@ -29,16 +39,67 @@ impl<'a> TryFrom<&'a str> for GithubRepo<'a> { !owner.is_empty() && !name.is_empty() && !name.contains('/'), "Expected a repository URL with no subpath" ); - Ok(Self { owner, name }) + Ok(Self { + owner: owner.to_owned(), + name: name.to_owned(), + original_url: original_url.to_owned(), + }) } } -impl std::fmt::Display for GithubRepo<'_> { +impl std::fmt::Display for GithubRepo { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}/{}", self.owner, self.name) } } +/// A tool's source URL, classified without rejecting unsupported or malformed URLs. +/// +/// Serialized as the original plain string, not as a tagged enum. +#[derive(Debug, Clone, Deserialize, PartialEq, Eq)] +#[serde(from = "String")] +pub enum ToolSource { + /// A supported GitHub repository URL. + Github(GithubRepo), + /// Any other source string, retained verbatim for manual review. + Other(String), +} + +impl ToolSource { + /// Returns the original source string without URL normalization. + #[must_use] + pub fn as_str(&self) -> &str { + match self { + Self::Github(repo) => repo.as_str(), + Self::Other(source) => source, + } + } +} + +impl From for ToolSource { + fn from(source: String) -> Self { + GithubRepo::try_from(source.as_str()).map_or(Self::Other(source), Self::Github) + } +} + +impl From<&str> for ToolSource { + fn from(source: &str) -> Self { + GithubRepo::try_from(source).map_or_else(|_| Self::Other(source.to_owned()), Self::Github) + } +} + +impl std::fmt::Display for ToolSource { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +impl Serialize for ToolSource { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(self.as_str()) + } +} + #[cfg(test)] mod tests { use super::*; @@ -46,13 +107,9 @@ mod tests { #[test] fn parses_plain_github_url() -> Result<()> { let repo = GithubRepo::try_from("https://github.com/owner/repo")?; - assert_eq!( - repo, - GithubRepo { - owner: "owner", - name: "repo" - } - ); + assert_eq!(repo.owner, "owner"); + assert_eq!(repo.name, "repo"); + assert_eq!(repo.as_str(), "https://github.com/owner/repo"); assert_eq!(repo.to_string(), "owner/repo"); Ok(()) } @@ -60,13 +117,8 @@ mod tests { #[test] fn parses_trailing_slash() -> Result<()> { let repo = GithubRepo::try_from("https://github.com/owner/repo/")?; - assert_eq!( - repo, - GithubRepo { - owner: "owner", - name: "repo" - } - ); + assert_eq!(repo.to_string(), "owner/repo"); + assert_eq!(repo.as_str(), "https://github.com/owner/repo/"); Ok(()) } @@ -113,6 +165,66 @@ mod tests { } } + #[test] + fn source_variants_preserve_raw_strings_through_serde() -> Result<()> { + for (raw, github) in [ + ("https://github.com/Owner/Repo.git", true), + ("http://github.com/Owner/Repo///", true), + ("https://github.com/owner/repo?tab=readme", true), + ("https://github.com/owner/repo#readme", true), + ("https://github.com/owner/repo%2Ftree", true), + ("https://github.com/owner/repo ", true), + ("https://gitlab.com/owner/repo/", false), + ("https://github.com/owner/repo/tree/main", false), + ("https://github.com/owner/", false), + ("https://github.com//repo", false), + ("https://GitHub.com/owner/repo", false), + (" https://github.com/owner/repo", false), + ("git@github.com:owner/repo.git", false), + ("not a URL", false), + ("", false), + ] { + let encoded = serde_json::to_string(raw)?; + for source in [ + ToolSource::from(raw), + ToolSource::from(raw.to_owned()), + serde_json::from_str::(&encoded)?, + serde_saphyr::from_str::(&encoded)?, + ] { + assert_eq!(matches!(source, ToolSource::Github(_)), github, "{raw}"); + assert_eq!(source.as_str(), raw); + assert_eq!(source.to_string(), raw); + assert_eq!(serde_json::to_string(&source)?, encoded); + } + } + Ok(()) + } + + #[test] + fn source_deserialization_matches_plain_string_acceptance() { + for raw in ["null", "42", "true", "[]", r#"{"Github":"owner/repo"}"#] { + assert!(serde_json::from_str::(raw).is_err(), "{raw}"); + assert_eq!( + serde_saphyr::from_str::(raw).ok(), + serde_saphyr::from_str::(raw) + .ok() + .map(ToolSource::from), + "{raw}" + ); + } + } + + #[test] + fn repository_owns_its_url() -> Result<()> { + let repo = { + let url = String::from("http://github.com/Owner/Repo///"); + GithubRepo::try_from(url.as_str())? + }; + assert_eq!(repo.to_string(), "Owner/Repo"); + assert_eq!(repo.as_str(), "http://github.com/Owner/Repo///"); + Ok(()) + } + #[test] fn rejects_subpath() { assert!(GithubRepo::try_from("https://github.com/owner/repo/tree/main/subdir").is_err()); @@ -138,8 +250,9 @@ mod tests { assert_eq!( GithubRepo::try_from(url).ok(), Some(GithubRepo { - owner: "owner", - name: "repo" + owner: "owner".into(), + name: "repo".into(), + original_url: url.into(), }) ); } diff --git a/ci/crates/pr-check/src/input.rs b/ci/crates/pr-check/src/input.rs index af5d2074f..d0c56433c 100644 --- a/ci/crates/pr-check/src/input.rs +++ b/ci/crates/pr-check/src/input.rs @@ -1,6 +1,7 @@ //! Catalog input paths and tool loading. use anyhow::{Context, Result, ensure}; +use github_repo::ToolSource; use serde::Deserialize; use std::fmt; use std::path::{Component, PathBuf}; @@ -40,7 +41,7 @@ impl fmt::Display for ToolPath { #[derive(Debug, Deserialize)] pub struct ToolEntry { pub name: String, - pub source: Option, + pub source: Option, pub homepage: Option, } @@ -104,6 +105,30 @@ mod tests { Ok(()) } + #[test] + fn tool_sources_are_classified_during_deserialization() -> Result<()> { + let tool: ToolEntry = + serde_saphyr::from_str("name: Example\nsource: http://github.com/Owner/Repo///\n")?; + let Some(ToolSource::Github(repo)) = tool.source else { + anyhow::bail!("Expected a GitHub repository"); + }; + assert_eq!(repo.to_string(), "Owner/Repo"); + assert_eq!(repo.as_str(), "http://github.com/Owner/Repo///"); + for source in [ + "https://gitlab.com/owner/repo", + "https://github.com/owner/repo/tree/main", + "", + ] { + let yaml = format!("name: Example\nsource: '{source}'\n"); + let tool: ToolEntry = serde_saphyr::from_str(&yaml)?; + assert!(matches!(tool.source, Some(ToolSource::Other(ref value)) if value == source)); + } + for yaml in ["name: Example\n", "name: Example\nsource: null\n"] { + assert!(serde_saphyr::from_str::(yaml)?.source.is_none()); + } + Ok(()) + } + #[test] fn tool_yaml_keeps_optional_fields_and_ignores_unrelated_metadata() -> Result<()> { let tool: ToolEntry = diff --git a/ci/crates/pr-check/src/network.rs b/ci/crates/pr-check/src/network.rs index 46695043f..53f07b743 100644 --- a/ci/crates/pr-check/src/network.rs +++ b/ci/crates/pr-check/src/network.rs @@ -2,7 +2,7 @@ use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; -use github_repo::GithubRepo; +use github_repo::{GithubRepo, ToolSource}; use serde::{Deserialize, de::DeserializeOwned}; use crate::checks::{Check, DomainAge}; @@ -67,7 +67,7 @@ impl GithubClient { /// # Errors /// /// Returns an error if the API call fails. - async fn repo_info(&self, repo: GithubRepo<'_>) -> Result> { + async fn repo_info(&self, repo: &GithubRepo) -> Result> { let url = format!("https://api.github.com/repos/{repo}"); self.get::(&url).await } @@ -77,7 +77,7 @@ impl GithubClient { /// # Errors /// /// Returns an error if the API call fails. - async fn contributor_count(&self, repo: GithubRepo<'_>) -> Result> { + async fn contributor_count(&self, repo: &GithubRepo) -> Result> { let url = format!("https://api.github.com/repos/{repo}/contributors?per_page=100&anon=0"); Ok(self .get::>(&url) @@ -95,11 +95,7 @@ impl GithubClient { pub async fn check_tool(client: &GithubClient, tool: &ToolEntry) -> Result { let source = &tool.source; - let repo = source - .as_deref() - .and_then(|url| GithubRepo::try_from(url).ok()); - - if let Some(repo) = repo { + if let Some(ToolSource::Github(repo)) = source { let repo_result = client.repo_info(repo).await; let contributors_result = client.contributor_count(repo).await; diff --git a/ci/crates/pr-check/src/report.rs b/ci/crates/pr-check/src/report.rs index fc40af8a5..94a6df49d 100644 --- a/ci/crates/pr-check/src/report.rs +++ b/ci/crates/pr-check/src/report.rs @@ -1,6 +1,7 @@ //! Report status, Markdown rendering, and workflow exit-code contract. use askama::Template; +use github_repo::ToolSource; use std::process::ExitCode; // Identifies the report as output from the contribution checker. @@ -42,7 +43,7 @@ impl CheckResult { #[derive(Debug)] pub struct ToolReport { pub name: String, - pub source: Option, + pub source: Option, pub stars: CheckResult, pub contributors: CheckResult, pub age: CheckResult, @@ -140,6 +141,26 @@ mod tests { } } + #[test] + fn comments_preserve_source_text_for_each_variant() { + for source in [ + "http://github.com/Owner/Repo///", + "https://gitlab.com/owner/repo", + "", + ] { + let report = ToolReport { + source: Some(source.into()), + ..passing_report() + }; + let reports = Reports::from_iter([report]); + assert!( + Comment::from(&reports) + .to_string() + .contains(&format!("Source: {source}\n")) + ); + } + } + #[test] fn collect_and_extend_preserve_input_order() { let report = |name: &str| ToolReport { diff --git a/ci/crates/render/src/deprecation.rs b/ci/crates/render/src/deprecation.rs index 5bf2f4e70..a67034bfc 100644 --- a/ci/crates/render/src/deprecation.rs +++ b/ci/crates/render/src/deprecation.rs @@ -1,6 +1,6 @@ use anyhow::{Context, Result}; use chrono::{DateTime, Local, NaiveDate, Utc}; -use github_repo::GithubRepo; +use github_repo::{GithubRepo, ToolSource}; use serde::Deserialize; use crate::types::Entry; @@ -38,7 +38,7 @@ impl GithubClient { }) } - async fn latest_commit_date(&self, repo: GithubRepo<'_>) -> Result>> { + async fn latest_commit_date(&self, repo: &GithubRepo) -> Result>> { let url = format!("https://api.github.com/repos/{repo}/commits?per_page=1"); let response = self .client @@ -93,11 +93,7 @@ pub async fn check_deprecated(token: &str, entries: &mut [Entry]) -> Result<()> let client = GithubClient::new(token)?; for entry in entries { - let Some(repo) = entry - .source - .as_deref() - .and_then(|url| GithubRepo::try_from(url).ok()) - else { + let Some(ToolSource::Github(repo)) = &entry.source else { continue; }; let last_commit = match client.latest_commit_date(repo).await { diff --git a/ci/crates/render/src/types.rs b/ci/crates/render/src/types.rs index 9ae47a6e7..e5909afe8 100644 --- a/ci/crates/render/src/types.rs +++ b/ci/crates/render/src/types.rs @@ -1,5 +1,6 @@ use anyhow::{Result, bail}; use askama::Template; +use github_repo::ToolSource; use serde::{Deserialize, Serialize, de::value::StrDeserializer}; use std::cmp::Ordering; use std::collections::{BTreeMap, BTreeSet}; @@ -66,7 +67,7 @@ pub struct ParsedEntry { pub license: String, pub types: BTreeSet, pub homepage: String, - pub source: Option, + pub source: Option, pub pricing: Option, pub plans: Option>, pub description: String, @@ -98,7 +99,7 @@ pub struct Entry { pub license: String, pub types: BTreeSet, pub homepage: String, - pub source: Option, + pub source: Option, pub pricing: Option, pub plans: Option>, pub description: String, @@ -264,7 +265,7 @@ pub struct ApiEntry { pub licenses: Vec, pub types: BTreeSet, pub homepage: String, - pub source: Option, + pub source: Option, pub pricing: Option, pub plans: Option>, pub description: String, diff --git a/ci/crates/render/tests/regression.rs b/ci/crates/render/tests/regression.rs index b097b9804..07e4dc2b5 100644 --- a/ci/crates/render/tests/regression.rs +++ b/ci/crates/render/tests/regression.rs @@ -1,8 +1,9 @@ use anyhow::{Context, Result}; +use github_repo::{GithubRepo, ToolSource}; use serde_json::json; use std::collections::{BTreeMap, BTreeSet}; -use render::types::{Catalog, Entry, ParsedEntry, Tag, ToolType, Type}; +use render::types::{ApiEntry, Catalog, Entry, ParsedEntry, Tag, ToolType, Type}; use render::{create_api, create_catalog, format_stats, stats}; fn tag(value: &str, kind: Type) -> Tag { @@ -26,6 +27,106 @@ fn parsed() -> Result { }))?) } +fn assert_source_preserved(tool: ParsedEntry, tags: &[Tag], raw: Option<&str>) -> Result<()> { + let expected = raw.map(ToolSource::from); + assert_eq!(tool.source, expected); + assert_eq!(serde_json::to_value(&tool)?["source"], json!(raw)); + let entry = Entry::from_parsed(tool, tags)?; + assert_eq!(entry.source, expected); + let encoded = serde_json::to_string(&entry)?; + assert_eq!(serde_json::from_str::(&encoded)?.source, expected); + assert_eq!(serde_saphyr::from_str::(&encoded)?.source, expected); + let api = create_api(vec![entry], tags, &[]); + let api_entry = api.values().next().context("Expected one API entry")?; + assert_eq!(api_entry.source, expected); + assert_eq!(serde_json::to_value(api_entry)?["source"], json!(raw)); + let encoded = serde_json::to_string(api_entry)?; + assert_eq!(serde_json::from_str::(&encoded)?.source, expected); + assert_eq!( + serde_saphyr::from_str::(&encoded)?.source, + expected + ); + Ok(()) +} + +#[test] +fn source_variants_and_absence_survive_normalization_and_api_output() -> Result<()> { + for (raw, github) in [ + (Some("http://github.com/Owner/Repo///"), true), + (Some("https://github.com/owner/repo?tab=readme"), true), + (Some("https://gitlab.com/Owner/Repo/"), false), + (Some("https://github.com/owner/repo/tree/main"), false), + (Some("https://github.com/owner/"), false), + (Some("not a URL"), false), + (Some(""), false), + (None, false), + ] { + let mut value = serde_json::to_value(parsed()?)?; + value["source"] = json!(raw); + let encoded = serde_json::to_string(&value)?; + for tool in [ + serde_json::from_str::(&encoded)?, + serde_saphyr::from_str::(&encoded)?, + ] { + assert_eq!(matches!(tool.source, Some(ToolSource::Github(_))), github); + assert_source_preserved(tool, &[tag("rust", Type::Language)], raw)?; + } + } + let mut value = serde_json::to_value(parsed()?)?; + value + .as_object_mut() + .context("Expected object")? + .remove("source"); + let encoded = serde_json::to_string(&value)?; + for tool in [ + serde_json::from_str::(&encoded)?, + serde_saphyr::from_str::(&encoded)?, + ] { + assert_source_preserved(tool, &[tag("rust", Type::Language)], None)?; + } + Ok(()) +} + +#[test] +fn full_catalog_preserves_original_sources_through_api_output() -> Result<()> { + #[derive(serde::Deserialize)] + struct RawSource { + source: Option, + } + + let data = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../data"); + let tags: Vec = serde_saphyr::from_str(&std::fs::read_to_string(data.join("tags.yml"))?)?; + let mut counts = [0; 3]; + for file in std::fs::read_dir(data.join("tools"))? { + let path = file?.path(); + if path.extension().is_none_or(|extension| extension != "yml") { + continue; + } + let yaml = std::fs::read_to_string(&path)?; + let raw: RawSource = serde_saphyr::from_str(&yaml)?; + let tool: ParsedEntry = serde_saphyr::from_str(&yaml)?; + let index = match &tool.source { + Some(ToolSource::Github(repo)) => { + assert_eq!(Some(repo.as_str()), raw.source.as_deref()); + 0 + } + Some(ToolSource::Other(source)) => { + assert!(GithubRepo::try_from(source.as_str()).is_err()); + 1 + } + None => 2, + }; + counts[index] += 1; + assert_source_preserved(tool, &tags, raw.source.as_deref()) + .with_context(|| path.display().to_string())?; + } + assert!( + counts.into_iter().all(|count| count > 0), + "Expected GitHub, other, and absent sources" + ); + Ok(()) +} + #[test] fn normalization_preserves_fields_and_uses_the_first_matching_tag() -> Result<()> { let original = parsed()?;