From 694043dbc2b562de91d933ed6651f58eca827b29 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Tue, 29 Oct 2024 17:04:08 -0700 Subject: [PATCH 1/6] Provide a project's users via the JSON API Closes #16999 --- tests/unit/legacy/api/test_json.py | 24 +++++++++++++++++++++--- warehouse/legacy/api/json.py | 16 ++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/tests/unit/legacy/api/test_json.py b/tests/unit/legacy/api/test_json.py index 3cbad70d2795..5de49d6887cc 100644 --- a/tests/unit/legacy/api/test_json.py +++ b/tests/unit/legacy/api/test_json.py @@ -26,6 +26,7 @@ JournalEntryFactory, ProjectFactory, ReleaseFactory, + RoleFactory, ) @@ -210,7 +211,9 @@ def test_renders(self, pyramid_config, db_request, db_session): ) for r in releases[1:] ] - user = UserFactory.create() + user = UserFactory.create(username="guido") + RoleFactory.create(user=user, project=project) + JournalEntryFactory.reset_sequence() je = JournalEntryFactory.create(name=project.name, submitted_by=user) @@ -260,6 +263,7 @@ def test_renders(self, pyramid_config, db_request, db_session): "requires_dist": None, "requires_python": None, "summary": None, + "users": ["guido"], "yanked": False, "yanked_reason": None, "version": "3.0", @@ -533,7 +537,14 @@ def test_detail_renders(self, pyramid_config, db_request, db_session): ) for r in releases[1:] ] - user = UserFactory.create() + + user = UserFactory.create(username="guido") + RoleFactory.create(user=user, project=project) + RoleFactory.create(user=UserFactory.create(username="dstufft"), project=project) + RoleFactory.create( + user=UserFactory.create(username="ewdurbin"), project=project + ) + JournalEntryFactory.reset_sequence() je = JournalEntryFactory.create(name=project.name, submitted_by=user) @@ -584,6 +595,7 @@ def test_detail_renders(self, pyramid_config, db_request, db_session): "requires_dist": None, "requires_python": None, "summary": None, + "users": ["dstufft", "ewdurbin", "guido"], "yanked": False, "yanked_reason": None, "version": "3.0", @@ -625,7 +637,12 @@ def test_minimal_renders(self, pyramid_config, db_request): size=200, ) - user = UserFactory.create() + user = UserFactory.create(username="dstufft") + RoleFactory.create(user=user, project=project) + RoleFactory.create( + user=UserFactory.create(username="ewdurbin"), project=project + ) + JournalEntryFactory.reset_sequence() je = JournalEntryFactory.create(name=project.name, submitted_by=user) @@ -676,6 +693,7 @@ def test_minimal_renders(self, pyramid_config, db_request): "requires_dist": None, "requires_python": None, "summary": None, + "users": ["dstufft", "ewdurbin"], "yanked": False, "yanked_reason": None, "version": "0.1", diff --git a/warehouse/legacy/api/json.py b/warehouse/legacy/api/json.py index 3fabc633bb95..b698818ec057 100644 --- a/warehouse/legacy/api/json.py +++ b/warehouse/legacy/api/json.py @@ -25,6 +25,8 @@ Project, Release, ReleaseURL, + Role, + User, ) from warehouse.utils.cors import _CORS_HEADERS @@ -65,6 +67,19 @@ def _json_data(request, project, release, *, all_releases): .filter(Release.project == project) ) + # Get all of the maintainers for this project. + maintainers = [ + r.user + for r in ( + request.db.query(Role) + .join(User) + .filter(Role.project == project) + .distinct(User.username) + .order_by(User.username) + .all() + ) + ] + # If we're not looking for all_releases, then we'll filter this further # to just this release. if not all_releases: @@ -158,6 +173,7 @@ def _json_data(request, project, release, *, all_releases): "author_email": release.author_email, "maintainer": release.maintainer, "maintainer_email": release.maintainer_email, + "users": sorted(user.username for user in maintainers), "requires_python": release.requires_python, "platform": release.platform, "downloads": {"last_day": -1, "last_week": -1, "last_month": -1}, From d1ec70d9c7b20f44e3b5a9be188c232f4d141da8 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Wed, 30 Oct 2024 16:32:43 -0700 Subject: [PATCH 2/6] Responding to review suggestions Add a "roles" key instead of a "users" key. --- tests/unit/legacy/api/test_json.py | 17 ++++++++--- warehouse/legacy/api/json.py | 47 +++++++++++++++++++++--------- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/tests/unit/legacy/api/test_json.py b/tests/unit/legacy/api/test_json.py index 5de49d6887cc..5c164d48204b 100644 --- a/tests/unit/legacy/api/test_json.py +++ b/tests/unit/legacy/api/test_json.py @@ -262,8 +262,10 @@ def test_renders(self, pyramid_config, db_request, db_session): "release_url": "/the/fake/url/", "requires_dist": None, "requires_python": None, + "roles": { + "owner": ["guido"], + }, "summary": None, - "users": ["guido"], "yanked": False, "yanked_reason": None, "version": "3.0", @@ -542,7 +544,9 @@ def test_detail_renders(self, pyramid_config, db_request, db_session): RoleFactory.create(user=user, project=project) RoleFactory.create(user=UserFactory.create(username="dstufft"), project=project) RoleFactory.create( - user=UserFactory.create(username="ewdurbin"), project=project + user=UserFactory.create(username="ewdurbin"), + project=project, + role_name="Maintainer", ) JournalEntryFactory.reset_sequence() @@ -594,8 +598,11 @@ def test_detail_renders(self, pyramid_config, db_request, db_session): "release_url": "/the/fake/url/", "requires_dist": None, "requires_python": None, + "roles": { + "owner": ["dstufft", "guido"], + "maintainer": ["ewdurbin"], + }, "summary": None, - "users": ["dstufft", "ewdurbin", "guido"], "yanked": False, "yanked_reason": None, "version": "3.0", @@ -692,8 +699,10 @@ def test_minimal_renders(self, pyramid_config, db_request): "release_url": "/the/fake/url/", "requires_dist": None, "requires_python": None, + "roles": { + "owner": ["dstufft", "ewdurbin"], + }, "summary": None, - "users": ["dstufft", "ewdurbin"], "yanked": False, "yanked_reason": None, "version": "0.1", diff --git a/warehouse/legacy/api/json.py b/warehouse/legacy/api/json.py index b698818ec057..7c7e2d907d20 100644 --- a/warehouse/legacy/api/json.py +++ b/warehouse/legacy/api/json.py @@ -10,6 +10,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +from collections import defaultdict + from packaging.utils import canonicalize_name, canonicalize_version from pyramid.httpexceptions import HTTPMovedPermanently, HTTPNotFound from pyramid.view import view_config @@ -67,19 +69,6 @@ def _json_data(request, project, release, *, all_releases): .filter(Release.project == project) ) - # Get all of the maintainers for this project. - maintainers = [ - r.user - for r in ( - request.db.query(Role) - .join(User) - .filter(Role.project == project) - .distinct(User.username) - .order_by(User.username) - .all() - ) - ] - # If we're not looking for all_releases, then we'll filter this further # to just this release. if not all_releases: @@ -159,6 +148,36 @@ def _json_data(request, project, release, *, all_releases): for vulnerability_record in release.vulnerabilities ] + # All roles and their users associated with this project. Note that + # unlike everything other than `project.name` (which is immutable), this + # data comes from the project itself, not the latest release. + # + # 2024-10-30(warsaw): It's convenient to use a defaultdict here. We could + # have alternatively used a base dict and the .setdefault() method, but it + # doesn't matter too much because we still have to post-process the data + # structure before we can pass it to the "info" key. The reason is that + # the JSON serializer doesn't know how to serialize defaultdicts, but it + # also doesn't know how to serialize sets, and we want to use a set just + # for uniqueness. Thus below we turn the defaultdict mapping role names to + # sets, into a base dict mapping role names to lists. I think it's + # moderately more efficient because the defaultdict only instantiates sets + # when the key is missing. + roles = defaultdict(set) + # Get all of the maintainers for this project. + for role in ( + request.db.query(Role) + .join(User) + .filter(Role.project == project) + .distinct(User.username) + .order_by(User.username) + .all() + ): + # 2024-10-30(warsaw): Normalizing the role name to lower case, but only + # because I think that looks better. This could introduce some + # friction in the future if we ever have a writable API that lets us + # add and modify roles. + roles[role.role_name.lower()].add(role.user.username) + data = { "info": { "name": project.name, @@ -173,8 +192,8 @@ def _json_data(request, project, release, *, all_releases): "author_email": release.author_email, "maintainer": release.maintainer, "maintainer_email": release.maintainer_email, - "users": sorted(user.username for user in maintainers), "requires_python": release.requires_python, + "roles": {key: sorted(value) for key, value in roles.items()}, "platform": release.platform, "downloads": {"last_day": -1, "last_week": -1, "last_month": -1}, "package_url": request.route_url("packaging.project", name=project.name), From db9e7a1d07296deabfe1cbcee4fa38c4e6c82a06 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 31 Oct 2024 12:59:25 -0700 Subject: [PATCH 3/6] Add documentation --- docs/dev/api-reference/json.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/dev/api-reference/json.rst b/docs/dev/api-reference/json.rst index 261e14d470de..011b0ce49dcc 100644 --- a/docs/dev/api-reference/json.rst +++ b/docs/dev/api-reference/json.rst @@ -117,6 +117,10 @@ Project "coverage ; extra == 'test'" ], "requires_python": ">=3.7", + "roles": { + "owner": ["alice", "bob"], + "maintainer: ["carol", "dave"], + }, "summary": "A sample Python project", "version": "3.0.0", "yanked": false, From 7a57c479b442d0ff97b1b3ed5248aeaba9a5cf48 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 31 Oct 2024 13:37:45 -0700 Subject: [PATCH 4/6] Fix doc typo --- docs/dev/api-reference/json.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev/api-reference/json.rst b/docs/dev/api-reference/json.rst index 011b0ce49dcc..21c47582ea61 100644 --- a/docs/dev/api-reference/json.rst +++ b/docs/dev/api-reference/json.rst @@ -119,7 +119,7 @@ Project "requires_python": ">=3.7", "roles": { "owner": ["alice", "bob"], - "maintainer: ["carol", "dave"], + "maintainer": ["carol", "dave"], }, "summary": "A sample Python project", "version": "3.0.0", From 2b5c230bc0c5d2b3c138ff6a46d908ef6971a4b0 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Fri, 1 Nov 2024 15:48:15 -0700 Subject: [PATCH 5/6] Only include ["info"]["roles"] on the project view, not the release view --- tests/unit/legacy/api/test_json.py | 234 ++++++++++++++++++++++++++++- warehouse/legacy/api/json.py | 67 +++++---- 2 files changed, 263 insertions(+), 38 deletions(-) diff --git a/tests/unit/legacy/api/test_json.py b/tests/unit/legacy/api/test_json.py index 5c164d48204b..f94b9ca044d8 100644 --- a/tests/unit/legacy/api/test_json.py +++ b/tests/unit/legacy/api/test_json.py @@ -375,6 +375,233 @@ def test_renders(self, pyramid_config, db_request, db_session): "vulnerabilities": [], } + def test_project_with_multiple_roles(self, pyramid_config, db_request, db_session): + project = ProjectFactory.create(has_docs=True) + description_content_type = "text/x-rst" + url = "/the/fake/url/" + project_urls = [ + "url," + url, + "Homepage,https://example.com/home2/", + "Source Code,https://example.com/source-code/", + "uri,http://john.doe@www.example.com:123/forum/questions/?tag=networking&order=newest#top", # noqa: E501 + "ldap,ldap://[2001:db8::7]/c=GB?objectClass?one", + "tel,tel:+1-816-555-1212", + "telnet,telnet://192.0.2.16:80/", + "urn,urn:oasis:names:specification:docbook:dtd:xml:4.1.2", + "reservedchars,http://example.com?&$+/:;=@#", # Commas don't work! + r"unsafechars,http://example.com <>[]{}|\^%", + ] + expected_urls = [] + for project_url in sorted( + project_urls, key=lambda u: u.split(",", 1)[0].strip().lower() + ): + expected_urls.append(tuple(project_url.split(",", 1))) + expected_urls = dict(tuple(expected_urls)) + + releases = [ + ReleaseFactory.create(project=project, version=v) + for v in ["0.1", "1.0", "2.0"] + ] + releases += [ + ReleaseFactory.create( + project=project, + version="3.0", + description=DescriptionFactory.create( + content_type=description_content_type + ), + ) + ] + + for urlspec in project_urls: + label, _, purl = urlspec.partition(",") + db_session.add( + ReleaseURL( + release=releases[3], + name=label.strip(), + url=purl.strip(), + ) + ) + + files = [ + FileFactory.create( + release=r, + filename=f"{project.name}-{r.version}.tar.gz", + python_version="source", + size=200, + ) + for r in releases[1:] + ] + user = UserFactory.create(username="guido") + RoleFactory.create(user=user, project=project) + RoleFactory.create(user=UserFactory.create(username="dstufft"), project=project) + RoleFactory.create( + user=UserFactory.create(username="ewdurbin"), + project=project, + role_name="Maintainer", + ) + + JournalEntryFactory.reset_sequence() + je = JournalEntryFactory.create(name=project.name, submitted_by=user) + + db_request.route_url = pretend.call_recorder(lambda *args, **kw: url) + db_request.matchdict = {"name": project.normalized_name} + + result = json.json_project(releases[-1], db_request) + + assert set(db_request.route_url.calls) == { + pretend.call("packaging.file", path=files[0].path), + pretend.call("packaging.file", path=files[1].path), + pretend.call("packaging.file", path=files[2].path), + pretend.call("packaging.project", name=project.name), + pretend.call( + "packaging.release", name=project.name, version=releases[3].version + ), + pretend.call("legacy.docs", project=project.name), + } + + _assert_has_cors_headers(db_request.response.headers) + assert db_request.response.headers["X-PyPI-Last-Serial"] == str(je.id) + + assert result == { + "info": { + "author": None, + "author_email": None, + "bugtrack_url": None, + "classifiers": [], + "description_content_type": description_content_type, + "description": releases[-1].description.raw, + "docs_url": "/the/fake/url/", + "download_url": None, + "downloads": {"last_day": -1, "last_week": -1, "last_month": -1}, + "dynamic": None, + "home_page": None, + "keywords": None, + "license": None, + "maintainer": None, + "maintainer_email": None, + "name": project.name, + "package_url": "/the/fake/url/", + "platform": None, + "project_url": "/the/fake/url/", + "project_urls": expected_urls, + "provides_extra": None, + "release_url": "/the/fake/url/", + "requires_dist": None, + "requires_python": None, + "roles": { + "owner": ["dstufft", "guido"], + "maintainer": ["ewdurbin"], + }, + "summary": None, + "yanked": False, + "yanked_reason": None, + "version": "3.0", + }, + "releases": { + "0.1": [], + "1.0": [ + { + "comment_text": None, + "downloads": -1, + "filename": files[0].filename, + "has_sig": False, + "md5_digest": files[0].md5_digest, + "digests": { + "md5": files[0].md5_digest, + "sha256": files[0].sha256_digest, + "blake2b_256": files[0].blake2_256_digest, + }, + "packagetype": files[0].packagetype, + "python_version": "source", + "size": 200, + "upload_time": files[0].upload_time.strftime( + "%Y-%m-%dT%H:%M:%S" + ), + "upload_time_iso_8601": files[0].upload_time.isoformat() + "Z", + "url": "/the/fake/url/", + "requires_python": None, + "yanked": False, + "yanked_reason": None, + } + ], + "2.0": [ + { + "comment_text": None, + "downloads": -1, + "filename": files[1].filename, + "has_sig": False, + "md5_digest": files[1].md5_digest, + "digests": { + "md5": files[1].md5_digest, + "sha256": files[1].sha256_digest, + "blake2b_256": files[1].blake2_256_digest, + }, + "packagetype": files[1].packagetype, + "python_version": "source", + "size": 200, + "upload_time": files[1].upload_time.strftime( + "%Y-%m-%dT%H:%M:%S" + ), + "upload_time_iso_8601": files[1].upload_time.isoformat() + "Z", + "url": "/the/fake/url/", + "requires_python": None, + "yanked": False, + "yanked_reason": None, + } + ], + "3.0": [ + { + "comment_text": None, + "downloads": -1, + "filename": files[2].filename, + "has_sig": False, + "md5_digest": files[2].md5_digest, + "digests": { + "blake2b_256": files[2].blake2_256_digest, + "md5": files[2].md5_digest, + "sha256": files[2].sha256_digest, + }, + "packagetype": files[2].packagetype, + "python_version": "source", + "size": 200, + "upload_time": files[2].upload_time.strftime( + "%Y-%m-%dT%H:%M:%S" + ), + "upload_time_iso_8601": files[2].upload_time.isoformat() + "Z", + "url": "/the/fake/url/", + "requires_python": None, + "yanked": False, + "yanked_reason": None, + } + ], + }, + "urls": [ + { + "comment_text": None, + "downloads": -1, + "filename": files[2].filename, + "has_sig": False, + "md5_digest": files[2].md5_digest, + "digests": { + "md5": files[2].md5_digest, + "sha256": files[2].sha256_digest, + "blake2b_256": files[2].blake2_256_digest, + }, + "packagetype": files[2].packagetype, + "python_version": "source", + "size": 200, + "upload_time": files[2].upload_time.strftime("%Y-%m-%dT%H:%M:%S"), + "upload_time_iso_8601": files[2].upload_time.isoformat() + "Z", + "url": "/the/fake/url/", + "requires_python": None, + "yanked": False, + "yanked_reason": None, + } + ], + "last_serial": je.id, + "vulnerabilities": [], + } + class TestJSONProjectSlash: def test_normalizing_redirects(self, db_request): @@ -598,10 +825,6 @@ def test_detail_renders(self, pyramid_config, db_request, db_session): "release_url": "/the/fake/url/", "requires_dist": None, "requires_python": None, - "roles": { - "owner": ["dstufft", "guido"], - "maintainer": ["ewdurbin"], - }, "summary": None, "yanked": False, "yanked_reason": None, @@ -699,9 +922,6 @@ def test_minimal_renders(self, pyramid_config, db_request): "release_url": "/the/fake/url/", "requires_dist": None, "requires_python": None, - "roles": { - "owner": ["dstufft", "ewdurbin"], - }, "summary": None, "yanked": False, "yanked_reason": None, diff --git a/warehouse/legacy/api/json.py b/warehouse/legacy/api/json.py index 7c7e2d907d20..11534b325ae0 100644 --- a/warehouse/legacy/api/json.py +++ b/warehouse/legacy/api/json.py @@ -148,36 +148,6 @@ def _json_data(request, project, release, *, all_releases): for vulnerability_record in release.vulnerabilities ] - # All roles and their users associated with this project. Note that - # unlike everything other than `project.name` (which is immutable), this - # data comes from the project itself, not the latest release. - # - # 2024-10-30(warsaw): It's convenient to use a defaultdict here. We could - # have alternatively used a base dict and the .setdefault() method, but it - # doesn't matter too much because we still have to post-process the data - # structure before we can pass it to the "info" key. The reason is that - # the JSON serializer doesn't know how to serialize defaultdicts, but it - # also doesn't know how to serialize sets, and we want to use a set just - # for uniqueness. Thus below we turn the defaultdict mapping role names to - # sets, into a base dict mapping role names to lists. I think it's - # moderately more efficient because the defaultdict only instantiates sets - # when the key is missing. - roles = defaultdict(set) - # Get all of the maintainers for this project. - for role in ( - request.db.query(Role) - .join(User) - .filter(Role.project == project) - .distinct(User.username) - .order_by(User.username) - .all() - ): - # 2024-10-30(warsaw): Normalizing the role name to lower case, but only - # because I think that looks better. This could introduce some - # friction in the future if we ever have a writable API that lets us - # add and modify roles. - roles[role.role_name.lower()].add(role.user.username) - data = { "info": { "name": project.name, @@ -193,7 +163,6 @@ def _json_data(request, project, release, *, all_releases): "maintainer": release.maintainer, "maintainer_email": release.maintainer_email, "requires_python": release.requires_python, - "roles": {key: sorted(value) for key, value in roles.items()}, "platform": release.platform, "downloads": {"last_day": -1, "last_week": -1, "last_month": -1}, "package_url": request.route_url("packaging.project", name=project.name), @@ -223,6 +192,42 @@ def _json_data(request, project, release, *, all_releases): if all_releases: data["releases"] = releases + # All roles and their users associated with this project. Note that + # unlike everything other than `project.name` (which is immutable), + # this data comes from the project itself, not the latest release. It + # should only be included in the response when `all_releases` is True, + # because that's a signal that we're asking for the project view, not a + # specific release view. + # + # 2024-10-30(warsaw): It's convenient to use a defaultdict here. We + # could have alternatively used a base dict and the .setdefault() + # method, but it doesn't matter too much because we still have to + # post-process the data structure before we can pass it to the "info" + # key. The reason is that the JSON serializer doesn't know how to + # serialize defaultdicts, but it also doesn't know how to serialize + # sets, and we want to use a set just for uniqueness. Thus below we + # turn the defaultdict mapping role names to sets, into a base dict + # mapping role names to lists. I think it's moderately more efficient + # because the defaultdict only instantiates sets when the key is + # missing. + roles = defaultdict(set) + # Get all of the maintainers for this project. + for role in ( + request.db.query(Role) + .join(User) + .filter(Role.project == project) + .distinct(User.username) + .order_by(User.username) + .all() + ): + # 2024-10-30(warsaw): Normalizing the role name to lower case, but only + # because I think that looks better. This could introduce some + # friction in the future if we ever have a writable API that lets us + # add and modify roles. + roles[role.role_name.lower()].add(role.user.username) + + data["info"]["roles"] = {key: sorted(value) for key, value in roles.items()} + return data From 43f062a4b3cb4fedd937018263ecefc032126fca Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Fri, 1 Nov 2024 16:04:02 -0700 Subject: [PATCH 6/6] make reformat --- warehouse/legacy/api/json.py | 1 - 1 file changed, 1 deletion(-) diff --git a/warehouse/legacy/api/json.py b/warehouse/legacy/api/json.py index 11534b325ae0..6fda3e9c1b88 100644 --- a/warehouse/legacy/api/json.py +++ b/warehouse/legacy/api/json.py @@ -228,7 +228,6 @@ def _json_data(request, project, release, *, all_releases): data["info"]["roles"] = {key: sorted(value) for key, value in roles.items()} - return data