From 205342049bcc2d0ecc26b88f2e6862c10c9b85fd Mon Sep 17 00:00:00 2001 From: Matthew Planchard Date: Tue, 17 Sep 2019 21:30:30 -0500 Subject: [PATCH] [268] Ensure remove_pkg removes all pkg instances (#273) * [268] Ensure remove_pkg removes all pkg instances Reported by @esciara in #268. Previously, the `remove_pkg` command was only removing the first matching package that it found so if, for example, there were a .tar.gz file and a .whl file, it would only remove one of them. Of course, it could be run in succession to accomplish full removal, but the expected behavior is that removal will remove the package entirely. Here, I've grouped `remove_pkg` related tests into a test class, added some tests that verify the expected behavior, and updated the `remove_pkg` method to remove all matching packages. * CHANGES.rst --- CHANGES.rst | 5 ++ pypiserver/_app.py | 16 +++--- tests/test_app.py | 128 ++++++++++++++++++++++++++++++++------------- 3 files changed, 107 insertions(+), 42 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index acab209..54e5fed 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,11 @@ Changelog 1.3.2 (tbd) ----------- +- FIX: The `remove_pkg` API action now removes any extant instances of a + package name-version combination, not just the first one found. This means + that now, for example, if a `.whl` and `.tar.gz` file exist for the + requested package name and version, both will be removed (thanks to + @esciara for reporting in #268) - DEV: switched to gitlab for CI 1.3.1 (2019-09-10) diff --git a/pypiserver/_app.py b/pypiserver/_app.py index 653a86c..9e80b18 100644 --- a/pypiserver/_app.py +++ b/pypiserver/_app.py @@ -147,14 +147,16 @@ def remove_pkg(): if not name or not version: msg = "Missing 'name'/'version' fields: name=%s, version=%s" raise HTTPError(400, msg % (name, version)) - found = None - for pkg in core.find_packages(packages()): - if pkg.pkgname == name and pkg.version == version: - found = pkg - break - if found is None: + pkgs = list( + filter( + lambda pkg: pkg.pkgname == name and pkg.version == version, + core.find_packages(packages()), + ) + ) + if len(pkgs) == 0: raise HTTPError(404, "%s (%s) not found" % (name, version)) - os.unlink(found.fn) + for pkg in pkgs: + os.unlink(pkg.fn) Upload = namedtuple("Upload", "pkg sig") diff --git a/tests/test_app.py b/tests/test_app.py index 93c5d11..eababa9 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -483,38 +483,6 @@ def test_upload_badFilename(package, root, testapp): assert "Bad filename: %s" % package in resp.text -@pytest.mark.parametrize( - ("name", "version"), - [ - (None, None), - (None, ""), - ("", None), - (None, "1"), - ("pkg", None), - ("", "1"), - ("pkg", ""), - ], -) -def test_remove_pkg_missingNaveVersion(name, version, root, testapp): - msg = "Missing 'name'/'version' fields: name=%s, version=%s" - params = {":action": "remove_pkg", "name": name, "version": version} - params = dict((k, v) for k, v in params.items() if v is not None) - resp = testapp.post("/", expect_errors=1, params=params) - - assert resp.status == "400 Bad Request" - assert msg % (name, version) in unescape(resp.text) - - -def test_remove_pkg_notFound(root, testapp): - resp = testapp.post( - "/", - expect_errors=1, - params={":action": "remove_pkg", "name": "foo", "version": "123"}, - ) - assert resp.status == "404 Not Found" - assert "foo (123) not found" in unescape(resp.text) - - @pytest.mark.parametrize( "pkgs,matches", [ @@ -575,6 +543,96 @@ def test_search(root, testapp, search_xml, pkgs, matches): assert returned["version"] in [match[1] for match in matches] -@pytest.mark.xfail() -def test_remove_pkg(root, testapp): - assert 0 +class TestRemovePkg: + """The API allows removal of packages.""" + + @pytest.mark.parametrize( + "pkg, name, ver", + ( + ("test-1.0.tar.gz", "test", "1.0"), + ("test-1.0-py2-py3-none-any.whl", "test", "1.0"), + ), + ) + def test_remove_pkg(self, root, testapp, pkg, name, ver): + """Packages can be removed via POST.""" + root.join(pkg).write("") + assert len(os.listdir(str(root))) == 1 + params = {":action": "remove_pkg", "name": name, "version": ver} + testapp.post("/", params=params) + assert len(os.listdir(str(root))) == 0 + + @pytest.mark.parametrize( + "pkg, name, ver", + ( + ("test-1.0.tar.gz", "test", "1.0"), + ("test-1.0-py2-py3-none-any.whl", "test", "1.0"), + ), + ) + @pytest.mark.parametrize( + "other", + ( + "test-2.0.tar.gz", + "test-2.0-py2-py3-none-any.whl", + "other-1.0.tar.gz", + "other-1.0-py2-py3-none-any.whl", + ), + ) + def test_remove_pkg_only_targeted( + self, root, testapp, pkg, name, ver, other + ): + """Only the targeted package is removed.""" + root.join(pkg).write("") + root.join(other).write("") + assert len(os.listdir(str(root))) == 2 + params = {":action": "remove_pkg", "name": name, "version": ver} + testapp.post("/", params=params) + assert len(os.listdir(str(root))) == 1 + + @pytest.mark.parametrize( + "pkgs, name, ver", + ( + ( + ("test-1.0.tar.gz", "test-1.0-py2-py3-none-any.whl"), + "test", + "1.0", + ), + ), + ) + def test_all_instances_removed(self, root, testapp, pkgs, name, ver): + """Test that all instances of the target are removed.""" + for pkg in pkgs: + root.join(pkg).write("") + assert len(os.listdir(str(root))) == len(pkgs) + params = {":action": "remove_pkg", "name": name, "version": ver} + testapp.post("/", params=params) + assert len(os.listdir(str(root))) == 0 + + @pytest.mark.parametrize( + ("name", "version"), + [ + (None, None), + (None, ""), + ("", None), + (None, "1"), + ("pkg", None), + ("", "1"), + ("pkg", ""), + ], + ) + def test_remove_pkg_missingNaveVersion(self, name, version, root, testapp): + msg = "Missing 'name'/'version' fields: name=%s, version=%s" + params = {":action": "remove_pkg", "name": name, "version": version} + params = dict((k, v) for k, v in params.items() if v is not None) + resp = testapp.post("/", expect_errors=1, params=params) + + assert resp.status == "400 Bad Request" + assert msg % (name, version) in unescape(resp.text) + + def test_remove_pkg_notFound(self, root, testapp): + resp = testapp.post( + "/", + expect_errors=1, + params={":action": "remove_pkg", "name": "foo", "version": "123"}, + ) + assert resp.status == "404 Not Found" + assert "foo (123) not found" in unescape(resp.text)