From 45811157b09e06da40056a730c6f0dab4689e770 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Mon, 13 Jan 2025 22:54:34 +0100 Subject: [PATCH 1/8] Improve error messages when creating projects and publishers This change improves the error messages when uploading a new project or creating a pending Trusted Publisher, when the new project's name already exists or is too similar to an existing project. Signed-off-by: Facundo Tuesca --- tests/unit/accounts/test_views.py | 11 ++++ tests/unit/forklift/test_legacy.py | 2 +- tests/unit/oidc/forms/test_activestate.py | 37 ++++++++---- tests/unit/oidc/forms/test_github.py | 57 +++++++++++++++---- tests/unit/oidc/forms/test_gitlab.py | 44 ++++++++++----- tests/unit/oidc/forms/test_google.py | 42 ++++++++++---- tests/unit/packaging/test_services.py | 39 ++++++++----- warehouse/accounts/views.py | 4 ++ warehouse/locale/messages.pot | 47 +++++++++------- warehouse/oidc/forms/_core.py | 68 +++++++++++++++-------- warehouse/oidc/forms/activestate.py | 3 +- warehouse/oidc/forms/github.py | 3 +- warehouse/oidc/forms/gitlab.py | 3 +- warehouse/oidc/forms/google.py | 3 +- warehouse/packaging/interfaces.py | 9 +-- warehouse/packaging/models.py | 31 +++++++++++ warehouse/packaging/services.py | 50 ++++++++++------- 17 files changed, 315 insertions(+), 138 deletions(-) diff --git a/tests/unit/accounts/test_views.py b/tests/unit/accounts/test_views.py index 150779c88561..df6ed937948c 100644 --- a/tests/unit/accounts/test_views.py +++ b/tests/unit/accounts/test_views.py @@ -3358,6 +3358,7 @@ def find_service(iface, name=None, context=None): find_service=pretend.call_recorder(find_service), route_url=pretend.stub(), POST=MultiDict(), + user=pretend.stub(id=pretend.stub()), registry=pretend.stub( settings={ "github.token": "fake-api-token", @@ -3526,6 +3527,7 @@ def test_manage_publishing(self, metrics, monkeypatch): api_token="fake-api-token", route_url=route_url, check_project_name=project_service.check_project_name, + user=request.user, ) ] assert pending_gitlab_publisher_form_cls.calls == [ @@ -3533,6 +3535,7 @@ def test_manage_publishing(self, metrics, monkeypatch): request.POST, route_url=route_url, check_project_name=project_service.check_project_name, + user=request.user, ) ] @@ -3621,6 +3624,7 @@ def test_manage_publishing_admin_disabled(self, monkeypatch, pyramid_request): api_token="fake-api-token", route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] assert pending_gitlab_publisher_form_cls.calls == [ @@ -3628,6 +3632,7 @@ def test_manage_publishing_admin_disabled(self, monkeypatch, pyramid_request): pyramid_request.POST, route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] @@ -3753,6 +3758,7 @@ def test_add_pending_oidc_publisher_admin_disabled( api_token="fake-api-token", route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] assert pending_gitlab_publisher_form_cls.calls == [ @@ -3760,6 +3766,7 @@ def test_add_pending_oidc_publisher_admin_disabled( pyramid_request.POST, route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] @@ -3897,6 +3904,7 @@ def test_add_pending_oidc_publisher_user_cannot_register( api_token="fake-api-token", route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] assert pending_gitlab_publisher_form_cls.calls == [ @@ -3904,6 +3912,7 @@ def test_add_pending_oidc_publisher_user_cannot_register( pyramid_request.POST, route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] @@ -4618,6 +4627,7 @@ def test_delete_pending_oidc_publisher_admin_disabled( api_token="fake-api-token", route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] assert pending_gitlab_publisher_form_cls.calls == [ @@ -4625,6 +4635,7 @@ def test_delete_pending_oidc_publisher_admin_disabled( pyramid_request.POST, route_url=pyramid_request.route_url, check_project_name=project_service.check_project_name, + user=pyramid_request.user, ) ] diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 3ba814a87bef..776656e0d083 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -907,7 +907,7 @@ def test_fails_with_ultranormalized_names( assert resp.status_code == 400 assert resp.status == ( - "400 The name {!r} is too similar to an existing project. " + "400 The name {!r} is too similar to an existing project named 'toasting'. " "See /the/help/url/ for more information." ).format(conflicting_name) diff --git a/tests/unit/oidc/forms/test_activestate.py b/tests/unit/oidc/forms/test_activestate.py index 17e5181f478d..67abf8eaf71e 100644 --- a/tests/unit/oidc/forms/test_activestate.py +++ b/tests/unit/oidc/forms/test_activestate.py @@ -19,7 +19,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import activestate -from warehouse.packaging.interfaces import ProjectNameUnavailableReason +from warehouse.packaging.models import ProjectNameUnavailableExisting fake_username = "some-username" fake_org_name = "some-org" @@ -50,6 +50,7 @@ def check_project_name(name): MultiDict(data), route_url=route_url, check_project_name=check_project_name, + user=pretend.stub(), ) # Test built-in validations @@ -61,27 +62,43 @@ def check_project_name(name): assert form._route_url == route_url assert form.validate() - def test_validate_project_name_already_in_use(self, pyramid_config): + @pytest.mark.parametrize("is_project_owner", [True, False]) + def test_validate_project_name_already_in_use( + self, pyramid_config, is_project_owner + ): route_url = pretend.call_recorder(lambda *args, **kwargs: "") + user = pretend.stub() + owners = [] + if is_project_owner: + owners.append(user) def check_project_name(name): - return ProjectNameUnavailableReason.AlreadyExists + return ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ) form = activestate.PendingActiveStatePublisherForm( route_url=route_url, check_project_name=check_project_name, + user=user, ) field = pretend.stub(data="some-project") with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"activestate"}}, - ) - ] + + if is_project_owner: + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"activestate"}}, + ) + ] + else: + assert route_url.calls == [] class TestActiveStatePublisherForm: diff --git a/tests/unit/oidc/forms/test_github.py b/tests/unit/oidc/forms/test_github.py index ad88ba67a1c7..2b71cf3f402d 100644 --- a/tests/unit/oidc/forms/test_github.py +++ b/tests/unit/oidc/forms/test_github.py @@ -18,12 +18,19 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import github -from warehouse.packaging.interfaces import ProjectNameUnavailableReason +from warehouse.packaging.models import ( + ProjectNameUnavailableExisting, + ProjectNameUnavailableInvalid, + ProjectNameUnavailableProhibited, + ProjectNameUnavailableSimilar, + ProjectNameUnavailableStdlib, +) class TestPendingGitHubPublisherForm: def test_validate(self, monkeypatch): route_url = pretend.stub() + user = pretend.stub() def check_project_name(name): return None # Name is available. @@ -41,6 +48,7 @@ def check_project_name(name): api_token=pretend.stub(), route_url=route_url, check_project_name=check_project_name, + user=user, ) # We're testing only the basic validation here. @@ -49,34 +57,61 @@ def check_project_name(name): assert form._check_project_name == check_project_name assert form._route_url == route_url + assert form._user == user assert form.validate() - def test_validate_project_name_already_in_use(self, pyramid_config): + @pytest.mark.parametrize("is_project_owner", [True, False]) + def test_validate_project_name_already_in_use( + self, pyramid_config, is_project_owner + ): route_url = pretend.call_recorder(lambda *args, **kwargs: "") + user = pretend.stub() + owners = [] + if is_project_owner: + owners.append(user) form = github.PendingGitHubPublisherForm( api_token="fake-token", route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableReason.AlreadyExists, + check_project_name=lambda name: ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ), + user=user, ) field = pretend.stub(data="some-project") with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"github"}}, - ) - ] - @pytest.mark.parametrize("reason", list(ProjectNameUnavailableReason)) + if is_project_owner: + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"github"}}, + ) + ] + else: + assert route_url.calls == [] + + @pytest.mark.parametrize( + "reason", + [ + ProjectNameUnavailableExisting(pretend.stub(owners=[pretend.stub()])), + ProjectNameUnavailableInvalid(), + ProjectNameUnavailableStdlib(), + ProjectNameUnavailableProhibited(), + ProjectNameUnavailableSimilar(pretend.stub()), + ], + ) def test_validate_project_name_unavailable(self, reason, pyramid_config): form = github.PendingGitHubPublisherForm( api_token="fake-token", route_url=pretend.call_recorder(lambda *args, **kwargs: ""), check_project_name=lambda name: reason, + user=pretend.stub(), ) field = pretend.stub(data="some-project") diff --git a/tests/unit/oidc/forms/test_gitlab.py b/tests/unit/oidc/forms/test_gitlab.py index b6737a2b1aa6..f037da669ab4 100644 --- a/tests/unit/oidc/forms/test_gitlab.py +++ b/tests/unit/oidc/forms/test_gitlab.py @@ -17,12 +17,13 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import gitlab -from warehouse.packaging.interfaces import ProjectNameUnavailableReason +from warehouse.packaging.models import ProjectNameUnavailableExisting class TestPendingGitLabPublisherForm: def test_validate(self, monkeypatch): route_url = pretend.stub() + user = pretend.stub() def check_project_name(name): return None # Name is available. @@ -36,32 +37,51 @@ def check_project_name(name): } ) form = gitlab.PendingGitLabPublisherForm( - MultiDict(data), route_url=route_url, check_project_name=check_project_name + MultiDict(data), + route_url=route_url, + check_project_name=check_project_name, + user=user, ) assert form._route_url == route_url assert form._check_project_name == check_project_name + assert form._user == user # We're testing only the basic validation here. assert form.validate() - def test_validate_project_name_already_in_use(self, pyramid_config): + @pytest.mark.parametrize("is_project_owner", [True, False]) + def test_validate_project_name_already_in_use( + self, pyramid_config, is_project_owner + ): + user = pretend.stub() + owners = [] + if is_project_owner: + owners.append(user) route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") form = gitlab.PendingGitLabPublisherForm( route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableReason.AlreadyExists, + check_project_name=lambda name: ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ), + user=user, ) field = pretend.stub(data="some-project") with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"gitlab"}}, - ) - ] + if is_project_owner: + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"gitlab"}}, + ) + ] + else: + assert route_url.calls == [] class TestGitLabPublisherForm: @@ -138,7 +158,6 @@ def test_validate_basic_invalid_fields(self, monkeypatch, data): ["invalid.git", "invalid.atom", "invalid--project"], ) def test_reserved_project_names(self, project_name): - data = MultiDict( { "namespace": "some", @@ -160,7 +179,6 @@ def test_reserved_project_names(self, project_name): ], ) def test_reserved_organization_names(self, namespace): - data = MultiDict( { "namespace": namespace, diff --git a/tests/unit/oidc/forms/test_google.py b/tests/unit/oidc/forms/test_google.py index 0da2163507f6..648f9048b672 100644 --- a/tests/unit/oidc/forms/test_google.py +++ b/tests/unit/oidc/forms/test_google.py @@ -17,12 +17,13 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import google -from warehouse.packaging.interfaces import ProjectNameUnavailableReason +from warehouse.packaging.models import ProjectNameUnavailableExisting class TestPendingGooglePublisherForm: def test_validate(self, monkeypatch): route_url = pretend.stub() + user = pretend.stub() def check_project_name(name): return None # Name is available. @@ -35,30 +36,49 @@ def check_project_name(name): } ) form = google.PendingGooglePublisherForm( - MultiDict(data), route_url=route_url, check_project_name=check_project_name + MultiDict(data), + route_url=route_url, + check_project_name=check_project_name, + user=user, ) assert form._check_project_name == check_project_name assert form._route_url == route_url + assert form._user == user assert form.validate() - def test_validate_project_name_already_in_use(self, pyramid_config): + @pytest.mark.parametrize("is_project_owner", [True, False]) + def test_validate_project_name_already_in_use( + self, pyramid_config, is_project_owner + ): + user = pretend.stub() + owners = [] + if is_project_owner: + owners.append(user) route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") form = google.PendingGooglePublisherForm( route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableReason.AlreadyExists, + check_project_name=lambda name: ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ), + user=user, ) field = pretend.stub(data="some-project") with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"google"}}, - ) - ] + if is_project_owner: + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"google"}}, + ) + ] + else: + assert route_url.calls == [] class TestGooglePublisherForm: diff --git a/tests/unit/packaging/test_services.py b/tests/unit/packaging/test_services.py index 9769fdae4838..10ed98d4536e 100644 --- a/tests/unit/packaging/test_services.py +++ b/tests/unit/packaging/test_services.py @@ -29,7 +29,13 @@ IFileStorage, IProjectService, ISimpleStorage, - ProjectNameUnavailableReason, +) +from warehouse.packaging.models import ( + ProjectNameUnavailableExisting, + ProjectNameUnavailableInvalid, + ProjectNameUnavailableProhibited, + ProjectNameUnavailableSimilar, + ProjectNameUnavailableStdlib, ) from warehouse.packaging.services import ( B2FileStorage, @@ -996,44 +1002,47 @@ def test_verify_service(self): def test_check_project_name_invalid(self, name): service = ProjectService(session=pretend.stub()) - assert service.check_project_name(name) is ProjectNameUnavailableReason.Invalid + assert isinstance( + service.check_project_name(name), ProjectNameUnavailableInvalid + ) @pytest.mark.parametrize("name", ["uu", "cgi", "nis", "mailcap"]) def test_check_project_name_stdlib(self, name): service = ProjectService(session=pretend.stub()) - assert service.check_project_name(name) is ProjectNameUnavailableReason.Stdlib + assert isinstance( + service.check_project_name(name), ProjectNameUnavailableStdlib + ) def test_check_project_name_already_exists(self, db_session): service = ProjectService(session=db_session) ProjectFactory.create(name="foo") - assert ( - service.check_project_name("foo") - is ProjectNameUnavailableReason.AlreadyExists + assert isinstance( + service.check_project_name("foo"), ProjectNameUnavailableExisting ) - assert ( - service.check_project_name("Foo") - is ProjectNameUnavailableReason.AlreadyExists + assert isinstance( + service.check_project_name("Foo"), + ProjectNameUnavailableExisting, ) def test_check_project_name_prohibited(self, db_session): service = ProjectService(session=db_session) ProhibitedProjectFactory.create(name="foo") - assert ( - service.check_project_name("foo") is ProjectNameUnavailableReason.Prohibited + assert isinstance( + service.check_project_name("foo"), ProjectNameUnavailableProhibited ) - assert ( - service.check_project_name("Foo") is ProjectNameUnavailableReason.Prohibited + assert isinstance( + service.check_project_name("Foo"), ProjectNameUnavailableProhibited ) def test_check_project_name_too_similar(self, db_session): service = ProjectService(session=db_session) ProjectFactory.create(name="f00") - assert ( - service.check_project_name("foo") is ProjectNameUnavailableReason.TooSimilar + assert isinstance( + service.check_project_name("foo"), ProjectNameUnavailableSimilar ) def test_check_project_name_ok(self, db_session): diff --git a/warehouse/accounts/views.py b/warehouse/accounts/views.py index aefd44e0a97e..a31e16c584c0 100644 --- a/warehouse/accounts/views.py +++ b/warehouse/accounts/views.py @@ -1511,21 +1511,25 @@ def __init__(self, request): api_token=self.request.registry.settings.get("github.token"), route_url=self.request.route_url, check_project_name=self.project_service.check_project_name, + user=request.user, ) self.pending_gitlab_publisher_form = PendingGitLabPublisherForm( self.request.POST, route_url=self.request.route_url, check_project_name=self.project_service.check_project_name, + user=request.user, ) self.pending_google_publisher_form = PendingGooglePublisherForm( self.request.POST, route_url=self.request.route_url, check_project_name=self.project_service.check_project_name, + user=request.user, ) self.pending_activestate_publisher_form = PendingActiveStatePublisherForm( self.request.POST, route_url=self.request.route_url, check_project_name=self.project_service.check_project_name, + user=request.user, ) @property diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index a344a9e404e0..21d88ccda067 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -296,28 +296,28 @@ msgstr "" msgid "You are now ${role} of the '${project_name}' project." msgstr "" -#: warehouse/accounts/views.py:1589 warehouse/accounts/views.py:1832 +#: warehouse/accounts/views.py:1593 warehouse/accounts/views.py:1836 #: warehouse/manage/views/__init__.py:1417 msgid "" "Trusted publishing is temporarily disabled. See https://pypi.org/help" "#admin-intervention for details." msgstr "" -#: warehouse/accounts/views.py:1610 +#: warehouse/accounts/views.py:1614 msgid "disabled. See https://pypi.org/help#admin-intervention for details." msgstr "" -#: warehouse/accounts/views.py:1626 +#: warehouse/accounts/views.py:1630 msgid "" "You must have a verified email in order to register a pending trusted " "publisher. See https://pypi.org/help#openid-connect for details." msgstr "" -#: warehouse/accounts/views.py:1639 +#: warehouse/accounts/views.py:1643 msgid "You can't register more than 3 pending trusted publishers at once." msgstr "" -#: warehouse/accounts/views.py:1655 warehouse/manage/views/__init__.py:1586 +#: warehouse/accounts/views.py:1659 warehouse/manage/views/__init__.py:1586 #: warehouse/manage/views/__init__.py:1699 #: warehouse/manage/views/__init__.py:1811 #: warehouse/manage/views/__init__.py:1921 @@ -326,29 +326,29 @@ msgid "" "again later." msgstr "" -#: warehouse/accounts/views.py:1665 warehouse/manage/views/__init__.py:1599 +#: warehouse/accounts/views.py:1669 warehouse/manage/views/__init__.py:1599 #: warehouse/manage/views/__init__.py:1712 #: warehouse/manage/views/__init__.py:1824 #: warehouse/manage/views/__init__.py:1934 msgid "The trusted publisher could not be registered" msgstr "" -#: warehouse/accounts/views.py:1680 +#: warehouse/accounts/views.py:1684 msgid "" "This trusted publisher has already been registered. Please contact PyPI's" " admins if this wasn't intentional." msgstr "" -#: warehouse/accounts/views.py:1707 +#: warehouse/accounts/views.py:1711 msgid "Registered a new pending publisher to create " msgstr "" -#: warehouse/accounts/views.py:1845 warehouse/accounts/views.py:1858 -#: warehouse/accounts/views.py:1865 +#: warehouse/accounts/views.py:1849 warehouse/accounts/views.py:1862 +#: warehouse/accounts/views.py:1869 msgid "Invalid publisher ID" msgstr "" -#: warehouse/accounts/views.py:1872 +#: warehouse/accounts/views.py:1876 msgid "Removed trusted publisher for project " msgstr "" @@ -384,7 +384,7 @@ msgstr "" msgid "Select project" msgstr "" -#: warehouse/manage/forms.py:506 warehouse/oidc/forms/_core.py:23 +#: warehouse/manage/forms.py:506 warehouse/oidc/forms/_core.py:29 #: warehouse/oidc/forms/gitlab.py:57 msgid "Specify project name" msgstr "" @@ -653,41 +653,46 @@ msgstr "" msgid "Expired invitation for '${username}' deleted." msgstr "" -#: warehouse/oidc/forms/_core.py:25 warehouse/oidc/forms/_core.py:35 +#: warehouse/oidc/forms/_core.py:31 warehouse/oidc/forms/_core.py:41 #: warehouse/oidc/forms/gitlab.py:60 warehouse/oidc/forms/gitlab.py:64 msgid "Invalid project name" msgstr "" -#: warehouse/oidc/forms/_core.py:50 +#: warehouse/oidc/forms/_core.py:61 #, python-brace-format msgid "" "This project already exists: use the project's publishing settings here to create a Trusted Publisher for it." msgstr "" -#: warehouse/oidc/forms/_core.py:59 +#: warehouse/oidc/forms/_core.py:70 +msgid "This project already exists." +msgstr "" + +#: warehouse/oidc/forms/_core.py:75 msgid "This project name isn't allowed" msgstr "" -#: warehouse/oidc/forms/_core.py:63 -msgid "This project name is too similar to an existing project" +#: warehouse/oidc/forms/_core.py:80 +#, python-brace-format +msgid "This project name is too similar to an existing project named '${name}'" msgstr "" -#: warehouse/oidc/forms/_core.py:68 +#: warehouse/oidc/forms/_core.py:88 msgid "" "This project name isn't allowed (conflict with the Python standard " "library module name)" msgstr "" -#: warehouse/oidc/forms/_core.py:84 warehouse/oidc/forms/_core.py:95 +#: warehouse/oidc/forms/_core.py:104 warehouse/oidc/forms/_core.py:115 msgid "Specify a publisher ID" msgstr "" -#: warehouse/oidc/forms/_core.py:85 warehouse/oidc/forms/_core.py:96 +#: warehouse/oidc/forms/_core.py:105 warehouse/oidc/forms/_core.py:116 msgid "Publisher must be specified by ID" msgstr "" -#: warehouse/oidc/forms/_core.py:101 +#: warehouse/oidc/forms/_core.py:121 msgid "Specify an environment name" msgstr "" diff --git a/warehouse/oidc/forms/_core.py b/warehouse/oidc/forms/_core.py index 39ab2a073c4e..b03a89317f0a 100644 --- a/warehouse/oidc/forms/_core.py +++ b/warehouse/oidc/forms/_core.py @@ -13,7 +13,13 @@ import wtforms from warehouse.i18n import localize as _ -from warehouse.packaging.interfaces import ProjectNameUnavailableReason +from warehouse.packaging.models import ( + ProjectNameUnavailableExisting, + ProjectNameUnavailableInvalid, + ProjectNameUnavailableProhibited, + ProjectNameUnavailableSimilar, + ProjectNameUnavailableStdlib, +) from warehouse.utils.project import PROJECT_NAME_RE @@ -31,38 +37,52 @@ def validate_project_name(self, field): project_name = field.data match self._check_project_name(project_name): - case ProjectNameUnavailableReason.Invalid: + case ProjectNameUnavailableInvalid(): raise wtforms.validators.ValidationError(_("Invalid project name")) - case ProjectNameUnavailableReason.AlreadyExists: - url_params = {name: value for name, value in self.data.items() if value} - url_params["provider"] = {self.provider} - url = self._route_url( - "manage.project.settings.publishing", - project_name=project_name, - _query=url_params, - ) + case ProjectNameUnavailableExisting(existing_project=existing_project): + # If the user owns the existing project, the error message includes a + # link to the project settings that the user can modify. + if self._user in existing_project.owners: + url_params = { + name: value for name, value in self.data.items() if value + } + url_params["provider"] = {self.provider} + url = self._route_url( + "manage.project.settings.publishing", + project_name=project_name, + _query=url_params, + ) - # We mark the error message as safe, so that the HTML hyperlink is - # not escaped by Jinja - raise wtforms.validators.ValidationError( - markupsafe.Markup( - _( - "This project already exists: use the project's publishing" - " settings here to create a Trusted" - " Publisher for it.", - mapping={"url": url}, + # We mark the error message as safe, so that the HTML hyperlink is + # not escaped by Jinja + raise wtforms.validators.ValidationError( + markupsafe.Markup( + _( + "This project already exists: use the project's " + "publishing settings here to " + "create a Trusted Publisher for it.", + mapping={"url": url}, + ) ) ) - ) - case ProjectNameUnavailableReason.Prohibited: + else: + raise wtforms.validators.ValidationError( + _("This project already exists.") + ) + + case ProjectNameUnavailableProhibited(): raise wtforms.validators.ValidationError( _("This project name isn't allowed") ) - case ProjectNameUnavailableReason.TooSimilar: + case ProjectNameUnavailableSimilar(similar_project=similar_project): raise wtforms.validators.ValidationError( - _("This project name is too similar to an existing project") + _( + "This project name is too similar to an existing project " + "named '${name}'", + mapping={"name": similar_project.name}, + ) ) - case ProjectNameUnavailableReason.Stdlib: + case ProjectNameUnavailableStdlib(): raise wtforms.validators.ValidationError( _( "This project name isn't allowed (conflict with the Python" diff --git a/warehouse/oidc/forms/activestate.py b/warehouse/oidc/forms/activestate.py index 46b5336f7705..68569ffc3de4 100644 --- a/warehouse/oidc/forms/activestate.py +++ b/warehouse/oidc/forms/activestate.py @@ -191,10 +191,11 @@ def validate_actor(self, field): class PendingActiveStatePublisherForm(ActiveStatePublisherBase, PendingPublisherMixin): __params__ = ActiveStatePublisherBase.__params__ + ["project_name"] - def __init__(self, *args, route_url, check_project_name, **kwargs): + def __init__(self, *args, route_url, check_project_name, user, **kwargs): super().__init__(*args, **kwargs) self._route_url = route_url self._check_project_name = check_project_name + self._user = user @property def provider(self) -> str: diff --git a/warehouse/oidc/forms/github.py b/warehouse/oidc/forms/github.py index 1ee524b5ea42..dd691aae9ef9 100644 --- a/warehouse/oidc/forms/github.py +++ b/warehouse/oidc/forms/github.py @@ -168,10 +168,11 @@ def normalized_environment(self): class PendingGitHubPublisherForm(GitHubPublisherBase, PendingPublisherMixin): __params__ = GitHubPublisherBase.__params__ + ["project_name"] - def __init__(self, *args, route_url, check_project_name, **kwargs): + def __init__(self, *args, route_url, check_project_name, user, **kwargs): super().__init__(*args, **kwargs) self._route_url = route_url self._check_project_name = check_project_name + self._user = user @property def provider(self) -> str: diff --git a/warehouse/oidc/forms/gitlab.py b/warehouse/oidc/forms/gitlab.py index 86fb08f0a936..28a30c0224c8 100644 --- a/warehouse/oidc/forms/gitlab.py +++ b/warehouse/oidc/forms/gitlab.py @@ -115,10 +115,11 @@ def normalized_environment(self): class PendingGitLabPublisherForm(GitLabPublisherBase, PendingPublisherMixin): __params__ = GitLabPublisherBase.__params__ + ["project_name"] - def __init__(self, *args, route_url, check_project_name, **kwargs): + def __init__(self, *args, route_url, check_project_name, user, **kwargs): super().__init__(*args, **kwargs) self._route_url = route_url self._check_project_name = check_project_name + self._user = user @property def provider(self) -> str: diff --git a/warehouse/oidc/forms/google.py b/warehouse/oidc/forms/google.py index 13b8aa30b6fc..98882f2274c6 100644 --- a/warehouse/oidc/forms/google.py +++ b/warehouse/oidc/forms/google.py @@ -34,10 +34,11 @@ def __init__(self, *args, **kwargs): class PendingGooglePublisherForm(GooglePublisherBase, PendingPublisherMixin): __params__ = GooglePublisherBase.__params__ + ["project_name"] - def __init__(self, *args, route_url, check_project_name, **kwargs): + def __init__(self, *args, route_url, check_project_name, user, **kwargs): super().__init__(*args, **kwargs) self._route_url = route_url self._check_project_name = check_project_name + self._user = user @property def provider(self) -> str: diff --git a/warehouse/packaging/interfaces.py b/warehouse/packaging/interfaces.py index ecafdf6c217a..bf66a29892e5 100644 --- a/warehouse/packaging/interfaces.py +++ b/warehouse/packaging/interfaces.py @@ -14,6 +14,7 @@ from zope.interface import Interface +from warehouse.packaging.models import Project from warehouse.rate_limiting.interfaces import RateLimiterException @@ -75,14 +76,6 @@ def remove_by_prefix(prefix): """ -class ProjectNameUnavailableReason(enum.Enum): - Invalid = "invalid" - Stdlib = "stdlib" - AlreadyExists = "already-exists" - Prohibited = "prohibited" - TooSimilar = "too-similar" - - class IProjectService(Interface): def check_project_name(name): """ diff --git a/warehouse/packaging/models.py b/warehouse/packaging/models.py index 4a5d03a9f8e2..13a0a4c5fd91 100644 --- a/warehouse/packaging/models.py +++ b/warehouse/packaging/models.py @@ -1154,3 +1154,34 @@ class AlternateRepository(db.Model): name: Mapped[str] url: Mapped[str] description: Mapped[str] + + +class ProjectNameUnavailableInvalid: + pass + + +class ProjectNameUnavailableStdlib: + pass + + +class ProjectNameUnavailableExisting: + def __init__(self, existing_project: Project): + self.existing_project: Project = existing_project + + +class ProjectNameUnavailableProhibited: + pass + + +class ProjectNameUnavailableSimilar: + def __init__(self, similar_project: Project): + self.similar_project: Project = similar_project + + +ProjectNameUnavailableError = ( + ProjectNameUnavailableInvalid + | ProjectNameUnavailableStdlib + | ProjectNameUnavailableExisting + | ProjectNameUnavailableProhibited + | ProjectNameUnavailableSimilar +) diff --git a/warehouse/packaging/services.py b/warehouse/packaging/services.py index b766cb3b0474..ecb864da4135 100644 --- a/warehouse/packaging/services.py +++ b/warehouse/packaging/services.py @@ -43,13 +43,18 @@ IFileStorage, IProjectService, ISimpleStorage, - ProjectNameUnavailableReason, TooManyProjectsCreated, ) from warehouse.packaging.models import ( JournalEntry, ProhibitedProjectName, Project, + ProjectNameUnavailableError, + ProjectNameUnavailableExisting, + ProjectNameUnavailableInvalid, + ProjectNameUnavailableProhibited, + ProjectNameUnavailableSimilar, + ProjectNameUnavailableStdlib, Role, ) from warehouse.rate_limiting import DummyRateLimiter, IRateLimiter @@ -443,32 +448,36 @@ def _hit_ratelimits(self, request, creator): self.ratelimiters["project.create.user"].hit(creator.id) self.ratelimiters["project.create.ip"].hit(request.remote_addr) - def check_project_name(self, name: str) -> ProjectNameUnavailableReason | None: + def check_project_name(self, name: str) -> ProjectNameUnavailableError | None: if not PROJECT_NAME_RE.match(name): - return ProjectNameUnavailableReason.Invalid + return ProjectNameUnavailableInvalid() # Also check for collisions with Python Standard Library modules. if canonicalize_name(name) in STDLIB_PROHIBITED: - return ProjectNameUnavailableReason.Stdlib + return ProjectNameUnavailableStdlib() - if self.db.query( - exists().where(Project.normalized_name == func.normalize_pep426_name(name)) - ).scalar(): - return ProjectNameUnavailableReason.AlreadyExists + if ( + existing_project := self.db.query(Project) + .where(Project.normalized_name == func.normalize_pep426_name(name)) + .scalar() + ): + return ProjectNameUnavailableExisting(existing_project) if self.db.query( exists().where( ProhibitedProjectName.name == func.normalize_pep426_name(name) ) ).scalar(): - return ProjectNameUnavailableReason.Prohibited + return ProjectNameUnavailableProhibited() - if self.db.query( - exists().where( + if ( + similar_project := self.db.query(Project) + .where( func.ultranormalize_name(Project.name) == func.ultranormalize_name(name) ) - ).scalar(): - return ProjectNameUnavailableReason.TooSimilar + .scalar() + ): + return ProjectNameUnavailableSimilar(similar_project) return None @@ -491,9 +500,9 @@ def create_project( # Verify that the project name is both valid and currently available. match self.check_project_name(name): - case ProjectNameUnavailableReason.Invalid: + case ProjectNameUnavailableInvalid(): raise HTTPBadRequest(f"The name {name!r} is invalid.") - case ProjectNameUnavailableReason.AlreadyExists: + case ProjectNameUnavailableExisting(): # Found existing project with conflicting name. raise HTTPConflict( ( @@ -504,7 +513,7 @@ def create_project( projecthelp=request.help_url(_anchor="project-name"), ), ) from None - case ProjectNameUnavailableReason.Prohibited: + case ProjectNameUnavailableProhibited(): raise HTTPBadRequest( ( "The name {name!r} isn't allowed. " @@ -514,17 +523,18 @@ def create_project( projecthelp=request.help_url(_anchor="project-name"), ), ) from None - case ProjectNameUnavailableReason.TooSimilar: + case ProjectNameUnavailableSimilar(similar_project=similar_project): raise HTTPBadRequest( ( - "The name {name!r} is too similar to an existing project. " - "See {projecthelp} for more information." + "The name {name!r} is too similar to an existing project named " + "{similar_name!r}. See {projecthelp} for more information." ).format( name=name, + similar_name=similar_project.name, projecthelp=request.help_url(_anchor="project-name"), ), ) from None - case ProjectNameUnavailableReason.Stdlib: + case ProjectNameUnavailableStdlib(): raise HTTPBadRequest( ( "The name {name!r} isn't allowed (conflict with Python " From 526af658f78c8a947dac2b83715ef9017a8e7f8a Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Mon, 13 Jan 2025 23:34:52 +0100 Subject: [PATCH 2/8] Fix test error Signed-off-by: Facundo Tuesca --- tests/unit/oidc/forms/test_github.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/oidc/forms/test_github.py b/tests/unit/oidc/forms/test_github.py index 2b71cf3f402d..980d3fc9dbbe 100644 --- a/tests/unit/oidc/forms/test_github.py +++ b/tests/unit/oidc/forms/test_github.py @@ -103,7 +103,7 @@ def test_validate_project_name_already_in_use( ProjectNameUnavailableInvalid(), ProjectNameUnavailableStdlib(), ProjectNameUnavailableProhibited(), - ProjectNameUnavailableSimilar(pretend.stub()), + ProjectNameUnavailableSimilar(pretend.stub(name="pkg_name")), ], ) def test_validate_project_name_unavailable(self, reason, pyramid_config): From 91e6b0d30096cd3b14e5a8a1a988fad2e3af709f Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Tue, 14 Jan 2025 14:37:18 +0100 Subject: [PATCH 3/8] Fix name checking Fix the check for projects with too-similar names when there is more than one existing project with the same ultranormalized name. Signed-off-by: Facundo Tuesca --- tests/unit/packaging/test_services.py | 20 ++++++++++++++++---- warehouse/packaging/services.py | 4 ++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/unit/packaging/test_services.py b/tests/unit/packaging/test_services.py index 10ed98d4536e..d0d7c04fba34 100644 --- a/tests/unit/packaging/test_services.py +++ b/tests/unit/packaging/test_services.py @@ -1016,11 +1016,11 @@ def test_check_project_name_stdlib(self, name): def test_check_project_name_already_exists(self, db_session): service = ProjectService(session=db_session) - ProjectFactory.create(name="foo") + project = ProjectFactory.create(name="foo") - assert isinstance( - service.check_project_name("foo"), ProjectNameUnavailableExisting - ) + unavailable_error = service.check_project_name("foo") + assert isinstance(unavailable_error, ProjectNameUnavailableExisting) + assert unavailable_error.existing_project == project assert isinstance( service.check_project_name("Foo"), ProjectNameUnavailableExisting, @@ -1045,6 +1045,18 @@ def test_check_project_name_too_similar(self, db_session): service.check_project_name("foo"), ProjectNameUnavailableSimilar ) + def test_check_project_name_too_similar_multiple_existing(self, db_session): + service = ProjectService(session=db_session) + project1 = ProjectFactory.create(name="f00") + project2 = ProjectFactory.create(name="f0o") + + unavailable_error = service.check_project_name("foo") + assert isinstance(unavailable_error, ProjectNameUnavailableSimilar) + assert ( + unavailable_error.similar_project == project1 + or unavailable_error.similar_project == project2 + ) + def test_check_project_name_ok(self, db_session): service = ProjectService(session=db_session) diff --git a/warehouse/packaging/services.py b/warehouse/packaging/services.py index ecb864da4135..6e420a11b3ed 100644 --- a/warehouse/packaging/services.py +++ b/warehouse/packaging/services.py @@ -459,7 +459,7 @@ def check_project_name(self, name: str) -> ProjectNameUnavailableError | None: if ( existing_project := self.db.query(Project) .where(Project.normalized_name == func.normalize_pep426_name(name)) - .scalar() + .first() ): return ProjectNameUnavailableExisting(existing_project) @@ -475,7 +475,7 @@ def check_project_name(self, name: str) -> ProjectNameUnavailableError | None: .where( func.ultranormalize_name(Project.name) == func.ultranormalize_name(name) ) - .scalar() + .first() ): return ProjectNameUnavailableSimilar(similar_project) From 361a1127663c83255feff77a2ecbbbed274c5d45 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Tue, 14 Jan 2025 20:15:41 +0100 Subject: [PATCH 4/8] Remove similar name from error messages Signed-off-by: Facundo Tuesca --- tests/unit/forklift/test_legacy.py | 2 +- warehouse/locale/messages.pot | 13 ++++++------- warehouse/oidc/forms/_core.py | 8 ++------ warehouse/packaging/services.py | 7 +++---- 4 files changed, 12 insertions(+), 18 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 776656e0d083..3ba814a87bef 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -907,7 +907,7 @@ def test_fails_with_ultranormalized_names( assert resp.status_code == 400 assert resp.status == ( - "400 The name {!r} is too similar to an existing project named 'toasting'. " + "400 The name {!r} is too similar to an existing project. " "See /the/help/url/ for more information." ).format(conflicting_name) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index 21d88ccda067..1aa7798285db 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -673,26 +673,25 @@ msgstr "" msgid "This project name isn't allowed" msgstr "" -#: warehouse/oidc/forms/_core.py:80 -#, python-brace-format -msgid "This project name is too similar to an existing project named '${name}'" +#: warehouse/oidc/forms/_core.py:79 +msgid "This project name is too similar to an existing project" msgstr "" -#: warehouse/oidc/forms/_core.py:88 +#: warehouse/oidc/forms/_core.py:84 msgid "" "This project name isn't allowed (conflict with the Python standard " "library module name)" msgstr "" -#: warehouse/oidc/forms/_core.py:104 warehouse/oidc/forms/_core.py:115 +#: warehouse/oidc/forms/_core.py:100 warehouse/oidc/forms/_core.py:111 msgid "Specify a publisher ID" msgstr "" -#: warehouse/oidc/forms/_core.py:105 warehouse/oidc/forms/_core.py:116 +#: warehouse/oidc/forms/_core.py:101 warehouse/oidc/forms/_core.py:112 msgid "Publisher must be specified by ID" msgstr "" -#: warehouse/oidc/forms/_core.py:121 +#: warehouse/oidc/forms/_core.py:117 msgid "Specify an environment name" msgstr "" diff --git a/warehouse/oidc/forms/_core.py b/warehouse/oidc/forms/_core.py index b03a89317f0a..9dfdbd1b540c 100644 --- a/warehouse/oidc/forms/_core.py +++ b/warehouse/oidc/forms/_core.py @@ -74,13 +74,9 @@ def validate_project_name(self, field): raise wtforms.validators.ValidationError( _("This project name isn't allowed") ) - case ProjectNameUnavailableSimilar(similar_project=similar_project): + case ProjectNameUnavailableSimilar(): raise wtforms.validators.ValidationError( - _( - "This project name is too similar to an existing project " - "named '${name}'", - mapping={"name": similar_project.name}, - ) + _("This project name is too similar to an existing project") ) case ProjectNameUnavailableStdlib(): raise wtforms.validators.ValidationError( diff --git a/warehouse/packaging/services.py b/warehouse/packaging/services.py index 6e420a11b3ed..67c05311d7b7 100644 --- a/warehouse/packaging/services.py +++ b/warehouse/packaging/services.py @@ -523,14 +523,13 @@ def create_project( projecthelp=request.help_url(_anchor="project-name"), ), ) from None - case ProjectNameUnavailableSimilar(similar_project=similar_project): + case ProjectNameUnavailableSimilar(): raise HTTPBadRequest( ( - "The name {name!r} is too similar to an existing project named " - "{similar_name!r}. See {projecthelp} for more information." + "The name {name!r} is too similar to an existing project. " + "See {projecthelp} for more information." ).format( name=name, - similar_name=similar_project.name, projecthelp=request.help_url(_anchor="project-name"), ), ) from None From 5a4f5b9b10e09c7a8298a8187b75faa030c77967 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Fri, 31 Jan 2025 21:01:51 +0100 Subject: [PATCH 5/8] Move errors back to interfaces.py Signed-off-by: Facundo Tuesca --- tests/unit/oidc/forms/test_activestate.py | 2 +- tests/unit/oidc/forms/test_github.py | 2 +- tests/unit/oidc/forms/test_gitlab.py | 2 +- tests/unit/oidc/forms/test_google.py | 2 +- tests/unit/packaging/test_services.py | 2 -- warehouse/oidc/forms/_core.py | 2 +- warehouse/packaging/interfaces.py | 31 +++++++++++++++++++++++ warehouse/packaging/models.py | 31 ----------------------- warehouse/packaging/services.py | 11 ++++---- 9 files changed, 41 insertions(+), 44 deletions(-) diff --git a/tests/unit/oidc/forms/test_activestate.py b/tests/unit/oidc/forms/test_activestate.py index 67abf8eaf71e..bcaf4819f86b 100644 --- a/tests/unit/oidc/forms/test_activestate.py +++ b/tests/unit/oidc/forms/test_activestate.py @@ -19,7 +19,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import activestate -from warehouse.packaging.models import ProjectNameUnavailableExisting +from warehouse.packaging.interfaces import ProjectNameUnavailableExisting fake_username = "some-username" fake_org_name = "some-org" diff --git a/tests/unit/oidc/forms/test_github.py b/tests/unit/oidc/forms/test_github.py index 980d3fc9dbbe..71181cfcddce 100644 --- a/tests/unit/oidc/forms/test_github.py +++ b/tests/unit/oidc/forms/test_github.py @@ -18,7 +18,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import github -from warehouse.packaging.models import ( +from warehouse.packaging.interfaces import ( ProjectNameUnavailableExisting, ProjectNameUnavailableInvalid, ProjectNameUnavailableProhibited, diff --git a/tests/unit/oidc/forms/test_gitlab.py b/tests/unit/oidc/forms/test_gitlab.py index f037da669ab4..de95baeecbe6 100644 --- a/tests/unit/oidc/forms/test_gitlab.py +++ b/tests/unit/oidc/forms/test_gitlab.py @@ -17,7 +17,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import gitlab -from warehouse.packaging.models import ProjectNameUnavailableExisting +from warehouse.packaging.interfaces import ProjectNameUnavailableExisting class TestPendingGitLabPublisherForm: diff --git a/tests/unit/oidc/forms/test_google.py b/tests/unit/oidc/forms/test_google.py index 648f9048b672..002900ec1f9d 100644 --- a/tests/unit/oidc/forms/test_google.py +++ b/tests/unit/oidc/forms/test_google.py @@ -17,7 +17,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import google -from warehouse.packaging.models import ProjectNameUnavailableExisting +from warehouse.packaging.interfaces import ProjectNameUnavailableExisting class TestPendingGooglePublisherForm: diff --git a/tests/unit/packaging/test_services.py b/tests/unit/packaging/test_services.py index d0d7c04fba34..c87e502f9eaa 100644 --- a/tests/unit/packaging/test_services.py +++ b/tests/unit/packaging/test_services.py @@ -29,8 +29,6 @@ IFileStorage, IProjectService, ISimpleStorage, -) -from warehouse.packaging.models import ( ProjectNameUnavailableExisting, ProjectNameUnavailableInvalid, ProjectNameUnavailableProhibited, diff --git a/warehouse/oidc/forms/_core.py b/warehouse/oidc/forms/_core.py index 9dfdbd1b540c..d973a1b95c10 100644 --- a/warehouse/oidc/forms/_core.py +++ b/warehouse/oidc/forms/_core.py @@ -13,7 +13,7 @@ import wtforms from warehouse.i18n import localize as _ -from warehouse.packaging.models import ( +from warehouse.packaging.interfaces import ( ProjectNameUnavailableExisting, ProjectNameUnavailableInvalid, ProjectNameUnavailableProhibited, diff --git a/warehouse/packaging/interfaces.py b/warehouse/packaging/interfaces.py index bf66a29892e5..209c1b1931cf 100644 --- a/warehouse/packaging/interfaces.py +++ b/warehouse/packaging/interfaces.py @@ -89,3 +89,34 @@ def create_project(name, creator, request, *, creator_is_owner=True): If `creator_is_owner`, a `Role` is also added to the project marking `creator` as a project owner. """ + + +class ProjectNameUnavailableInvalid: + pass + + +class ProjectNameUnavailableStdlib: + pass + + +class ProjectNameUnavailableExisting: + def __init__(self, existing_project: Project): + self.existing_project: Project = existing_project + + +class ProjectNameUnavailableProhibited: + pass + + +class ProjectNameUnavailableSimilar: + def __init__(self, similar_project: Project): + self.similar_project: Project = similar_project + + +ProjectNameUnavailableError = ( + ProjectNameUnavailableInvalid + | ProjectNameUnavailableStdlib + | ProjectNameUnavailableExisting + | ProjectNameUnavailableProhibited + | ProjectNameUnavailableSimilar +) diff --git a/warehouse/packaging/models.py b/warehouse/packaging/models.py index 13a0a4c5fd91..4a5d03a9f8e2 100644 --- a/warehouse/packaging/models.py +++ b/warehouse/packaging/models.py @@ -1154,34 +1154,3 @@ class AlternateRepository(db.Model): name: Mapped[str] url: Mapped[str] description: Mapped[str] - - -class ProjectNameUnavailableInvalid: - pass - - -class ProjectNameUnavailableStdlib: - pass - - -class ProjectNameUnavailableExisting: - def __init__(self, existing_project: Project): - self.existing_project: Project = existing_project - - -class ProjectNameUnavailableProhibited: - pass - - -class ProjectNameUnavailableSimilar: - def __init__(self, similar_project: Project): - self.similar_project: Project = similar_project - - -ProjectNameUnavailableError = ( - ProjectNameUnavailableInvalid - | ProjectNameUnavailableStdlib - | ProjectNameUnavailableExisting - | ProjectNameUnavailableProhibited - | ProjectNameUnavailableSimilar -) diff --git a/warehouse/packaging/services.py b/warehouse/packaging/services.py index 67c05311d7b7..e14446c3419b 100644 --- a/warehouse/packaging/services.py +++ b/warehouse/packaging/services.py @@ -43,18 +43,17 @@ IFileStorage, IProjectService, ISimpleStorage, + ProjectNameUnavailableExisting, + ProjectNameUnavailableInvalid, + ProjectNameUnavailableProhibited, + ProjectNameUnavailableSimilar, + ProjectNameUnavailableStdlib, TooManyProjectsCreated, ) from warehouse.packaging.models import ( JournalEntry, ProhibitedProjectName, Project, - ProjectNameUnavailableError, - ProjectNameUnavailableExisting, - ProjectNameUnavailableInvalid, - ProjectNameUnavailableProhibited, - ProjectNameUnavailableSimilar, - ProjectNameUnavailableStdlib, Role, ) from warehouse.rate_limiting import DummyRateLimiter, IRateLimiter From f940ab84073f3b0ba32ff6f86e68a7d54e29e088 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Fri, 31 Jan 2025 21:07:35 +0100 Subject: [PATCH 6/8] De-parametrize tests Signed-off-by: Facundo Tuesca --- tests/unit/oidc/forms/test_activestate.py | 50 +++++++++++++++-------- tests/unit/oidc/forms/test_github.py | 48 ++++++++++++++-------- tests/unit/oidc/forms/test_gitlab.py | 48 ++++++++++++++-------- tests/unit/oidc/forms/test_google.py | 47 +++++++++++++-------- 4 files changed, 121 insertions(+), 72 deletions(-) diff --git a/tests/unit/oidc/forms/test_activestate.py b/tests/unit/oidc/forms/test_activestate.py index bcaf4819f86b..6002654ae6c4 100644 --- a/tests/unit/oidc/forms/test_activestate.py +++ b/tests/unit/oidc/forms/test_activestate.py @@ -62,15 +62,40 @@ def check_project_name(name): assert form._route_url == route_url assert form.validate() - @pytest.mark.parametrize("is_project_owner", [True, False]) - def test_validate_project_name_already_in_use( - self, pyramid_config, is_project_owner - ): + def test_validate_project_name_already_in_use_owner(self, pyramid_config): + route_url = pretend.call_recorder(lambda *args, **kwargs: "") + user = pretend.stub() + owners = [user] + + def check_project_name(name): + return ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ) + + form = activestate.PendingActiveStatePublisherForm( + route_url=route_url, + check_project_name=check_project_name, + user=user, + ) + + field = pretend.stub(data="some-project") + with pytest.raises(wtforms.validators.ValidationError): + form.validate_project_name(field) + + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"activestate"}}, + ) + ] + + def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): route_url = pretend.call_recorder(lambda *args, **kwargs: "") user = pretend.stub() owners = [] - if is_project_owner: - owners.append(user) def check_project_name(name): return ProjectNameUnavailableExisting( @@ -87,18 +112,7 @@ def check_project_name(name): with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - if is_project_owner: - # The project settings URL is only shown in the error message if - # the user is the owner of the project - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"activestate"}}, - ) - ] - else: - assert route_url.calls == [] + assert route_url.calls == [] class TestActiveStatePublisherForm: diff --git a/tests/unit/oidc/forms/test_github.py b/tests/unit/oidc/forms/test_github.py index 71181cfcddce..09b858167ab0 100644 --- a/tests/unit/oidc/forms/test_github.py +++ b/tests/unit/oidc/forms/test_github.py @@ -60,15 +60,38 @@ def check_project_name(name): assert form._user == user assert form.validate() - @pytest.mark.parametrize("is_project_owner", [True, False]) - def test_validate_project_name_already_in_use( - self, pyramid_config, is_project_owner - ): + def test_validate_project_name_already_in_use_owner(self, pyramid_config): + route_url = pretend.call_recorder(lambda *args, **kwargs: "") + user = pretend.stub() + owners = [user] + + form = github.PendingGitHubPublisherForm( + api_token="fake-token", + route_url=route_url, + check_project_name=lambda name: ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ), + user=user, + ) + + field = pretend.stub(data="some-project") + with pytest.raises(wtforms.validators.ValidationError): + form.validate_project_name(field) + + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"github"}}, + ) + ] + + def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): route_url = pretend.call_recorder(lambda *args, **kwargs: "") user = pretend.stub() owners = [] - if is_project_owner: - owners.append(user) form = github.PendingGitHubPublisherForm( api_token="fake-token", @@ -83,18 +106,7 @@ def test_validate_project_name_already_in_use( with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - if is_project_owner: - # The project settings URL is only shown in the error message if - # the user is the owner of the project - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"github"}}, - ) - ] - else: - assert route_url.calls == [] + assert route_url.calls == [] @pytest.mark.parametrize( "reason", diff --git a/tests/unit/oidc/forms/test_gitlab.py b/tests/unit/oidc/forms/test_gitlab.py index de95baeecbe6..b0ec9a08f2ec 100644 --- a/tests/unit/oidc/forms/test_gitlab.py +++ b/tests/unit/oidc/forms/test_gitlab.py @@ -49,14 +49,36 @@ def check_project_name(name): # We're testing only the basic validation here. assert form.validate() - @pytest.mark.parametrize("is_project_owner", [True, False]) - def test_validate_project_name_already_in_use( - self, pyramid_config, is_project_owner - ): + def test_validate_project_name_already_in_use_owner(self, pyramid_config): + user = pretend.stub() + owners = [user] + route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") + + form = gitlab.PendingGitLabPublisherForm( + route_url=route_url, + check_project_name=lambda name: ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ), + user=user, + ) + + field = pretend.stub(data="some-project") + with pytest.raises(wtforms.validators.ValidationError): + form.validate_project_name(field) + + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"gitlab"}}, + ) + ] + + def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): user = pretend.stub() owners = [] - if is_project_owner: - owners.append(user) route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") form = gitlab.PendingGitLabPublisherForm( @@ -70,18 +92,8 @@ def test_validate_project_name_already_in_use( field = pretend.stub(data="some-project") with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - if is_project_owner: - # The project settings URL is only shown in the error message if - # the user is the owner of the project - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"gitlab"}}, - ) - ] - else: - assert route_url.calls == [] + + assert route_url.calls == [] class TestGitLabPublisherForm: diff --git a/tests/unit/oidc/forms/test_google.py b/tests/unit/oidc/forms/test_google.py index 002900ec1f9d..6de8d2a9b9d0 100644 --- a/tests/unit/oidc/forms/test_google.py +++ b/tests/unit/oidc/forms/test_google.py @@ -47,14 +47,35 @@ def check_project_name(name): assert form._user == user assert form.validate() - @pytest.mark.parametrize("is_project_owner", [True, False]) - def test_validate_project_name_already_in_use( - self, pyramid_config, is_project_owner - ): + def test_validate_project_name_already_in_use_owner(self, pyramid_config): + user = pretend.stub() + owners = [user] + route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") + form = google.PendingGooglePublisherForm( + route_url=route_url, + check_project_name=lambda name: ProjectNameUnavailableExisting( + existing_project=pretend.stub(owners=owners) + ), + user=user, + ) + + field = pretend.stub(data="some-project") + with pytest.raises(wtforms.validators.ValidationError): + form.validate_project_name(field) + + # The project settings URL is only shown in the error message if + # the user is the owner of the project + assert route_url.calls == [ + pretend.call( + "manage.project.settings.publishing", + project_name="some-project", + _query={"provider": {"google"}}, + ) + ] + + def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): user = pretend.stub() owners = [] - if is_project_owner: - owners.append(user) route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") form = google.PendingGooglePublisherForm( route_url=route_url, @@ -67,18 +88,8 @@ def test_validate_project_name_already_in_use( field = pretend.stub(data="some-project") with pytest.raises(wtforms.validators.ValidationError): form.validate_project_name(field) - if is_project_owner: - # The project settings URL is only shown in the error message if - # the user is the owner of the project - assert route_url.calls == [ - pretend.call( - "manage.project.settings.publishing", - project_name="some-project", - _query={"provider": {"google"}}, - ) - ] - else: - assert route_url.calls == [] + + assert route_url.calls == [] class TestGooglePublisherForm: From 05d0c0c4e02cd79b3efc7eea1ed6760474178a4f Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Fri, 31 Jan 2025 21:59:51 +0100 Subject: [PATCH 7/8] Use exceptions instead of errors Also change the TooSimilar error to store the conflicting project name instead of a reference to the entire project. Signed-off-by: Facundo Tuesca --- tests/unit/oidc/forms/test_github.py | 2 +- tests/unit/packaging/test_services.py | 50 ++++++------ warehouse/packaging/interfaces.py | 37 +++++---- warehouse/packaging/services.py | 113 +++++++++++++------------- 4 files changed, 102 insertions(+), 100 deletions(-) diff --git a/tests/unit/oidc/forms/test_github.py b/tests/unit/oidc/forms/test_github.py index 09b858167ab0..e0d55fbb2ecc 100644 --- a/tests/unit/oidc/forms/test_github.py +++ b/tests/unit/oidc/forms/test_github.py @@ -115,7 +115,7 @@ def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): ProjectNameUnavailableInvalid(), ProjectNameUnavailableStdlib(), ProjectNameUnavailableProhibited(), - ProjectNameUnavailableSimilar(pretend.stub(name="pkg_name")), + ProjectNameUnavailableSimilar(similar_project_name="pkg_name"), ], ) def test_validate_project_name_unavailable(self, reason, pyramid_config): diff --git a/tests/unit/packaging/test_services.py b/tests/unit/packaging/test_services.py index c87e502f9eaa..b39c3db59cb7 100644 --- a/tests/unit/packaging/test_services.py +++ b/tests/unit/packaging/test_services.py @@ -1000,65 +1000,61 @@ def test_verify_service(self): def test_check_project_name_invalid(self, name): service = ProjectService(session=pretend.stub()) - assert isinstance( - service.check_project_name(name), ProjectNameUnavailableInvalid - ) + with pytest.raises(ProjectNameUnavailableInvalid): + service.check_project_name(name) @pytest.mark.parametrize("name", ["uu", "cgi", "nis", "mailcap"]) def test_check_project_name_stdlib(self, name): service = ProjectService(session=pretend.stub()) - assert isinstance( - service.check_project_name(name), ProjectNameUnavailableStdlib - ) + with pytest.raises(ProjectNameUnavailableStdlib): + service.check_project_name(name) def test_check_project_name_already_exists(self, db_session): service = ProjectService(session=db_session) project = ProjectFactory.create(name="foo") - unavailable_error = service.check_project_name("foo") - assert isinstance(unavailable_error, ProjectNameUnavailableExisting) - assert unavailable_error.existing_project == project - assert isinstance( - service.check_project_name("Foo"), - ProjectNameUnavailableExisting, - ) + with pytest.raises(ProjectNameUnavailableExisting) as exc: + service.check_project_name("foo") + assert exc.value.existing_project == project + + with pytest.raises(ProjectNameUnavailableExisting): + service.check_project_name("Foo") def test_check_project_name_prohibited(self, db_session): service = ProjectService(session=db_session) ProhibitedProjectFactory.create(name="foo") - assert isinstance( - service.check_project_name("foo"), ProjectNameUnavailableProhibited - ) - assert isinstance( - service.check_project_name("Foo"), ProjectNameUnavailableProhibited - ) + with pytest.raises(ProjectNameUnavailableProhibited): + service.check_project_name("foo") + + with pytest.raises(ProjectNameUnavailableProhibited): + service.check_project_name("Foo") def test_check_project_name_too_similar(self, db_session): service = ProjectService(session=db_session) ProjectFactory.create(name="f00") - assert isinstance( - service.check_project_name("foo"), ProjectNameUnavailableSimilar - ) + with pytest.raises(ProjectNameUnavailableSimilar): + service.check_project_name("foo") def test_check_project_name_too_similar_multiple_existing(self, db_session): service = ProjectService(session=db_session) project1 = ProjectFactory.create(name="f00") project2 = ProjectFactory.create(name="f0o") - unavailable_error = service.check_project_name("foo") - assert isinstance(unavailable_error, ProjectNameUnavailableSimilar) + with pytest.raises(ProjectNameUnavailableSimilar) as exc: + service.check_project_name("foo") assert ( - unavailable_error.similar_project == project1 - or unavailable_error.similar_project == project2 + exc.value.similar_project_name == project1.name + or exc.value.similar_project_name == project2.name ) def test_check_project_name_ok(self, db_session): service = ProjectService(session=db_session) - assert service.check_project_name("foo") is None + # Should not raise any exception + service.check_project_name("foo") def test_project_service_factory(): diff --git a/warehouse/packaging/interfaces.py b/warehouse/packaging/interfaces.py index 209c1b1931cf..33510ccf3a2a 100644 --- a/warehouse/packaging/interfaces.py +++ b/warehouse/packaging/interfaces.py @@ -91,32 +91,39 @@ def create_project(name, creator, request, *, creator_is_owner=True): """ -class ProjectNameUnavailableInvalid: +class ProjectNameUnavailableError(Exception): + """Base exception for project name unavailability errors.""" + pass -class ProjectNameUnavailableStdlib: +class ProjectNameUnavailableInvalid(ProjectNameUnavailableError): + """Project name is invalid.""" + pass -class ProjectNameUnavailableExisting: +class ProjectNameUnavailableStdlib(ProjectNameUnavailableError): + """Project name conflicts with Python stdlib module.""" + + pass + + +class ProjectNameUnavailableExisting(ProjectNameUnavailableError): + """Project name conflicts with existing project.""" + def __init__(self, existing_project: Project): self.existing_project: Project = existing_project -class ProjectNameUnavailableProhibited: - pass +class ProjectNameUnavailableProhibited(ProjectNameUnavailableError): + """Project name is prohibited.""" + pass -class ProjectNameUnavailableSimilar: - def __init__(self, similar_project: Project): - self.similar_project: Project = similar_project +class ProjectNameUnavailableSimilar(ProjectNameUnavailableError): + """Project name is too similar to existing project.""" -ProjectNameUnavailableError = ( - ProjectNameUnavailableInvalid - | ProjectNameUnavailableStdlib - | ProjectNameUnavailableExisting - | ProjectNameUnavailableProhibited - | ProjectNameUnavailableSimilar -) + def __init__(self, similar_project_name: str): + self.similar_project_name: str = similar_project_name diff --git a/warehouse/packaging/services.py b/warehouse/packaging/services.py index e14446c3419b..c07ce156598d 100644 --- a/warehouse/packaging/services.py +++ b/warehouse/packaging/services.py @@ -30,7 +30,7 @@ from packaging.utils import canonicalize_name from pyramid.httpexceptions import HTTPBadRequest, HTTPConflict, HTTPForbidden -from sqlalchemy import exists, func +from sqlalchemy import exists, func, select from zope.interface import implementer from warehouse.admin.flags import AdminFlagValue @@ -447,36 +447,34 @@ def _hit_ratelimits(self, request, creator): self.ratelimiters["project.create.user"].hit(creator.id) self.ratelimiters["project.create.ip"].hit(request.remote_addr) - def check_project_name(self, name: str) -> ProjectNameUnavailableError | None: + def check_project_name(self, name: str) -> None: if not PROJECT_NAME_RE.match(name): - return ProjectNameUnavailableInvalid() + raise ProjectNameUnavailableInvalid() # Also check for collisions with Python Standard Library modules. if canonicalize_name(name) in STDLIB_PROHIBITED: - return ProjectNameUnavailableStdlib() + raise ProjectNameUnavailableStdlib() if ( existing_project := self.db.query(Project) .where(Project.normalized_name == func.normalize_pep426_name(name)) .first() ): - return ProjectNameUnavailableExisting(existing_project) + raise ProjectNameUnavailableExisting(existing_project) if self.db.query( exists().where( ProhibitedProjectName.name == func.normalize_pep426_name(name) ) ).scalar(): - return ProjectNameUnavailableProhibited() + raise ProjectNameUnavailableProhibited() - if ( - similar_project := self.db.query(Project) - .where( + if similar_project_name := self.db.scalars( + select(Project.name).where( func.ultranormalize_name(Project.name) == func.ultranormalize_name(name) ) - .first() - ): - return ProjectNameUnavailableSimilar(similar_project) + ).first(): + raise ProjectNameUnavailableSimilar(similar_project_name) return None @@ -498,51 +496,52 @@ def create_project( ) from None # Verify that the project name is both valid and currently available. - match self.check_project_name(name): - case ProjectNameUnavailableInvalid(): - raise HTTPBadRequest(f"The name {name!r} is invalid.") - case ProjectNameUnavailableExisting(): - # Found existing project with conflicting name. - raise HTTPConflict( - ( - "The name {name!r} conflicts with an existing project. " - "See {projecthelp} for more information." - ).format( - name=name, - projecthelp=request.help_url(_anchor="project-name"), - ), - ) from None - case ProjectNameUnavailableProhibited(): - raise HTTPBadRequest( - ( - "The name {name!r} isn't allowed. " - "See {projecthelp} for more information." - ).format( - name=name, - projecthelp=request.help_url(_anchor="project-name"), - ), - ) from None - case ProjectNameUnavailableSimilar(): - raise HTTPBadRequest( - ( - "The name {name!r} is too similar to an existing project. " - "See {projecthelp} for more information." - ).format( - name=name, - projecthelp=request.help_url(_anchor="project-name"), - ), - ) from None - case ProjectNameUnavailableStdlib(): - raise HTTPBadRequest( - ( - "The name {name!r} isn't allowed (conflict with Python " - "Standard Library module name). See " - "{projecthelp} for more information." - ).format( - name=name, - projecthelp=request.help_url(_anchor="project-name"), - ), - ) from None + try: + self.check_project_name(name) + except ProjectNameUnavailableInvalid: + raise HTTPBadRequest(f"The name {name!r} is invalid.") + except ProjectNameUnavailableExisting: + # Found existing project with conflicting name. + raise HTTPConflict( + ( + "The name {name!r} conflicts with an existing project. " + "See {projecthelp} for more information." + ).format( + name=name, + projecthelp=request.help_url(_anchor="project-name"), + ), + ) from None + except ProjectNameUnavailableProhibited: + raise HTTPBadRequest( + ( + "The name {name!r} isn't allowed. " + "See {projecthelp} for more information." + ).format( + name=name, + projecthelp=request.help_url(_anchor="project-name"), + ), + ) from None + except ProjectNameUnavailableSimilar: + raise HTTPBadRequest( + ( + "The name {name!r} is too similar to an existing project. " + "See {projecthelp} for more information." + ).format( + name=name, + projecthelp=request.help_url(_anchor="project-name"), + ), + ) from None + except ProjectNameUnavailableStdlib: + raise HTTPBadRequest( + ( + "The name {name!r} isn't allowed (conflict with Python " + "Standard Library module name). See " + "{projecthelp} for more information." + ).format( + name=name, + projecthelp=request.help_url(_anchor="project-name"), + ), + ) from None # The project name is valid: create it and add it project = Project(name=name) From 88ffa210e4d5dc87edbda0785ae97936777c312a Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Thu, 6 Feb 2025 14:28:05 +0100 Subject: [PATCH 8/8] Address review comments Signed-off-by: Facundo Tuesca --- tests/unit/oidc/forms/test_activestate.py | 6 ++-- tests/unit/oidc/forms/test_github.py | 24 +++++++------- tests/unit/oidc/forms/test_gitlab.py | 6 ++-- tests/unit/oidc/forms/test_google.py | 6 ++-- tests/unit/packaging/test_services.py | 26 +++++++-------- warehouse/oidc/forms/_core.py | 20 ++++++------ warehouse/packaging/interfaces.py | 17 ++++++---- warehouse/packaging/services.py | 40 +++++++++++------------ 8 files changed, 74 insertions(+), 71 deletions(-) diff --git a/tests/unit/oidc/forms/test_activestate.py b/tests/unit/oidc/forms/test_activestate.py index 6002654ae6c4..f9b0d8574d2c 100644 --- a/tests/unit/oidc/forms/test_activestate.py +++ b/tests/unit/oidc/forms/test_activestate.py @@ -19,7 +19,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import activestate -from warehouse.packaging.interfaces import ProjectNameUnavailableExisting +from warehouse.packaging.interfaces import ProjectNameUnavailableExistingError fake_username = "some-username" fake_org_name = "some-org" @@ -68,7 +68,7 @@ def test_validate_project_name_already_in_use_owner(self, pyramid_config): owners = [user] def check_project_name(name): - return ProjectNameUnavailableExisting( + return ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ) @@ -98,7 +98,7 @@ def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): owners = [] def check_project_name(name): - return ProjectNameUnavailableExisting( + return ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ) diff --git a/tests/unit/oidc/forms/test_github.py b/tests/unit/oidc/forms/test_github.py index e0d55fbb2ecc..f7930528b338 100644 --- a/tests/unit/oidc/forms/test_github.py +++ b/tests/unit/oidc/forms/test_github.py @@ -19,11 +19,11 @@ from warehouse.oidc.forms import github from warehouse.packaging.interfaces import ( - ProjectNameUnavailableExisting, - ProjectNameUnavailableInvalid, - ProjectNameUnavailableProhibited, - ProjectNameUnavailableSimilar, - ProjectNameUnavailableStdlib, + ProjectNameUnavailableExistingError, + ProjectNameUnavailableInvalidError, + ProjectNameUnavailableProhibitedError, + ProjectNameUnavailableSimilarError, + ProjectNameUnavailableStdlibError, ) @@ -68,7 +68,7 @@ def test_validate_project_name_already_in_use_owner(self, pyramid_config): form = github.PendingGitHubPublisherForm( api_token="fake-token", route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableExisting( + check_project_name=lambda name: ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ), user=user, @@ -96,7 +96,7 @@ def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): form = github.PendingGitHubPublisherForm( api_token="fake-token", route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableExisting( + check_project_name=lambda name: ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ), user=user, @@ -111,11 +111,11 @@ def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): @pytest.mark.parametrize( "reason", [ - ProjectNameUnavailableExisting(pretend.stub(owners=[pretend.stub()])), - ProjectNameUnavailableInvalid(), - ProjectNameUnavailableStdlib(), - ProjectNameUnavailableProhibited(), - ProjectNameUnavailableSimilar(similar_project_name="pkg_name"), + ProjectNameUnavailableExistingError(pretend.stub(owners=[pretend.stub()])), + ProjectNameUnavailableInvalidError(), + ProjectNameUnavailableStdlibError(), + ProjectNameUnavailableProhibitedError(), + ProjectNameUnavailableSimilarError(similar_project_name="pkg_name"), ], ) def test_validate_project_name_unavailable(self, reason, pyramid_config): diff --git a/tests/unit/oidc/forms/test_gitlab.py b/tests/unit/oidc/forms/test_gitlab.py index b0ec9a08f2ec..cd50060e81f7 100644 --- a/tests/unit/oidc/forms/test_gitlab.py +++ b/tests/unit/oidc/forms/test_gitlab.py @@ -17,7 +17,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import gitlab -from warehouse.packaging.interfaces import ProjectNameUnavailableExisting +from warehouse.packaging.interfaces import ProjectNameUnavailableExistingError class TestPendingGitLabPublisherForm: @@ -56,7 +56,7 @@ def test_validate_project_name_already_in_use_owner(self, pyramid_config): form = gitlab.PendingGitLabPublisherForm( route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableExisting( + check_project_name=lambda name: ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ), user=user, @@ -83,7 +83,7 @@ def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): form = gitlab.PendingGitLabPublisherForm( route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableExisting( + check_project_name=lambda name: ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ), user=user, diff --git a/tests/unit/oidc/forms/test_google.py b/tests/unit/oidc/forms/test_google.py index 6de8d2a9b9d0..9cb014a24801 100644 --- a/tests/unit/oidc/forms/test_google.py +++ b/tests/unit/oidc/forms/test_google.py @@ -17,7 +17,7 @@ from webob.multidict import MultiDict from warehouse.oidc.forms import google -from warehouse.packaging.interfaces import ProjectNameUnavailableExisting +from warehouse.packaging.interfaces import ProjectNameUnavailableExistingError class TestPendingGooglePublisherForm: @@ -53,7 +53,7 @@ def test_validate_project_name_already_in_use_owner(self, pyramid_config): route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") form = google.PendingGooglePublisherForm( route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableExisting( + check_project_name=lambda name: ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ), user=user, @@ -79,7 +79,7 @@ def test_validate_project_name_already_in_use_not_owner(self, pyramid_config): route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url") form = google.PendingGooglePublisherForm( route_url=route_url, - check_project_name=lambda name: ProjectNameUnavailableExisting( + check_project_name=lambda name: ProjectNameUnavailableExistingError( existing_project=pretend.stub(owners=owners) ), user=user, diff --git a/tests/unit/packaging/test_services.py b/tests/unit/packaging/test_services.py index b39c3db59cb7..83dbbb2e60b4 100644 --- a/tests/unit/packaging/test_services.py +++ b/tests/unit/packaging/test_services.py @@ -29,11 +29,11 @@ IFileStorage, IProjectService, ISimpleStorage, - ProjectNameUnavailableExisting, - ProjectNameUnavailableInvalid, - ProjectNameUnavailableProhibited, - ProjectNameUnavailableSimilar, - ProjectNameUnavailableStdlib, + ProjectNameUnavailableExistingError, + ProjectNameUnavailableInvalidError, + ProjectNameUnavailableProhibitedError, + ProjectNameUnavailableSimilarError, + ProjectNameUnavailableStdlibError, ) from warehouse.packaging.services import ( B2FileStorage, @@ -1000,42 +1000,42 @@ def test_verify_service(self): def test_check_project_name_invalid(self, name): service = ProjectService(session=pretend.stub()) - with pytest.raises(ProjectNameUnavailableInvalid): + with pytest.raises(ProjectNameUnavailableInvalidError): service.check_project_name(name) @pytest.mark.parametrize("name", ["uu", "cgi", "nis", "mailcap"]) def test_check_project_name_stdlib(self, name): service = ProjectService(session=pretend.stub()) - with pytest.raises(ProjectNameUnavailableStdlib): + with pytest.raises(ProjectNameUnavailableStdlibError): service.check_project_name(name) def test_check_project_name_already_exists(self, db_session): service = ProjectService(session=db_session) project = ProjectFactory.create(name="foo") - with pytest.raises(ProjectNameUnavailableExisting) as exc: + with pytest.raises(ProjectNameUnavailableExistingError) as exc: service.check_project_name("foo") assert exc.value.existing_project == project - with pytest.raises(ProjectNameUnavailableExisting): + with pytest.raises(ProjectNameUnavailableExistingError): service.check_project_name("Foo") def test_check_project_name_prohibited(self, db_session): service = ProjectService(session=db_session) ProhibitedProjectFactory.create(name="foo") - with pytest.raises(ProjectNameUnavailableProhibited): + with pytest.raises(ProjectNameUnavailableProhibitedError): service.check_project_name("foo") - with pytest.raises(ProjectNameUnavailableProhibited): + with pytest.raises(ProjectNameUnavailableProhibitedError): service.check_project_name("Foo") def test_check_project_name_too_similar(self, db_session): service = ProjectService(session=db_session) ProjectFactory.create(name="f00") - with pytest.raises(ProjectNameUnavailableSimilar): + with pytest.raises(ProjectNameUnavailableSimilarError): service.check_project_name("foo") def test_check_project_name_too_similar_multiple_existing(self, db_session): @@ -1043,7 +1043,7 @@ def test_check_project_name_too_similar_multiple_existing(self, db_session): project1 = ProjectFactory.create(name="f00") project2 = ProjectFactory.create(name="f0o") - with pytest.raises(ProjectNameUnavailableSimilar) as exc: + with pytest.raises(ProjectNameUnavailableSimilarError) as exc: service.check_project_name("foo") assert ( exc.value.similar_project_name == project1.name diff --git a/warehouse/oidc/forms/_core.py b/warehouse/oidc/forms/_core.py index d973a1b95c10..75765a272a79 100644 --- a/warehouse/oidc/forms/_core.py +++ b/warehouse/oidc/forms/_core.py @@ -14,11 +14,11 @@ from warehouse.i18n import localize as _ from warehouse.packaging.interfaces import ( - ProjectNameUnavailableExisting, - ProjectNameUnavailableInvalid, - ProjectNameUnavailableProhibited, - ProjectNameUnavailableSimilar, - ProjectNameUnavailableStdlib, + ProjectNameUnavailableExistingError, + ProjectNameUnavailableInvalidError, + ProjectNameUnavailableProhibitedError, + ProjectNameUnavailableSimilarError, + ProjectNameUnavailableStdlibError, ) from warehouse.utils.project import PROJECT_NAME_RE @@ -37,9 +37,9 @@ def validate_project_name(self, field): project_name = field.data match self._check_project_name(project_name): - case ProjectNameUnavailableInvalid(): + case ProjectNameUnavailableInvalidError(): raise wtforms.validators.ValidationError(_("Invalid project name")) - case ProjectNameUnavailableExisting(existing_project=existing_project): + case ProjectNameUnavailableExistingError(existing_project=existing_project): # If the user owns the existing project, the error message includes a # link to the project settings that the user can modify. if self._user in existing_project.owners: @@ -70,15 +70,15 @@ def validate_project_name(self, field): _("This project already exists.") ) - case ProjectNameUnavailableProhibited(): + case ProjectNameUnavailableProhibitedError(): raise wtforms.validators.ValidationError( _("This project name isn't allowed") ) - case ProjectNameUnavailableSimilar(): + case ProjectNameUnavailableSimilarError(): raise wtforms.validators.ValidationError( _("This project name is too similar to an existing project") ) - case ProjectNameUnavailableStdlib(): + case ProjectNameUnavailableStdlibError(): raise wtforms.validators.ValidationError( _( "This project name isn't allowed (conflict with the Python" diff --git a/warehouse/packaging/interfaces.py b/warehouse/packaging/interfaces.py index 33510ccf3a2a..c9eb3150336e 100644 --- a/warehouse/packaging/interfaces.py +++ b/warehouse/packaging/interfaces.py @@ -9,14 +9,17 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from __future__ import annotations -import enum +import typing from zope.interface import Interface -from warehouse.packaging.models import Project from warehouse.rate_limiting.interfaces import RateLimiterException +if typing.TYPE_CHECKING: + from warehouse.packaging.models import Project + class TooManyProjectsCreated(RateLimiterException): pass @@ -97,32 +100,32 @@ class ProjectNameUnavailableError(Exception): pass -class ProjectNameUnavailableInvalid(ProjectNameUnavailableError): +class ProjectNameUnavailableInvalidError(ProjectNameUnavailableError): """Project name is invalid.""" pass -class ProjectNameUnavailableStdlib(ProjectNameUnavailableError): +class ProjectNameUnavailableStdlibError(ProjectNameUnavailableError): """Project name conflicts with Python stdlib module.""" pass -class ProjectNameUnavailableExisting(ProjectNameUnavailableError): +class ProjectNameUnavailableExistingError(ProjectNameUnavailableError): """Project name conflicts with existing project.""" def __init__(self, existing_project: Project): self.existing_project: Project = existing_project -class ProjectNameUnavailableProhibited(ProjectNameUnavailableError): +class ProjectNameUnavailableProhibitedError(ProjectNameUnavailableError): """Project name is prohibited.""" pass -class ProjectNameUnavailableSimilar(ProjectNameUnavailableError): +class ProjectNameUnavailableSimilarError(ProjectNameUnavailableError): """Project name is too similar to existing project.""" def __init__(self, similar_project_name: str): diff --git a/warehouse/packaging/services.py b/warehouse/packaging/services.py index c07ce156598d..a7713ae93b0a 100644 --- a/warehouse/packaging/services.py +++ b/warehouse/packaging/services.py @@ -43,11 +43,11 @@ IFileStorage, IProjectService, ISimpleStorage, - ProjectNameUnavailableExisting, - ProjectNameUnavailableInvalid, - ProjectNameUnavailableProhibited, - ProjectNameUnavailableSimilar, - ProjectNameUnavailableStdlib, + ProjectNameUnavailableExistingError, + ProjectNameUnavailableInvalidError, + ProjectNameUnavailableProhibitedError, + ProjectNameUnavailableSimilarError, + ProjectNameUnavailableStdlibError, TooManyProjectsCreated, ) from warehouse.packaging.models import ( @@ -449,32 +449,32 @@ def _hit_ratelimits(self, request, creator): def check_project_name(self, name: str) -> None: if not PROJECT_NAME_RE.match(name): - raise ProjectNameUnavailableInvalid() + raise ProjectNameUnavailableInvalidError() # Also check for collisions with Python Standard Library modules. if canonicalize_name(name) in STDLIB_PROHIBITED: - raise ProjectNameUnavailableStdlib() + raise ProjectNameUnavailableStdlibError() - if ( - existing_project := self.db.query(Project) - .where(Project.normalized_name == func.normalize_pep426_name(name)) - .first() - ): - raise ProjectNameUnavailableExisting(existing_project) + if existing_project := self.db.scalars( + select(Project).where( + Project.normalized_name == func.normalize_pep426_name(name) + ) + ).first(): + raise ProjectNameUnavailableExistingError(existing_project) if self.db.query( exists().where( ProhibitedProjectName.name == func.normalize_pep426_name(name) ) ).scalar(): - raise ProjectNameUnavailableProhibited() + raise ProjectNameUnavailableProhibitedError() if similar_project_name := self.db.scalars( select(Project.name).where( func.ultranormalize_name(Project.name) == func.ultranormalize_name(name) ) ).first(): - raise ProjectNameUnavailableSimilar(similar_project_name) + raise ProjectNameUnavailableSimilarError(similar_project_name) return None @@ -498,9 +498,9 @@ def create_project( # Verify that the project name is both valid and currently available. try: self.check_project_name(name) - except ProjectNameUnavailableInvalid: + except ProjectNameUnavailableInvalidError: raise HTTPBadRequest(f"The name {name!r} is invalid.") - except ProjectNameUnavailableExisting: + except ProjectNameUnavailableExistingError: # Found existing project with conflicting name. raise HTTPConflict( ( @@ -511,7 +511,7 @@ def create_project( projecthelp=request.help_url(_anchor="project-name"), ), ) from None - except ProjectNameUnavailableProhibited: + except ProjectNameUnavailableProhibitedError: raise HTTPBadRequest( ( "The name {name!r} isn't allowed. " @@ -521,7 +521,7 @@ def create_project( projecthelp=request.help_url(_anchor="project-name"), ), ) from None - except ProjectNameUnavailableSimilar: + except ProjectNameUnavailableSimilarError: raise HTTPBadRequest( ( "The name {name!r} is too similar to an existing project. " @@ -531,7 +531,7 @@ def create_project( projecthelp=request.help_url(_anchor="project-name"), ), ) from None - except ProjectNameUnavailableStdlib: + except ProjectNameUnavailableStdlibError: raise HTTPBadRequest( ( "The name {name!r} isn't allowed (conflict with Python "