Skip to content

Preserve original trailing slash if present in find-links URLs #14387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jtfmumm
Copy link
Contributor

@jtfmumm jtfmumm commented Jul 1, 2025

As part of #14245 and #14346, uv was normalizing trailing slashes for find-links URLs by removing them. However, find-links URLs have different meanings depending on whether a trailing slash is present (as evidenced in 301 redirects from PyPI and #14367). This PR preserves trailing slashes if present in find-links URLs.

Fixes #14367

@jtfmumm jtfmumm assigned jtfmumm and unassigned jtfmumm Jul 1, 2025
@jtfmumm jtfmumm added the bug Something isn't working label Jul 1, 2025
@jtfmumm jtfmumm force-pushed the jtfm/add-trailing-slash-for-find-links branch from f527ad5 to caa4b8d Compare July 1, 2025 09:27
@jtfmumm jtfmumm added the do-not-merge Pull request is not ready to merge label Jul 1, 2025
@jtfmumm jtfmumm force-pushed the jtfm/add-trailing-slash-for-find-links branch from caa4b8d to 5233821 Compare July 1, 2025 09:27
@jtfmumm jtfmumm temporarily deployed to uv-test-registries July 1, 2025 09:34 — with GitHub Actions Inactive
@charliermarsh
Copy link
Member

I think relying on url.given() is dangerous, I don't think that persists through serialization / deserialization. I think we need to avoid stripping the trailing slash for --find-links URLs in the first place.

@jtfmumm jtfmumm changed the title Add original trailing slash if missing before find-links request Preserve original trailing slash if present on find-links URLs Jul 4, 2025
@jtfmumm jtfmumm had a problem deploying to uv-test-registries July 4, 2025 08:08 — with GitHub Actions Error
@jtfmumm jtfmumm force-pushed the jtfm/add-trailing-slash-for-find-links branch from 41143c0 to 2cc4ce5 Compare July 4, 2025 08:08
@jtfmumm jtfmumm temporarily deployed to uv-test-registries July 4, 2025 08:10 — with GitHub Actions Inactive
@jtfmumm jtfmumm force-pushed the jtfm/add-trailing-slash-for-find-links branch 2 times, most recently from cdbc20f to a5ac13d Compare July 4, 2025 08:50
@jtfmumm jtfmumm temporarily deployed to uv-test-registries July 4, 2025 08:52 — with GitHub Actions Inactive

/// Construct an [`IndexUrl`] from a [`VerbatimUrl`], applying a [`TrailingSlashPolicy`]
/// to non-file URLs.
fn from_verbatim_url_with_trailing_slash_policy(
Copy link
Contributor Author

@jtfmumm jtfmumm Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic was moved from the From<VerbatimUrl> implementation. It's unchanged except that the trailing slash is only removed if TrailingSlashPolicy::Remove is passed in.

@jtfmumm
Copy link
Contributor Author

jtfmumm commented Jul 4, 2025

I think relying on url.given() is dangerous, I don't think that persists through serialization / deserialization. I think we need to avoid stripping the trailing slash for --find-links URLs in the first place.

Updated. I've also updated the VerbatimUrl rustdocs in #14456 to make this behavior clearer.

@jtfmumm jtfmumm changed the title Preserve original trailing slash if present on find-links URLs Preserve original trailing slash if present in find-links URLs Jul 4, 2025
@jtfmumm jtfmumm force-pushed the jtfm/add-trailing-slash-for-find-links branch from a5ac13d to 6012b9d Compare July 4, 2025 09:06
@jtfmumm jtfmumm removed the do-not-merge Pull request is not ready to merge label Jul 4, 2025
@jtfmumm jtfmumm marked this pull request as ready for review July 4, 2025 09:06
@jtfmumm jtfmumm temporarily deployed to uv-test-registries July 4, 2025 09:07 — with GitHub Actions Inactive
/// directory.
///
/// Preserves trailing slash if present in `path`.
pub fn parse_preserving_trailing_slash(path: &str) -> Result<Self, IndexUrlError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this name. Originally I had it as parse_without_slash_normalization. Could be parse_preserve_trailing_slash but I think parse_preserve_slash isn't enough information?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I avoided things like parse_as_given or parse_exact since there could have already been different kinds of normalization prior to this point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just use:

  • parse_simple_api
  • parse_find_links

And thus not require callers to pass in a trailing slash policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's better. I can follow a similar pattern with the verbatim URL cases: e.g., simple_api_from_verbatim_url

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah or from_simple_api_url?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


/// Construct an [`IndexUrl`] from a [`VerbatimUrl`], preserving a trailing
/// slash if present.
pub fn from_verbatim_url_preserving_trailing_slash(url: VerbatimUrl) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same point as above about the function name

jtfmumm added a commit that referenced this pull request Jul 4, 2025
fn from(url: VerbatimUrl) -> Self {
// Remove trailing slashes for consistency. They'll be re-added if necessary when
// querying the Simple API.
Self::from_verbatim_url_with_trailing_slash_policy(url, TrailingSlashPolicy::Remove)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just get rid of this, since it seems like callers must be cognizant of whether they want to preserve or remove the trailing slash.

@jtfmumm jtfmumm temporarily deployed to uv-test-registries July 7, 2025 14:28 — with GitHub Actions Inactive
@jtfmumm jtfmumm temporarily deployed to uv-test-publish July 7, 2025 14:28 — with GitHub Actions Inactive
@jtfmumm jtfmumm force-pushed the jtfm/add-trailing-slash-for-find-links branch from 87b2cd6 to 081168d Compare July 7, 2025 14:53
@jtfmumm jtfmumm force-pushed the jtfm/add-trailing-slash-for-find-links branch from 081168d to 29651ed Compare July 7, 2025 14:54
@jtfmumm jtfmumm temporarily deployed to uv-test-registries July 7, 2025 14:56 — with GitHub Actions Inactive
@charliermarsh
Copy link
Member

Actually, hmm, there may be one bug here. We call without_trailing_slash in satisfies on all index URLs. But I think that will now be wrong for --find-links URLs? So we'll incorrectly think that the lockfile needs to be updated, since there will be an index URL in the lockfile that doesn't match the "collection" of index URLs.

@charliermarsh
Copy link
Member

We should probably try to find a way to remove without_trailing_slash there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--find-links URL Handling Regression in uv 0.7.17
3 participants