Repository: conan-io/conan
Pull request: fixing mixing apis for download cache
Changelog: Fix: Avoid caching revision "0" under api V2 (revisions enabled) in the download cache.
Docs: https://github.com/conan-io/docs/pull/1552

Fix https://github.com/conan-io/conan/issues/6467


Problem statement:
[bug] Download cache should handle v1 and v2 clients simultaneously
Download cache uses URLs for caching things in api V2, which works well for revisioned things, because the revision is a hash. But this fails if clients using v1 keep re-uploading the revision "0", which shouldn't be cached. 
To do:

- Implement return of checksums in Api V2 (@jgsogo). The api is prepared for that.
- Skip the caching of revision "0" artifacts in Api V2.

Hints from issue discussion:
What do you mean with...?
> Implement return of checksums in Api V2
```python

class RestV2Methods(RestCommonMethods):

    def _get_file_list_json(self, url):
        data = self.get_json(url)
        # Discarding (.keys()) still empty metadata for files
        data["files"] = list(data["files"].keys())
        return data
```

The api is prepared so data can be retrieved when asked for files in the server. We should return here the checksum of the files, as it is done in the "snapshot" methods of api V1, and propagate those checksums. The first necessary change is to implement that in the server side.

Commit message: removed defaults

Patch to review:
diff --git a/conans/client/rest/rest_client_v2.py b/conans/client/rest/rest_client_v2.py
index 99dc9db1ffe..0cc5b4929c8 100644
--- a/conans/client/rest/rest_client_v2.py
+++ b/conans/client/rest/rest_client_v2.py
@@ -37,10 +37,10 @@ def _get_file_list_json(self, url):
         data["files"] = list(data["files"].keys())
         return data
 
-    def _get_remote_file_contents(self, url):
+    def _get_remote_file_contents(self, url, cache):
         # We don't want traces in output of these downloads, they are ugly in output
         downloader = FileDownloader(self.requester, None, self.verify_ssl, self._config)
-        if self._config.download_cache:
+        if cache and self._config.download_cache:
             downloader = CachedFileDownloader(self._config.download_cache, downloader)
         contents = downloader.download(url, auth=self.auth)
         return contents
@@ -58,12 +58,14 @@ def get_recipe_manifest(self, ref):
         if not ref.revision:
             ref = self.get_latest_recipe_revision(ref)
         url = self.router.recipe_manifest(ref)
-        content = self._get_remote_file_contents(url)
+        cache = (ref.revision != "0")
+        content = self._get_remote_file_contents(url, cache=cache)
         return FileTreeManifest.loads(decode_text(content))
 
     def get_package_manifest(self, pref):
         url = self.router.package_manifest(pref)
-        content = self._get_remote_file_contents(url)
+        cache = (pref.revision != "0")
+        content = self._get_remote_file_contents(url, cache=cache)
         try:
             return FileTreeManifest.loads(decode_text(content))
         except Exception as e:
@@ -75,7 +77,8 @@ def get_package_manifest(self, pref):
 
     def get_package_info(self, pref):
         url = self.router.package_info(pref)
-        content = self._get_remote_file_contents(url)
+        cache = (pref.revision != "0")
+        content = self._get_remote_file_contents(url, cache=cache)
         return ConanInfo.loads(decode_text(content))
 
     def get_recipe(self, ref, dest_folder):
@@ -88,7 +91,8 @@ def get_recipe(self, ref, dest_folder):
 
         # If we didn't indicated reference, server got the latest, use absolute now, it's safer
         urls = {fn: self.router.recipe_file(ref, fn) for fn in files}
-        self._download_and_save_files(urls, dest_folder, files)
+        cache = (ref.revision != "0")
+        self._download_and_save_files(urls, dest_folder, files, cache=cache)
         ret = {fn: os.path.join(dest_folder, fn) for fn in files}
         return ret
 
@@ -106,7 +110,8 @@ def get_recipe_sources(self, ref, dest_folder):
 
         # If we didn't indicated reference, server got the latest, use absolute now, it's safer
         urls = {fn: self.router.recipe_file(ref, fn) for fn in files}
-        self._download_and_save_files(urls, dest_folder, files)
+        cache = (ref.revision != "0")
+        self._download_and_save_files(urls, dest_folder, files, cache=cache)
         ret = {fn: os.path.join(dest_folder, fn) for fn in files}
         return ret
 
@@ -117,7 +122,8 @@ def get_package(self, pref, dest_folder):
         check_compressed_files(PACKAGE_TGZ_NAME, files)
         # If we didn't indicated reference, server got the latest, use absolute now, it's safer
         urls = {fn: self.router.package_file(pref, fn) for fn in files}
-        self._download_and_save_files(urls, dest_folder, files)
+        cache = (pref.revision != "0")
+        self._download_and_save_files(urls, dest_folder, files, cache=cache)
         ret = {fn: os.path.join(dest_folder, fn) for fn in files}
         return ret
 
@@ -128,7 +134,8 @@ def get_recipe_path(self, ref, path):
             return self._list_dir_contents(path, files)
         else:
             url = self.router.recipe_file(ref, path)
-            content = self._get_remote_file_contents(url)
+            cache = (ref.revision != "0")
+            content = self._get_remote_file_contents(url, cache=cache)
             return decode_text(content)
 
     def get_package_path(self, pref, path):
@@ -136,7 +143,8 @@ def get_package_path(self, pref, path):
         url = self.router.package_snapshot(pref)
         files = self._get_file_list_json(url)
         if self._is_dir(path, files):
-            return self._list_dir_contents(path, files)
+            cache = (pref.revision != "0")
+            return self._list_dir_contents(path, files, cache=cache)
         else:
             url = self.router.package_file(pref, path)
             content = self._get_remote_file_contents(url)
@@ -205,9 +213,9 @@ def _upload_files(self, files, urls, retry, retry_wait, display_name=None):
         else:
             logger.debug("\nUPLOAD: All uploaded! Total time: %s\n" % str(time.time() - t1))
 
-    def _download_and_save_files(self, urls, dest_folder, files):
+    def _download_and_save_files(self, urls, dest_folder, files, cache):
         downloader = FileDownloader(self.requester, self._output, self.verify_ssl, self._config)
-        if self._config.download_cache:
+        if cache and self._config.download_cache:
             downloader = CachedFileDownloader(self._config.download_cache, downloader)
         # Take advantage of filenames ordering, so that conan_package.tgz and conan_export.tgz
         # can be < conanfile, conaninfo, and sent always the last, so smaller files go first
diff --git a/conans/test/functional/download_cache_test.py b/conans/test/functional/download_cache_test.py
index 986017e9f55..0a324306ba2 100644
--- a/conans/test/functional/download_cache_test.py
+++ b/conans/test/functional/download_cache_test.py
@@ -168,6 +168,41 @@ def source(self):
         self.assertIn("ERROR: conanfile.py: Error in source() method, line 7", client.out)
         self.assertIn("Not found: http://localhost", client.out)
 
+    @unittest.skipIf(get_env("TESTING_REVISIONS_ENABLED", False), "Hybrid test with both v1 and v2")
+    def test_revision0_v2_skip(self):
+        client = TestClient(default_server_user=True)
+        client.run("config set general.revisions_enabled=False")
+        conanfile = textwrap.dedent("""
+            from conans import ConanFile
+            class Pkg(ConanFile):
+                exports = "*"
+                def package(self):
+                    self.copy("*")
+                def deploy(self):
+                    self.copy("*")
+            """)
+        client.save({"conanfile.py": conanfile,
+                     "header.h": "header"})
+        client.run("create . mypkg/0.1@user/testing")
+        client.run("upload * --all --confirm")
+
+        client2 = TestClient(servers=client.servers)
+        client2.run("config set general.revisions_enabled=True")
+        cache_folder = temp_folder()
+        client2.run('config set storage.download_cache="%s"' % cache_folder)
+        client2.run("install mypkg/0.1@user/testing")
+        self.assertEqual("header", client2.load("header.h"))
+
+        # modify non-revisioned pkg
+        client.save({"conanfile.py": conanfile,
+                     "header.h": "header2"})
+        client.run("create . mypkg/0.1@user/testing")
+        client.run("upload * --all --confirm")
+
+        client2.run("remove * -f")
+        client2.run("install mypkg/0.1@user/testing")
+        self.assertEqual("header2", client2.load("header.h"))
+
 
 class CachedDownloaderUnitTest(unittest.TestCase):
     def setUp(self):


Code review comments:
diff_hunk: @@ -58,12 +58,14 @@ def get_recipe_manifest(self, ref):
         if not ref.revision:
             ref = self.get_latest_recipe_revision(ref)
         url = self.router.recipe_manifest(ref)
-        content = self._get_remote_file_contents(url)
+        cache = (ref.revision != "0")
path: conans/client/rest/rest_client_v2.py
text: @user1:
In the sources, when we refer to this _magic number_ we use `DEFAULT_REVISION_V1`

```suggestion
        cache = (ref.revision != DEFAULT_REVISION_V1)
```

diff_hunk: @@ -37,10 +37,10 @@ def _get_file_list_json(self, url):
         data["files"] = list(data["files"].keys())
         return data
 
-    def _get_remote_file_contents(self, url):
+    def _get_remote_file_contents(self, url, cache):
path: conans/client/rest/rest_client_v2.py
text: @user1:
```suggestion
    def _get_remote_file_contents(self, url, use_cache):
```

Can you rename it to `use_cache`?