Fix double upload bug and improve test

quetz/main.py

@@ -1395,9 +1395,6 @@ async def post_upload(
 
  dest = os.path.join(condainfo.info["subdir"], filename)
 
- body.seek(0)
- await pkgstore.add_package_async(body, channel_name, dest)
-
  package_name = str(condainfo.info.get("name"))
  package_data = rest_models.Package(
  name=package_name,
@@ -1434,6 +1431,9 @@ async def post_upload(
  logger.debug(f"duplicate package '{package_name}' in channel '{channel_name}'")
  raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Duplicate")
 
+ body.seek(0)
+ await pkgstore.add_package_async(body, channel_name, dest)
+
  pm.hook.post_add_package_version(version=version, condainfo=condainfo)
 
  wrapped_bg_task = background_task_wrapper(indexing.update_indexes, logger)
@@ -1478,7 +1478,7 @@ def _assert_filename_package_name_consistent(file_name: str, package_name: str):
  wait=wait_exponential(multiplier=1, min=4, max=10),
  after=after_log(logger, logging.WARNING),
 )
-def _extract_and_upload_package(file, channel_name, channel_proxylist):
+def _extract_and_upload_package(file, channel_name, channel_proxylist, force: bool):
  try:
  conda_info = CondaInfo(file.file, file.filename)
  except Exception as e:
@@ -1496,6 +1496,10 @@ def _extract_and_upload_package(file, channel_name, channel_proxylist):
  logger.info(f"Skip upload of proxied file {file.filename}")
  return conda_info
 
+ # Make sure that we do not upload over existing files
+ if pkgstore.file_exists(channel_name, dest) and not force:
+ raise FileExistsError(f"{file.filename}")
+
  try:
  file.file.seek(0)
  logger.debug(
@@ -1573,8 +1577,14 @@ def handle_package_files(
  files,
  (channel.name,) * len(files),
  (channel_proxylist,) * len(files),
+ (force,) * len(files),
  )
  ]
+ except FileExistsError as e:
+ raise HTTPException(
+ status_code=status.HTTP_409_CONFLICT,
+ detail=f"Duplicate {str(e)}",
+ )
  except exceptions.PackageError as e:
  raise HTTPException(
  status_code=status.HTTP_400_BAD_REQUEST, detail=e.detail

quetz/tests/api/test_main_packages.py

@@ -1,9 +1,10 @@
 import hashlib
 import json
 import os
+import shutil
 import time
 from pathlib import Path
-from typing import BinaryIO, Callable, Union
+from typing import BinaryIO, Callable, Tuple, Union
 
 import pytest
 
@@ -333,12 +334,12 @@ def _upload_file_1(
  auth_client,
  public_channel,
  public_package,
- package_filename: str,
+ filepath: Path,
  force: bool = False,
 ):
  """Upload a file using /channels/{channel_name}/packages/{package_name}/files/"""
- with open(package_filename, "rb") as fid:
- files = {"files": (package_filename, fid)}
+ with open(filepath, "rb") as fid:
+ files = {"files": (filepath.name, fid)}
  response = auth_client.post(
  f"/api/channels/{public_channel.name}/packages/"
  f"{public_package.name}/files/",
@@ -352,15 +353,15 @@ def _upload_file_2(
  auth_client,
  public_channel,
  public_package,
- package_filename: str,
+ filepath: Path,
  force: bool = False,
 ):
  """Upload a file using /channels/{channel_name}/upload/{file_name}"""
 
- with open(package_filename, "rb") as fid:
+ with open(filepath, "rb") as fid:
  body_bytes = fid.read()
  response = auth_client.post(
- f"/api/channels/{public_channel.name}/upload/{package_filename}",
+ f"/api/channels/{public_channel.name}/upload/{filepath.name}",
  content=body_bytes,
  params={"force": force, "sha256": hashlib.sha256(body_bytes).hexdigest()},
  )
@@ -384,7 +385,7 @@ def test_upload_package_version_wrong_filename(
  os.rename("test-package-0.1-0.tar.bz2", package_filename)
 
  response = upload_function(
- auth_client, public_channel, public_package, package_filename
+ auth_client, public_channel, public_package, Path(package_filename)
  )
 
  package_dir = Path(pkgstore.channels_dir) / public_channel.name / 'linux-64'
@@ -397,12 +398,14 @@ def test_upload_package_version_wrong_filename(
  assert not os.path.exists(package_dir)
 
 
-def sha256(path: Union[Path, str]) -> str:
+def sha_and_md5(path: Union[Path, str]) -> Tuple[str, str]:
  sha = hashlib.sha256()
+ md5 = hashlib.md5()
  with open(path, "rb") as f:
  for chunk in iter(lambda: f.read(2**16), b""):
  sha.update(chunk)
- return sha.hexdigest()
+ md5.update(chunk)
+ return sha.hexdigest(), md5.hexdigest()
 
 
 @pytest.mark.parametrize("upload_function", [_upload_file_1, _upload_file_2])
@@ -416,78 +419,128 @@ def test_upload_duplicate_package_version(
  config,
  remove_package_versions,
  upload_function: Callable,
+ tmp_path,
 ):
  pkgstore = config.get_package_store()
 
- package_filename = "test-package-0.1-0.tar.bz2"
- package_filename_copy = "test-package-0.1-0_copy.tar.bz2"
+ def get_repodata():
+ """Helper function to read repo data"""
+ package_dir = Path(pkgstore.channels_dir) / public_channel.name / 'linux-64'
+ return json.loads((package_dir / 'repodata.json').read_text())
+
+ # Test setup: path1 is a package we will upload
+ path1 = Path(__file__).parent.parent / "data" / "test-package-0.1-0.tar.bz2"
+
+ # To test duplicate uploads, we have a second file `_copy`, which is the same
+ # package and version but has a different content, so different hashes
+ # We must move this file to a temporary directory so that we can give it the same
+ # name as the first file.
+ path2 = tmp_path / path1.name
+ shutil.copyfile(
+ Path(__file__).parent.parent / "data" / "test-package-0.1-0_copy.tar.bz2", path2
+ )
 
- file_sha = sha256(package_filename)
- file_copy_sha = sha256(package_filename_copy)
+ # Sanity checks
+ sha1, md51 = sha_and_md5(path1)
+ sha2, md52 = sha_and_md5(path2)
+ assert (sha1 != sha2) and (
+ md51 != md52
+ ), "Sanity check failure: Test files have same hash"
  assert (
- file_sha != file_copy_sha
- ), "Sanity check: Test files must have different hashes for this test."
- file_size = os.path.getsize(package_filename)
- file_copy_size = os.path.getsize(package_filename_copy)
+ path1.name == path2.name
+ ), "Sanity check failure: Test files have different name"
+
+ size1 = os.path.getsize(path1)
+ size2 = os.path.getsize(path2)
  assert (
- file_size != file_copy_size
- ), "Sanity check: Test files must have different sizes for this test."
+ size1 != size2
+ ), "Sanity check failure: Test files must have different sizes for this test."
 
- upload_function(auth_client, public_channel, public_package, package_filename)
+ # First upload
+ # File should not exist
+ assert not pkgstore.file_exists(public_channel.name, f"linux-64/{path1.name}")
+ response = upload_function(auth_client, public_channel, public_package, path1)
 
- def get_repodata():
- """Helper function to read repo data"""
- package_dir = Path(pkgstore.channels_dir) / public_channel.name / 'linux-64'
- return json.loads((package_dir / 'repodata.json').read_text())
+ # Expect success
+ assert response.status_code == 201
 
- repodata_init = get_repodata()
- assert repodata_init["packages"][package_filename]["sha256"] == file_sha
+ # Check meta data is OK
+ repodata_after_first = get_repodata()
+ assert repodata_after_first["packages"][path1.name]["sha256"] == sha1
+ assert repodata_after_first["packages"][path1.name]["md5"] == md51
+ assert repodata_after_first["packages"][path1.name]["size"] == size1
 
- # Try submitting the same package without 'force' flag
- response = upload_function(
- auth_client, public_channel, public_package, package_filename
+ # Check that the file in the store is OK
+ file_in_store = (
+ Path(pkgstore.channels_dir) / public_channel.name / 'linux-64' / path1.name
  )
+ assert size1 == os.path.getsize(file_in_store)
+ assert (sha1, md51) == sha_and_md5(file_in_store)
+
+ # Second upload: File with same name but different content, without force
+ response = upload_function(auth_client, public_channel, public_package, path2)
 
- # Expect 409 here
+ # Expect 409 since the file already exists
  assert response.status_code == 409
  detail = response.json()['detail']
  assert "Duplicate" in detail
 
- # Change the archive to test force update
- os.remove(package_filename)
- os.rename(package_filename_copy, package_filename)
+ # Check meta data is OK: It should not have changed with respect to before
+ repodata_after_second = get_repodata()
+ assert repodata_after_second == repodata_after_first
 
+ # Check that the file in the store is OK
+ file_in_store = (
+ Path(pkgstore.channels_dir) / public_channel.name / 'linux-64' / path1.name
+ )
+
+ # File in store should not be the second file
+ assert not (
+ (sha2, md52) == sha_and_md5(file_in_store)
+ ), "Duplicate upload without force updated stored file."
+ assert not (
+ size2 == os.path.getsize(file_in_store)
+ ), "Duplicate upload without force updated stored file."
+
+ # File in store should be the first file
+ assert size1 == os.path.getsize(
+ file_in_store
+ ), "Duplicate upload without force updated stored file."
+ assert (sha1, md51) == sha_and_md5(
+ file_in_store
+ ), "Duplicate upload without force updated stored file."
+
+ # Third upload: Same as second but now with force
  # Ensure the 'time_modified' value changes in repodata.json
  time.sleep(1)
 
  # Submit the same package with 'force' flag
  response = upload_function(
- auth_client, public_channel, public_package, package_filename, force=True
+ auth_client, public_channel, public_package, path2, force=True
  )
  assert response.status_code == 201
 
  # Check that repodata content has been updated
- repodata = get_repodata()
+ repodata_after_force = get_repodata()
 
  # Info should match
- assert repodata_init["info"] == repodata["info"]
+ assert repodata_after_first["info"] == repodata_after_force["info"]
 
  # Package keys should match
- assert repodata_init["packages"].keys() == repodata["packages"].keys()
- repodata_init_pkg = repodata_init["packages"][package_filename]
- repodata_pkg = repodata["packages"][package_filename]
-
- # Hashes should match pre-upload expectation
- assert repodata_init_pkg["sha256"] == file_sha
- assert repodata_pkg["sha256"] == file_copy_sha
+ assert (
+ repodata_after_first["packages"].keys()
+ == repodata_after_force["packages"].keys()
+ )
 
- # Sizes should match pre-upload expectation
- assert repodata_init_pkg["size"] == file_size
- assert repodata_pkg["size"] == file_copy_size
+ # Hashes should now have changed to the second file
+ assert repodata_after_force["packages"][path1.name]["sha256"] == sha2
+ assert repodata_after_force["packages"][path1.name]["md5"] == md52
+ assert repodata_after_force["packages"][path1.name]["size"] == size2
 
- # Upload-related metadata should not match
- for key in "time_modified", "md5", "sha256":
- assert repodata_init_pkg[key] != repodata_pkg[key]
+ assert (
+ repodata_after_force["packages"][path1.name]["time_modified"]
+ > repodata_after_first["packages"][path1.name]["time_modified"]
+ )
 
 
 @pytest.mark.parametrize("package_name", ["test-package"])