Repository: conan-io/conan
Pull request: Do not fail with 'conan remove -r remote -p' if there are no packages in the remote
Changelog: Bugfix: Do not fail for `conan remove -r remote -p` when there are no packages in the remote.
Docs: omit

Check there are no packages before trying to remove the package folder in the server. It prevents a 404 error.

close https://github.com/conan-io/conan/issues/7332

#REVISIONS: 1

Problem statement:
[bug] conan remove fails when using -p without package id together with -r
When I do `conan remove -f -p -r <remote> <existing_reference>` with a reference for which there is only a recipe but no binary package on the remote Conan fails with the following error message:

```
ERROR: {
  "errors" : [ {
    "status" : 404,
    "message" : "Couldn't find 'user/pkg/0.1.0/develop/d8cc7d7e1317dcbc71d65614521acffd/package'"
  } ]
}. [Remote: my-remote]
```

I would expect it to succeed even if there is nothing to do.

Hints from issue discussion:
It seems that the `-p` option without parameters is broken even when there is a binary package on the remote. I still get the same error.
It totally makes sense. Just to confirm, can you report the server you are using? Artifactory? Which version? Is there any special configuration like virtual repositories?

Thanks!
We're using Artifactory 6.17.0, no virtual repositories, revisions enabled.
I guess when no package ID is specified Conan would have to use the Artifactory API to list the available packages and then iterate on that list to delete them individually. Probably that functionality is currently missing.
Here are some traces

```
{"_action": "COMMAND", "name": "remove", "parameters": {"builds": null, "force": true, "outdated": false, "packages": [], "pattern": "EDP_Assign/1.8.0@sick/develop", "query": null, "remote_name": "edp-conan-local", "src": false}, "time": 1594288541.797875}
{"_action": "REST_API_CALL", "duration": 0.055852413177490234, "headers": {"User-Agent": "Conan/1.27.0 (Python 3.7.2) python-requests/2.21.0", "X-Client-Anonymous-Id": "**********", "X-Client-Id": ""}, "method": "GET", "time": 1594288541.8786561, "url": "https://myartifactory/artifactory/api/conan/edp-conan-local/v1/ping"}
{"_action": "REST_API_CALL", "duration": 0.00894618034362793, "headers": {"User-Agent": "Conan/1.27.0 (Python 3.7.2) python-requests/2.21.0", "X-Client-Anonymous-Id": "**********", "X-Client-Id": ""}, "method": "GET", "time": 1594288541.894613, "url": "https://myartifactory/artifactory/api/conan/edp-conan-local/v2/users/check_credentials"}
{"_action": "REST_API_CALL", "duration": 0.011967897415161133, "headers": {"User-Agent": "Conan/1.27.0 (Python 3.7.2) python-requests/2.21.0", "X-Client-Anonymous-Id": "**********", "X-Client-Id": ""}, "method": "GET", "time": 1594288541.9145598, "url": "https://myartifactory/artifactory/api/conan/edp-conan-local/v2/conans/EDP_Assign/1.8.0/sick/develop/revisions"}
{"_action": "REST_API_CALL", "duration": 0.009973287582397461, "headers": {"User-Agent": "Conan/1.27.0 (Python 3.7.2) python-requests/2.21.0", "X-Client-Anonymous-Id": "**********", "X-Client-Id": ""}, "method": "DELETE", "time": 1594288541.932512, "url": "https://myartifactory/artifactory/api/conan/edp-conan-local/v2/conans/EDP_Assign/1.8.0/sick/develop/revisions/235391/packages"}
{"_action": "EXCEPTION", "class": "NotFoundException", "message": "{\n  \"errors\" : [ {\n    \"status\" : 404,\n    \"message\" : \"Couldn't find 'sick/EDP_Assign/1.8.0/develop/235391/package'\"\n  } ]\n}. [Remote: edp-conan-local]", "time": 1594288541.9424858}
```
There's this function in `conans/server/store/server_store.py` which just unconditionally assumes that there must be a `package` folder:

```python
def remove_all_packages(self, ref):
    assert ref.revision is not None, "BUG: server store needs RREV remove_all_packages"
    assert isinstance(ref, ConanFileReference)
    packages_folder = self.packages(ref)
    self._storage_adapter.delete_folder(packages_folder)
```
Thanks for the investigation! You are one step away from creating the PR 😉 
Hm not sure, that looks like some kind of server side implementation to me because the only storage adapter that I can find is a disk storage adapter. I guess that's not part of the callstack on the client side. Trying to look into this some more but probably I could use some help ;)
Unless that's actually the implementation on Artifactory side. Then a fix would require us to update Artifactory which is not what I want to hear...
The Conan API with Artifactory is quite straightforward, as you can see from your logs, Artifactory returns an error if the resource you are requesting to DELETE doesn't exist 

```
{"_action": "REST_API_CALL", "duration": 0.009973287582397461, "headers": {"User-Agent": "Conan/1.27.0 (Python 3.7.2) python-requests/2.21.0", "X-Client-Anonymous-Id": "**********", "X-Client-Id": ""}, "method": "DELETE", "time": 1594288541.932512, "url": "https://myartifactory/artifactory/api/conan/edp-conan-local/v2/conans/EDP_Assign/1.8.0/sick/develop/revisions/235391/packages"}
```

This is a design decision in Artifactory that will affect every other package manager and I'm sure it is something we have _to live with_, so we need to implement the **logic in the client-side** (it will take us just one release, it should be really easy):
 a) my first approach would be to check if the message/error returned by Artifactory it is clear about what has happened, Conan can capture that concrete error and ignore it safely.
 b) if that is not possible, Conan can list the packages in the server and, if it is empty, do not call the `remove_all_packages` function.
 c) (Maybe looking at the sources, a `try/catch-pass` around `remove_all_packages` is enough... )

Commit message: if not conaninfo, search doesn't work

Patch to review:
diff --git a/conans/client/rest/rest_client_v1.py b/conans/client/rest/rest_client_v1.py
index ad977f59994..5b4d5c9c6da 100644
--- a/conans/client/rest/rest_client_v1.py
+++ b/conans/client/rest/rest_client_v1.py
@@ -1,6 +1,7 @@
 import os
 import time
 import traceback
+from collections import namedtuple
 
 from six.moves.urllib.parse import parse_qs, urljoin, urlparse, urlsplit
 
@@ -306,10 +307,13 @@ def _remove_conanfile_files(self, ref, files):
     @handle_return_deserializer()
     def remove_packages(self, ref, package_ids=None):
         """ Remove any packages specified by package_ids"""
-        self.check_credentials()
-        payload = {"package_ids": package_ids}
-        url = self.router.remove_packages(ref)
-        return self._post_json(url, payload)
+        pcks = self.search_packages(ref, query=None)
+        if pcks:
+            self.check_credentials()
+            payload = {"package_ids": package_ids}
+            url = self.router.remove_packages(ref)
+            return self._post_json(url, payload)
+        return namedtuple("_", ['status_code', 'content'])(200, b'')
 
     @handle_return_deserializer()
     def remove_conanfile(self, ref):
diff --git a/conans/client/rest/rest_client_v2.py b/conans/client/rest/rest_client_v2.py
index af997117057..f0082fb52a0 100644
--- a/conans/client/rest/rest_client_v2.py
+++ b/conans/client/rest/rest_client_v2.py
@@ -246,13 +246,16 @@ def remove_packages(self, ref, package_ids=None):
         for ref in refs:
             assert ref.revision is not None, "remove_packages needs RREV"
             if not package_ids:
-                url = self.router.remove_all_packages(ref)
-                response = self.requester.delete(url, auth=self.auth, headers=self.custom_headers,
-                                                 verify=self.verify_ssl)
-                if response.status_code != 200:  # Error message is text
-                    # To be able to access ret.text (ret.content are bytes)
-                    response.charset = "utf-8"
-                    raise get_exception_from_error(response.status_code)(response.text)
+                # Check there are packages to remove
+                package_search_url = self.router.search_packages(ref)
+                if self.get_json(package_search_url):
+                    url = self.router.remove_all_packages(ref)
+                    response = self.requester.delete(url, auth=self.auth, verify=self.verify_ssl,
+                                                     headers=self.custom_headers)
+                    if response.status_code != 200:  # Error message is text
+                        # To be able to access ret.text (ret.content are bytes)
+                        response.charset = "utf-8"
+                        raise get_exception_from_error(response.status_code)(response.text)
             else:
                 for pid in package_ids:
                     pref = PackageReference(ref, pid)
diff --git a/conans/test/functional/command/remote_test.py b/conans/test/functional/command/remote_test.py
index ece905ef876..9dcf06d6645 100644
--- a/conans/test/functional/command/remote_test.py
+++ b/conans/test/functional/command/remote_test.py
@@ -3,9 +3,10 @@
 import unittest
 from collections import OrderedDict
 
+from conans.model.ref import ConanFileReference
+from conans.test.utils.genconanfile import GenConanfile
 from conans.test.utils.tools import NO_SETTINGS_PACKAGE_ID, TestClient, TestServer
 from conans.util.files import load
-from conans.model.ref import ConanFileReference
 
 
 class RemoteTest(unittest.TestCase):
@@ -21,12 +22,7 @@ def setUp(self):
         self.client = TestClient(servers=self.servers, users=self.users)
 
     def test_removed_references(self):
-        conanfile = """
-from conans import ConanFile
-class HelloConan(ConanFile):
-    pass
-"""
-        self.client.save({"conanfile.py": conanfile})
+        self.client.save({"conanfile.py": GenConanfile()})
         self.client.run("create . lib/1.0@lasote/channel")
         self.client.run('upload "*" -c -r remote1')
         self.client.run('upload "*" -c -r remote2')
@@ -86,7 +82,7 @@ class HelloConan(ConanFile):
 
     def list_raw_test(self):
         self.client.run("remote list --raw")
-        output = re.sub(r"http:\/\/fake.+\.com", "http://fake.com", str(self.client.out))
+        output = re.sub(r"http://fake.+.com", "http://fake.com", str(self.client.out))
         self.assertIn("remote0 http://fake.com True", output)
         self.assertIn("remote1 http://fake.com True", output)
         self.assertIn("remote2 http://fake.com True", output)
@@ -342,12 +338,10 @@ def invalid_remote_disable_test(self):
         client = TestClient()
 
         client.run("remote disable invalid_remote", assert_error=True)
-        self.assertIn("ERROR: Remote 'invalid_remote' not found in remotes",
-                      client.out)
+        self.assertIn("ERROR: Remote 'invalid_remote' not found in remotes", client.out)
 
         client.run("remote enable invalid_remote", assert_error=True)
-        self.assertIn("ERROR: Remote 'invalid_remote' not found in remotes",
-                      client.out)
+        self.assertIn("ERROR: Remote 'invalid_remote' not found in remotes", client.out)
 
         client.run("remote disable invalid_wildcard_*")
 
@@ -390,12 +384,10 @@ def duplicated_error_tests(self):
         self.client.run("remote list")
         url = str(self.client.out).split()[1]
         self.client.run("remote add newname %s" % url, assert_error=True)
-        self.assertIn("Remote 'remote0' already exists with same URL",
-                      self.client.out)
+        self.assertIn("Remote 'remote0' already exists with same URL", self.client.out)
 
         self.client.run("remote update remote1 %s" % url, assert_error=True)
-        self.assertIn("Remote 'remote0' already exists with same URL",
-                      self.client.out)
+        self.assertIn("Remote 'remote0' already exists with same URL", self.client.out)
 
     def basic_refs_test(self):
         self.client.run("remote add_ref Hello/0.1@user/testing remote0")
@@ -468,9 +460,7 @@ def test_metadata_editable_packages(self):
         """
         Check that 'conan remote' commands work with editable packages
         """
-        self.client.save({"conanfile.py": """from conans import ConanFile
-class Conan(ConanFile):
-    pass"""})
+        self.client.save({"conanfile.py": GenConanfile()})
         self.client.run("create . pkg/1.1@lasote/stable")
         self.client.run("upload pkg/1.1@lasote/stable --all -c --remote remote1")
         self.client.run("remove -f pkg/1.1@lasote/stable")
@@ -503,3 +493,9 @@ class Conan(ConanFile):
         self.client.run("remote list")
         self.assertNotIn("remote1", self.client.out)
         self.assertNotIn("remote0", self.client.out)
+
+    def test_remove_package_empty(self):
+        self.client.save({"conanfile.py": GenConanfile("name", "version")})
+        self.client.run("export . name/version@lasote/stable")
+        self.client.run("upload name/version@lasote/stable --remote remote1")
+        self.client.run("remove -f -p -r remote1 name/version@lasote/stable")
diff --git a/conans/test/functional/remote/rest_api_test.py b/conans/test/functional/remote/rest_api_test.py
index ae024354c3e..15d7afe5397 100644
--- a/conans/test/functional/remote/rest_api_test.py
+++ b/conans/test/functional/remote/rest_api_test.py
@@ -7,13 +7,13 @@
 from nose.plugins.attrib import attr
 
 from conans import DEFAULT_REVISION_V1
-from conans.client.userio import UserIO
-from conans.client.remote_manager import Remote
 from conans.client.conf import ConanClientConfigParser
+from conans.client.remote_manager import Remote
 from conans.client.rest.auth_manager import ConanApiAuthManager
 from conans.client.rest.conan_requester import ConanRequester
 from conans.client.rest.rest_client import RestApiClientFactory
 from conans.client.rest.rest_client_v1 import complete_url
+from conans.client.userio import UserIO
 from conans.model.info import ConanInfo
 from conans.model.manifest import FileTreeManifest
 from conans.model.ref import ConanFileReference, PackageReference
@@ -231,11 +231,14 @@ def remove_packages_test(self):
         for sha in ["1", "2", "3", "4", "5"]:
             # Upload an package
             pref = PackageReference(ref, sha, DEFAULT_REVISION_V1)
-            self._upload_package(pref)
+            self._upload_package(pref, {CONANINFO: ""})
             folder = self.server.server_store.package(pref)
             self.assertTrue(os.path.exists(folder))
             folders[sha] = folder
 
+        data = self.api.search_packages(ref, None)
+        self.assertEqual(len(data), 5)
+
         self.api.remove_packages(ref, ["1"])
         self.assertTrue(os.path.exists(self.server.server_store.base_folder(ref)))
         self.assertFalse(os.path.exists(folders["1"]))


Code review comments:
diff_hunk: @@ -246,13 +246,16 @@ def remove_packages(self, ref, package_ids=None):
         for ref in refs:
             assert ref.revision is not None, "remove_packages needs RREV"
             if not package_ids:
-                url = self.router.remove_all_packages(ref)
-                response = self.requester.delete(url, auth=self.auth, headers=self.custom_headers,
-                                                 verify=self.verify_ssl)
-                if response.status_code != 200:  # Error message is text
-                    # To be able to access ret.text (ret.content are bytes)
-                    response.charset = "utf-8"
-                    raise get_exception_from_error(response.status_code)(response.text)
+                # Check there are packages to remove
+                package_search_url = self.router.search_packages(ref)
path: conans/client/rest/rest_client_v2.py
text: @user1:
The ``search_packages`` is  quite slow, tbh, I am not very excited about this approach. I would say:

- This seems something to be fixed in the server side. I think it is a bug and can be fixed, it only belongs to the Conan API
- In the meantime, I would try to workaround it in the client side with a try - except 404 and only checking the ``search_packages()`` in that case. But annotating clearly it is a temporary workaround.

diff_hunk: @@ -306,10 +307,13 @@ def _remove_conanfile_files(self, ref, files):
     @handle_return_deserializer()
     def remove_packages(self, ref, package_ids=None):
         """ Remove any packages specified by package_ids"""
-        self.check_credentials()
-        payload = {"package_ids": package_ids}
-        url = self.router.remove_packages(ref)
-        return self._post_json(url, payload)
+        pcks = self.search_packages(ref, query=None)
path: conans/client/rest/rest_client_v1.py
text: @user1:
I would only do the search if ``package_ids is None``, in other cases it should still raise.

Actually, I'd prefer capturing the raise, checking if package_ids is None, and re-raising if not, but I guess it is not easy to differentiate the 404 that the ``packages`` folder is missing or the 404 that the recipe is missing.