Skip to content

Commit

Permalink
Remove usages of verbatim URL in URL resolver (#4221)
Browse files Browse the repository at this point in the history
## Summary

Should be no behavior changes, but one piece of technical debt I noticed
left over in the URL resolver. We already have structured paths, so we
shouldn't need to compare verbatim URLs.
  • Loading branch information
charliermarsh authored Jun 10, 2024
1 parent fd52fe7 commit 0c1dcb7
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 232 deletions.
39 changes: 38 additions & 1 deletion crates/pypi-types/src/parsed_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use thiserror::Error;
use url::{ParseError, Url};

use pep508_rs::{Pep508Url, UnnamedRequirementUrl, VerbatimUrl, VerbatimUrlError};
use uv_git::{GitUrl, OidParseError};
use uv_git::{GitReference, GitSha, GitUrl, OidParseError};

use crate::{ArchiveInfo, DirInfo, DirectUrl, VcsInfo, VcsKind};

Expand Down Expand Up @@ -175,6 +175,17 @@ pub struct ParsedPathUrl {
pub editable: bool,
}

impl ParsedPathUrl {
/// Construct a [`ParsedPathUrl`] from a path requirement source.
pub fn from_source(path: PathBuf, editable: bool, url: Url) -> Self {
Self {
url,
path,
editable,
}
}
}

/// A Git repository URL.
///
/// Examples:
Expand All @@ -186,6 +197,22 @@ pub struct ParsedGitUrl {
pub subdirectory: Option<PathBuf>,
}

impl ParsedGitUrl {
/// Construct a [`ParsedGitUrl`] from a Git requirement source.
pub fn from_source(
repository: Url,
reference: GitReference,
precise: Option<GitSha>,
subdirectory: Option<PathBuf>,
) -> Self {
let mut url = GitUrl::new(repository, reference);
if let Some(precise) = precise {
url = url.with_precise(precise);
}
Self { url, subdirectory }
}
}

impl TryFrom<Url> for ParsedGitUrl {
type Error = ParsedUrlError;

Expand Down Expand Up @@ -219,6 +246,16 @@ pub struct ParsedArchiveUrl {
pub subdirectory: Option<PathBuf>,
}

impl ParsedArchiveUrl {
/// Construct a [`ParsedArchiveUrl`] from a URL requirement source.
pub fn from_source(location: Url, subdirectory: Option<PathBuf>) -> Self {
Self {
url: location,
subdirectory,
}
}
}

impl From<Url> for ParsedArchiveUrl {
fn from(url: Url) -> Self {
let subdirectory = get_subdirectory(&url);
Expand Down
61 changes: 38 additions & 23 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ use either::Either;
use itertools::Itertools;
use pubgrub::range::Range;
use rustc_hash::FxHashSet;
use same_file::is_same_file;
use tracing::warn;

use distribution_types::Verbatim;
use pep440_rs::Version;
use pep508_rs::MarkerEnvironment;
use pypi_types::{ParsedUrl, Requirement, RequirementSource, VerbatimParsedUrl};
use pypi_types::{
ParsedArchiveUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement, RequirementSource,
};
use uv_configuration::{Constraints, Overrides};
use uv_git::GitResolver;
use uv_normalize::{ExtraName, GroupName, PackageName};
Expand Down Expand Up @@ -258,15 +259,23 @@ impl PubGrubRequirement {
version,
})
}
RequirementSource::Url { url, .. } => {
RequirementSource::Url {
subdirectory,
location,
url,
} => {
let Some(expected) = urls.get(&requirement.name) else {
return Err(ResolveError::DisallowedUrl(
requirement.name.clone(),
url.to_string(),
));
};

if !Urls::is_allowed(&expected.verbatim, url, git) {
let parsed_url = ParsedUrl::Archive(ParsedArchiveUrl::from_source(
location.clone(),
subdirectory.clone(),
));
if !Urls::same_resource(&expected.parsed_url, &parsed_url, git) {
return Err(ResolveError::ConflictingUrlsTransitive(
requirement.name.clone(),
expected.verbatim.verbatim().to_string(),
Expand All @@ -284,15 +293,27 @@ impl PubGrubRequirement {
version: Range::full(),
})
}
RequirementSource::Git { url, .. } => {
RequirementSource::Git {
repository,
reference,
precise,
url,
subdirectory,
} => {
let Some(expected) = urls.get(&requirement.name) else {
return Err(ResolveError::DisallowedUrl(
requirement.name.clone(),
url.to_string(),
));
};

if !Urls::is_allowed(&expected.verbatim, url, git) {
let parsed_url = ParsedUrl::Git(ParsedGitUrl::from_source(
repository.clone(),
reference.clone(),
*precise,
subdirectory.clone(),
));
if !Urls::same_resource(&expected.parsed_url, &parsed_url, git) {
return Err(ResolveError::ConflictingUrlsTransitive(
requirement.name.clone(),
expected.verbatim.verbatim().to_string(),
Expand All @@ -310,30 +331,24 @@ impl PubGrubRequirement {
version: Range::full(),
})
}
RequirementSource::Path { url, path, .. } => {
RequirementSource::Path {
editable,
url,
path,
} => {
let Some(expected) = urls.get(&requirement.name) else {
return Err(ResolveError::DisallowedUrl(
requirement.name.clone(),
url.to_string(),
));
};

let mut is_allowed = Urls::is_allowed(&expected.verbatim, url, git);
if !is_allowed {
if let VerbatimParsedUrl {
parsed_url: ParsedUrl::Path(previous_path),
..
} = &expected
{
// On Windows, we can have two versions of the same path, e.g.
// `C:\Users\KONSTA~1` and `C:\Users\Konstantin`.
if is_same_file(path, &previous_path.path).unwrap_or(false) {
is_allowed = true;
}
}
}

if !is_allowed {
let parsed_url = ParsedUrl::Path(ParsedPathUrl::from_source(
path.clone(),
*editable,
url.to_url(),
));
if !Urls::same_resource(&expected.parsed_url, &parsed_url, git) {
return Err(ResolveError::ConflictingUrlsTransitive(
requirement.name.clone(),
expected.verbatim.verbatim().to_string(),
Expand Down
Loading

0 comments on commit 0c1dcb7

Please sign in to comment.