fix(publish): normalize legacy repository URL trailing slash#10732
fix(publish): normalize legacy repository URL trailing slash#10732LouisLau-art wants to merge 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's GuideNormalizes legacy repository upload URLs to always end with a trailing slash and adds coverage to ensure publishing uses the normalized URL when configured without it. Class diagram for Publisher and legacy repository URL normalizationclassDiagram
class publisher_module {
_normalize_legacy_repository_url(url: str) str
}
class Publisher {
+publish(repository_name: str, username: str, password: str, cert: str, client_cert: str, dry_run: bool) None
}
publisher_module ..> Publisher : used_by
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/publishing/test_publisher.py:103-104` </location>
<code_context>
+ publisher = Publisher(poetry, NullIO())
+ publisher.publish("testpypi", None, None)
+
+ assert uploader_auth.call_args == [("foo", "bar")]
+ assert uploader_upload.call_args == [
+ ("https://test.pypi.org/legacy/",),
+ {"cert": True, "client_cert": None, "dry_run": False, "skip_existing": False},
</code_context>
<issue_to_address>
**issue (testing):** Use mock's assertion helpers instead of comparing `call_args` to raw lists
`call_args` is a `(args, kwargs)` tuple, so comparing it to a list will not work as intended and may cause this test to always fail. Instead, assert on the expected calls via the mock helpers:
```python
uploader_auth.assert_called_once_with("foo", "bar")
uploader_upload.assert_called_once_with(
"https://test.pypi.org/legacy/",
cert=True,
client_cert=None,
dry_run=False,
skip_existing=False,
)
```
This makes the expectations explicit and avoids depending on the internal structure of `call_args`.
</issue_to_address>
### Comment 2
<location> `tests/publishing/test_publisher.py:85-94` </location>
<code_context>
+def test_publish_normalizes_legacy_repository_url_without_trailing_slash(
</code_context>
<issue_to_address>
**suggestion (testing):** Add complementary tests to cover non-legacy and already-normalized URLs
This test captures the `/legacy` regression, but it’s worth also checking that:
1. URLs already ending in `/legacy/` are left unchanged (no double slash).
2. URLs not ending in `/legacy` or `/legacy/` are not modified.
You can either add two focused tests or parametrize this one over input/expected URLs to more fully exercise the normalization helper and guard against similar regressions.
Suggested implementation:
```python
@pytest.mark.parametrize(
("configured_url", "expected_url"),
[
("https://test.pypi.org/legacy", "https://test.pypi.org/legacy/"),
("https://test.pypi.org/legacy/", "https://test.pypi.org/legacy/"),
("https://test.pypi.org/simple", "https://test.pypi.org/simple"),
],
)
def test_publish_normalizes_legacy_repository_url_without_trailing_slash(
fixture_dir: FixtureDirGetter,
mocker: MockerFixture,
config: Config,
configured_url: str,
expected_url: str,
) -> None:
```
```python
poetry.config.merge(
{
"repositories": {"testpypi": {"url": configured_url}},
```
1. Ensure `pytest` is imported at the top of `tests/publishing/test_publisher.py` if it is not already:
- `import pytest`
2. In the remainder of this test (not visible in the snippet), wherever the test currently asserts against a hard-coded normalized URL like `"https://test.pypi.org/legacy/"`, update those assertions to use `expected_url` instead. For example, if you have:
- `uploader_auth.assert_called_with("testpypi", "https://test.pypi.org/legacy/")`
- change it to:
- `uploader_auth.assert_called_with("testpypi", expected_url)`
3. Similarly, if the URL is passed to any other collaborator (e.g. `Uploader.upload` or a `Publisher` instance) and is asserted on, replace the hard-coded string with `expected_url` to fully exercise the normalization behavior across all three parametrized cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert uploader_auth.call_args == [("foo", "bar")] | ||
| assert uploader_upload.call_args == [ |
There was a problem hiding this comment.
issue (testing): Use mock's assertion helpers instead of comparing call_args to raw lists
call_args is a (args, kwargs) tuple, so comparing it to a list will not work as intended and may cause this test to always fail. Instead, assert on the expected calls via the mock helpers:
uploader_auth.assert_called_once_with("foo", "bar")
uploader_upload.assert_called_once_with(
"https://test.pypi.org/legacy/",
cert=True,
client_cert=None,
dry_run=False,
skip_existing=False,
)This makes the expectations explicit and avoids depending on the internal structure of call_args.
| def test_publish_normalizes_legacy_repository_url_without_trailing_slash( | ||
| fixture_dir: FixtureDirGetter, mocker: MockerFixture, config: Config | ||
| ) -> None: | ||
| uploader_auth = mocker.patch("poetry.publishing.uploader.Uploader.auth") | ||
| uploader_upload = mocker.patch("poetry.publishing.uploader.Uploader.upload") | ||
|
|
||
| poetry = Factory().create_poetry(fixture_dir("sample_project")) | ||
| poetry._config = config | ||
| poetry.config.merge( | ||
| { |
There was a problem hiding this comment.
suggestion (testing): Add complementary tests to cover non-legacy and already-normalized URLs
This test captures the /legacy regression, but it’s worth also checking that:
- URLs already ending in
/legacy/are left unchanged (no double slash). - URLs not ending in
/legacyor/legacy/are not modified.
You can either add two focused tests or parametrize this one over input/expected URLs to more fully exercise the normalization helper and guard against similar regressions.
Suggested implementation:
@pytest.mark.parametrize(
("configured_url", "expected_url"),
[
("https://test.pypi.org/legacy", "https://test.pypi.org/legacy/"),
("https://test.pypi.org/legacy/", "https://test.pypi.org/legacy/"),
("https://test.pypi.org/simple", "https://test.pypi.org/simple"),
],
)
def test_publish_normalizes_legacy_repository_url_without_trailing_slash(
fixture_dir: FixtureDirGetter,
mocker: MockerFixture,
config: Config,
configured_url: str,
expected_url: str,
) -> None: poetry.config.merge(
{
"repositories": {"testpypi": {"url": configured_url}},- Ensure
pytestis imported at the top oftests/publishing/test_publisher.pyif it is not already:import pytest
- In the remainder of this test (not visible in the snippet), wherever the test currently asserts against a hard-coded normalized URL like
"https://test.pypi.org/legacy/", update those assertions to useexpected_urlinstead. For example, if you have:uploader_auth.assert_called_with("testpypi", "https://test.pypi.org/legacy/")- change it to:
uploader_auth.assert_called_with("testpypi", expected_url)
- Similarly, if the URL is passed to any other collaborator (e.g.
Uploader.uploador aPublisherinstance) and is asserted on, replace the hard-coded string withexpected_urlto fully exercise the normalization behavior across all three parametrized cases.
| url = self._poetry.config.get(f"repositories.{repository_name}.url") | ||
| if url is None: | ||
| raise RuntimeError(f"Repository {repository_name} is not defined") | ||
| url = _normalize_legacy_repository_url(url) |
There was a problem hiding this comment.
Wouldn't this be good enough?
| url = _normalize_legacy_repository_url(url) | |
| if url.endswith("/legacy"): | |
| url += "/" |
|
Is there definitely a problem that needs solving here? Do we have a reproducer? I see that the issue report was made in 2022. If there is a real problem here, with a trivial fix available, it is surprising that it should have survived all this time. |
|
@dimbleby Yes, I believe there is a real reproducer here, and it is the first half of #6687 (the trailing-slash part, not the auth confusion that came later in the same report). The minimal case is: poetry config repositories.testpypi https://test.pypi.org/legacy
poetry publish -r testpypiThe original reporter notes that this silently fails, while the same flow works once the URL is changed to So this PR is intentionally very narrow: it only normalizes configured repository URLs that end in My guess for why this survived for so long is that it only affects explicitly configured legacy endpoints without the trailing slash; users who rely on the default PyPI/TestPyPI setup, or who already configure If you'd prefer, I can also add a slightly higher-level regression test, but I started with the smallest change at the point where Poetry consumes the configured repository URL. |
|
I see what the original report says. Did you verify that it is true today? |
|
Yes, I verified it again on the current code. I tested on Result:
So today it still passes the configured URL through verbatim, and the missing trailing slash case is still present. |
|
Yes, but my question is whether this is actually an issue. Does pypi - or testpypi - really reject uploads when the user does not provide a trailing slash? This should be answered by experiment. |
|
Either way I don't know that it is up to poetry to guess that users meant something different than what they said. The endpoint is documented as https://upload.pypi.org/legacy/. Probably it's normal that if you configure the wrong endpoint, it won't work. |
|
I just tried with From a user's point of view at least a better error message would be nice if you just forget a slash.
Valid point. I think if there always has to be a trailing slash we can as well add it if it is missing. However, I am not sure if this is specific to PyPI or also valid for other indices... |
3 participants
Fixes #6687.
When a repository upload URL ends with /legacy (no trailing slash), some servers respond with a redirect that can cause uploads to silently fail. Poetry now normalizes legacy upload URLs to ensure they end with /legacy/.
Test:
Summary by Sourcery
Normalize legacy repository upload URLs used during publishing to avoid issues caused by missing trailing slashes.
Bug Fixes:
Tests: