From c59c8e080a2a2c591c9833e363fafda9932d582a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 6 Aug 2024 08:58:09 -0400 Subject: [PATCH] Use `UrlString` for `Source` struct (#5804) I don't think this will save any time in serialization, but it should save us some deserialization, since we only need to parse URLs for the packages we use... --- crates/distribution-types/src/file.rs | 21 +++++ crates/uv-resolver/src/lock.rs | 67 ++++++-------- ...r__lock__tests__hash_optional_missing.snap | 36 ++------ ...r__lock__tests__hash_optional_present.snap | 36 ++------ ...missing_dependency_source_unambiguous.snap | 90 ++++--------------- ...dependency_source_version_unambiguous.snap | 90 ++++--------------- ...issing_dependency_version_unambiguous.snap | 90 ++++--------------- ...lock__tests__source_direct_has_subdir.snap | 36 ++------ ..._lock__tests__source_direct_no_subdir.snap | 36 ++------ 9 files changed, 119 insertions(+), 383 deletions(-) diff --git a/crates/distribution-types/src/file.rs b/crates/distribution-types/src/file.rs index 84f434cac..2b593745b 100644 --- a/crates/distribution-types/src/file.rs +++ b/crates/distribution-types/src/file.rs @@ -6,6 +6,7 @@ use serde::{Deserialize, Serialize}; use url::Url; use pep440_rs::{VersionSpecifiers, VersionSpecifiersParseError}; +use pep508_rs::VerbatimUrl; use pypi_types::{CoreMetadata, HashDigest, Yanked}; /// Error converting [`pypi_types::File`] to [`distribution_type::File`]. @@ -149,6 +150,8 @@ impl Display for FileLocation { Clone, PartialEq, Eq, + PartialOrd, + Ord, Hash, Serialize, Deserialize, @@ -180,6 +183,24 @@ impl From for UrlString { } } +impl From<&Url> for UrlString { + fn from(value: &Url) -> Self { + UrlString(value.to_string()) + } +} + +impl From for UrlString { + fn from(value: VerbatimUrl) -> Self { + UrlString(value.raw().to_string()) + } +} + +impl From<&VerbatimUrl> for UrlString { + fn from(value: &VerbatimUrl) -> Self { + UrlString(value.raw().to_string()) + } +} + impl From for String { fn from(value: UrlString) -> Self { value.0 diff --git a/crates/uv-resolver/src/lock.rs b/crates/uv-resolver/src/lock.rs index 978b7465c..dbc9696dd 100644 --- a/crates/uv-resolver/src/lock.rs +++ b/crates/uv-resolver/src/lock.rs @@ -762,7 +762,7 @@ impl Distribution { let wheels = self .wheels .iter() - .map(|wheel| wheel.to_registry_dist(url)) + .map(|wheel| wheel.to_registry_dist(url.to_url())) .collect(); let reg_built_dist = RegistryBuiltDist { wheels, @@ -784,7 +784,7 @@ impl Distribution { Source::Direct(url, direct) => { let filename: WheelFilename = self.wheels[best_wheel_index].filename.clone(); let url = Url::from(ParsedArchiveUrl { - url: url.clone(), + url: url.to_url(), subdirectory: direct.subdirectory.as_ref().map(PathBuf::from), }); let direct_dist = DirectUrlBuiltDist { @@ -864,7 +864,7 @@ impl Distribution { Source::Git(url, git) => { // Reconstruct the `GitUrl` from the `GitSource`. let git_url = - uv_git::GitUrl::new(url.clone(), GitReference::from(git.kind.clone())) + uv_git::GitUrl::new(url.to_url(), GitReference::from(git.kind.clone())) .with_precise(git.precise); // Reconstruct the PEP 508-compatible URL from the `GitSource`. @@ -883,7 +883,7 @@ impl Distribution { } Source::Direct(url, direct) => { let url = Url::from(ParsedArchiveUrl { - url: url.clone(), + url: url.to_url(), subdirectory: direct.subdirectory.as_ref().map(PathBuf::from), }); let direct_dist = DirectUrlSourceDist { @@ -920,7 +920,7 @@ impl Distribution { url: FileLocation::AbsoluteUrl(file_url.clone()), yanked: None, }); - let index = IndexUrl::Url(VerbatimUrl::from_url(url.clone())); + let index = IndexUrl::Url(VerbatimUrl::from_url(url.to_url())); let reg_dist = RegistrySourceDist { name: self.id.name.clone(), @@ -962,7 +962,7 @@ impl Distribution { // Add any wheels. for wheel in &self.wheels { let hash = wheel.hash.as_ref().map(|h| h.0.clone()); - let wheel = wheel.to_registry_dist(url); + let wheel = wheel.to_registry_dist(url.to_url()); let compat = WheelCompatibility::Compatible(HashComparison::Matched, None, None); prioritized_dist.insert_built(wheel, hash, compat); @@ -1178,7 +1178,7 @@ impl Distribution { match &self.id.source { Source::Git(url, git) => Some(ResolvedRepositoryReference { reference: RepositoryReference { - url: RepositoryUrl::new(url), + url: RepositoryUrl::new(&url.to_url()), reference: GitReference::from(git.kind.clone()), }, sha: git.precise, @@ -1374,9 +1374,9 @@ impl From for DistributionIdForDependency { #[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord, serde::Deserialize)] #[serde(try_from = "SourceWire")] enum Source { - Registry(Url), - Git(Url, GitSource), - Direct(Url, DirectSource), + Registry(UrlString), + Git(UrlString, GitSource), + Direct(UrlString, DirectSource), Path(PathBuf), Directory(PathBuf), Editable(PathBuf), @@ -1435,14 +1435,14 @@ impl Source { fn from_direct_built_dist(direct_dist: &DirectUrlBuiltDist) -> Source { Source::Direct( - direct_dist.url.to_url(), + UrlString::from(&direct_dist.url), DirectSource { subdirectory: None }, ) } fn from_direct_source_dist(direct_dist: &DirectUrlSourceDist) -> Source { Source::Direct( - direct_dist.url.to_url(), + UrlString::from(&direct_dist.url), DirectSource { subdirectory: direct_dist .subdirectory @@ -1470,23 +1470,14 @@ impl Source { } fn from_index_url(index_url: &IndexUrl) -> Source { - /// Redact a URL by removing any username and password. - fn redact(mut url: Url) -> Result { - url.set_username("")?; - url.set_password(None)?; - Ok(url) - } - + // Remove any sensitive credentials from the index URL. + let redacted = index_url.redacted(); match *index_url { - IndexUrl::Pypi(ref verbatim_url) => { - Source::Registry(redact(verbatim_url.to_url()).expect("Could not redact URL")) - } - IndexUrl::Url(ref verbatim_url) => { - Source::Registry(redact(verbatim_url.to_url()).expect("Could not redact URL")) - } + IndexUrl::Pypi(_) => Source::Registry(UrlString::from(redacted.as_ref())), + IndexUrl::Url(_) => Source::Registry(UrlString::from(redacted.as_ref())), // TODO(konsti): Retain path on index url without converting to URL. - IndexUrl::Path(ref verbatim_url) => Source::Path( - verbatim_url + IndexUrl::Path(_) => Source::Path( + redacted .to_file_path() .expect("Could not convert index URL to path"), ), @@ -1495,7 +1486,7 @@ impl Source { fn from_git_dist(git_dist: &GitSourceDist) -> Source { Source::Git( - locked_git_url(git_dist), + UrlString::from(locked_git_url(git_dist)), GitSource { kind: GitSourceKind::from(git_dist.git.reference().clone()), precise: git_dist.git.precise().expect("precise commit"), @@ -1512,13 +1503,13 @@ impl Source { let mut source_table = InlineTable::new(); match *self { Source::Registry(ref url) => { - source_table.insert("registry", Value::from(url.as_str())); + source_table.insert("registry", Value::from(url.as_ref())); } Source::Git(ref url, _) => { - source_table.insert("git", Value::from(url.as_str())); + source_table.insert("git", Value::from(url.as_ref())); } Source::Direct(ref url, DirectSource { ref subdirectory }) => { - source_table.insert("url", Value::from(url.as_str())); + source_table.insert("url", Value::from(url.as_ref())); if let Some(ref subdirectory) = *subdirectory { source_table.insert("subdirectory", Value::from(subdirectory)); } @@ -1588,13 +1579,13 @@ impl Source { #[serde(untagged)] enum SourceWire { Registry { - registry: Url, + registry: UrlString, }, Git { git: String, }, Direct { - url: Url, + url: UrlString, #[serde(default)] subdirectory: Option, }, @@ -1635,7 +1626,7 @@ impl TryFrom for Source { }, }) .map_err(LockErrorKind::InvalidGitSourceUrl)?; - Ok(Source::Git(url, git_source)) + Ok(Source::Git(UrlString::from(url), git_source)) } Direct { url, subdirectory } => Ok(Source::Direct(url, DirectSource { subdirectory })), Path { path } => Ok(Source::Path(path.into())), @@ -2092,7 +2083,7 @@ impl Wheel { } } - fn to_registry_dist(&self, url: &Url) -> RegistryBuiltWheel { + fn to_registry_dist(&self, url: Url) -> RegistryBuiltWheel { let filename: WheelFilename = self.filename.clone(); let file = Box::new(distribution_types::File { dist_info_metadata: false, @@ -2104,7 +2095,7 @@ impl Wheel { url: FileLocation::AbsoluteUrl(self.url.clone()), yanked: None, }); - let index = IndexUrl::Url(VerbatimUrl::from_url(url.clone())); + let index = IndexUrl::Url(VerbatimUrl::from_url(url)); RegistryBuiltWheel { filename, file, @@ -2218,7 +2209,7 @@ impl Dependency { }, Source::Git(repository, git) => { let git_url = - uv_git::GitUrl::new(repository.clone(), GitReference::from(git.kind.clone())) + uv_git::GitUrl::new(repository.to_url(), GitReference::from(git.kind.clone())) .with_precise(git.precise); let parsed_url = ParsedUrl::Git(ParsedGitUrl { @@ -2229,7 +2220,7 @@ impl Dependency { } Source::Direct(url, direct) => { let parsed_url = ParsedUrl::Archive(ParsedArchiveUrl { - url: url.clone(), + url: url.to_url(), subdirectory: direct.subdirectory.as_ref().map(PathBuf::from), }); RequirementSource::from_verbatim_parsed_url(parsed_url) diff --git a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_missing.snap b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_missing.snap index 636ef555f..bc5bfe507 100644 --- a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_missing.snap +++ b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_missing.snap @@ -18,21 +18,9 @@ Ok( ), version: "4.3.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: None, @@ -74,21 +62,9 @@ Ok( ), version: "4.3.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 0, }, diff --git a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_present.snap b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_present.snap index 4cd107c7c..bd0581d79 100644 --- a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_present.snap +++ b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__hash_optional_present.snap @@ -18,21 +18,9 @@ Ok( ), version: "4.3.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: None, @@ -81,21 +69,9 @@ Ok( ), version: "4.3.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 0, }, diff --git a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap index 6c9e068fc..cd6bc6db2 100644 --- a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap +++ b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap @@ -18,21 +18,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: Some( @@ -68,21 +56,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: Some( @@ -115,21 +91,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, extra: {}, @@ -147,21 +111,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 0, DistributionId { @@ -170,21 +122,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 1, }, diff --git a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap index 6c9e068fc..cd6bc6db2 100644 --- a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap +++ b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap @@ -18,21 +18,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: Some( @@ -68,21 +56,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: Some( @@ -115,21 +91,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, extra: {}, @@ -147,21 +111,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 0, DistributionId { @@ -170,21 +122,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 1, }, diff --git a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap index 6c9e068fc..cd6bc6db2 100644 --- a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap +++ b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap @@ -18,21 +18,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: Some( @@ -68,21 +56,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, sdist: Some( @@ -115,21 +91,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }, extra: {}, @@ -147,21 +111,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 0, DistributionId { @@ -170,21 +122,9 @@ Ok( ), version: "0.1.0", source: Registry( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "pypi.org", - ), - ), - port: None, - path: "/simple", - query: None, - fragment: None, - }, + UrlString( + "https://pypi.org/simple", + ), ), }: 1, }, diff --git a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_has_subdir.snap b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_has_subdir.snap index 51430bee4..a80bda2fa 100644 --- a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_has_subdir.snap +++ b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_has_subdir.snap @@ -18,21 +18,9 @@ Ok( ), version: "4.3.0", source: Direct( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "burntsushi.net", - ), - ), - port: None, - path: "/", - query: None, - fragment: None, - }, + UrlString( + "https://burntsushi.net", + ), DirectSource { subdirectory: Some( "wat/foo/bar", @@ -55,21 +43,9 @@ Ok( ), version: "4.3.0", source: Direct( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "burntsushi.net", - ), - ), - port: None, - path: "/", - query: None, - fragment: None, - }, + UrlString( + "https://burntsushi.net", + ), DirectSource { subdirectory: Some( "wat/foo/bar", diff --git a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_no_subdir.snap b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_no_subdir.snap index 7844eef5c..746b0c8b5 100644 --- a/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_no_subdir.snap +++ b/crates/uv-resolver/src/snapshots/uv_resolver__lock__tests__source_direct_no_subdir.snap @@ -18,21 +18,9 @@ Ok( ), version: "4.3.0", source: Direct( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "burntsushi.net", - ), - ), - port: None, - path: "/", - query: None, - fragment: None, - }, + UrlString( + "https://burntsushi.net", + ), DirectSource { subdirectory: None, }, @@ -53,21 +41,9 @@ Ok( ), version: "4.3.0", source: Direct( - Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "burntsushi.net", - ), - ), - port: None, - path: "/", - query: None, - fragment: None, - }, + UrlString( + "https://burntsushi.net", + ), DirectSource { subdirectory: None, },