Merge pull request #4810 from opensafely-core/allow-release-reuploads

Allow release reuploads

jobserver/api/releases.py

@@ -302,12 +302,9 @@ def post(self, request, workspace_name):
         metadata = serializer.validated_data["metadata"]
         release_id = metadata.pop("airlock_id", None)
 
-        try:
-            release = releases.create_release(
-                workspace, backend, user, files, metadata=metadata, id=release_id
-            )
-        except releases.ReleaseFileAlreadyExists as exc:
-            raise ValidationError({"detail": str(exc)})
+        release = releases.create_release(
+            workspace, backend, user, files, metadata=metadata, id=release_id
+        )
 
         slacks.notify_release_created(release)
 

jobserver/releases.py

@@ -69,44 +69,35 @@ def build_outputs_zip(release_files, url_builder_func):
     return in_memory_zf
 
 
-def check_not_already_uploaded(workspace, filename, filehash, backend):
-    """Check if this workspace/filename/filehash combination has been uploaded before."""
-    duplicate = ReleaseFile.objects.filter(
-        release__backend=backend,
-        workspace=workspace,
-        name=filename,
-        filehash=filehash,
-    )
-    if duplicate.exists():
-        raise ReleaseFileAlreadyExists(
-            f"This version of '{filename}' in workspace {workspace} has already been uploaded from backend '{backend.slug}'"
-        )
-
-
 @transaction.atomic
 def create_release(workspace, backend, created_by, requested_files, **kwargs):
-    for f in requested_files:
-        check_not_already_uploaded(workspace, f["name"], f["sha256"], backend)
-
-    release = Release.objects.create(
+    release_id = kwargs.pop("id", None)
+    create_kwargs = dict(
         workspace=workspace,
         backend=backend,
-        created_by=created_by,
         requested_files=requested_files,
-        **kwargs,
+        defaults={
+            "created_by": created_by,
+            **kwargs,
+        },
     )
+    if release_id is not None:
+        create_kwargs["id"] = release_id
 
-    for f in requested_files:
-        ReleaseFile.objects.create(
-            release=release,
-            workspace=release.workspace,
-            created_by=created_by,
-            name=f["name"],
-            filehash=f["sha256"],
-            size=f["size"],
-            mtime=f["date"],
-            metadata=f["metadata"],
-        )
+    release, created = Release.objects.get_or_create(**create_kwargs)
+
+    if created:
+        for f in requested_files:
+            ReleaseFile.objects.create(
+                release=release,
+                workspace=release.workspace,
+                created_by=created_by,
+                name=f["name"],
+                filehash=f["sha256"],
+                size=f["size"],
+                mtime=f["date"],
+                metadata=f["metadata"],
+            )
 
     return release
 

tests/unit/jobserver/api/test_releases.py

@@ -816,35 +816,79 @@ def test_releaseworkspaceapi_post_create_release_with_oversized_file(
     assert response.data["files"][0]["size"][0].startswith("File size should be <16Mb.")
 
 
-def test_releaseworkspaceapi_post_release_already_exists(api_rf, project_membership):
-    user = UserFactory(roles=[OutputChecker])
-    release = ReleaseFactory()
-    rfile = ReleaseFileFactory(
-        release=release,
-        workspace=release.workspace,
-        created_by=user,
-        name="file.txt",
-        filehash="hash",
+def test_releaseworkspaceapi_post_release_already_exists(
+    api_rf, slack_messages, project_membership
+):
+    user = UserFactory(roles=[OutputChecker], username="test-output-checker")
+    files = [
+        {
+            "name": "file.txt",
+            "url": "url",
+            "size": 7,
+            "sha256": "hash",
+            "date": timezone.now(),
+            "metadata": {},
+            "review": None,
+        }
+    ]
+    release = ReleaseFactory(requested_files=files, created_by=user)
+    BackendMembershipFactory(backend=release.backend, user=user)
+    project_membership(project=release.workspace.project, user=user)
+
+    data = {
+        "files": files,
+        "metadata": {},
+        "review": {},
+    }
+
+    # Create release via API with same data
+    request = api_rf.post(
+        "/",
+        data=data,
+        format="json",
+        headers={
+            "authorization": release.backend.auth_token,
+            "os-user": user.username,
+        },
+    )
+
+    response = ReleaseWorkspaceAPI.as_view(get_github_api=FakeGitHubAPI)(
+        request, workspace_name=release.workspace.name
     )
 
+    assert response.status_code == 201
+    assert Release.objects.count() == 1
+    assert Release.objects.latest("id").id == release.id
+
+
+def test_releaseworkspaceapi_post_release_already_exists_with_airlock_id(
+    api_rf, slack_messages, project_membership
+):
+    user = UserFactory(roles=[OutputChecker], username="test-output-checker")
+    files = [
+        {
+            "name": "file.txt",
+            "url": "url",
+            "size": 7,
+            "sha256": "hash",
+            "date": timezone.now(),
+            "metadata": {},
+            "review": None,
+        }
+    ]
+    release = ReleaseFactory(
+        requested_files=files, created_by=user, id="01AAA1AAAAAAA1AAAAA11A1AAA"
+    )
     BackendMembershipFactory(backend=release.backend, user=user)
     project_membership(project=release.workspace.project, user=user)
 
     data = {
-        "files": [
-            {
-                "name": rfile.name,
-                "url": "url",
-                "size": 7,
-                "sha256": rfile.filehash,
-                "date": timezone.now(),
-                "metadata": {},
-                "review": None,
-            }
-        ],
-        "metadata": {},
+        "files": files,
+        "metadata": {"tool": "airlock", "airlock_id": "01AAA1AAAAAAA1AAAAA11A1AAA"},
         "review": {},
     }
+
+    # Create release via API with same data
     request = api_rf.post(
         "/",
         data=data,
@@ -859,9 +903,58 @@ def test_releaseworkspaceapi_post_release_already_exists(api_rf, project_members
         request, workspace_name=release.workspace.name
     )
 
-    assert response.status_code == 400
-    assert "file.txt" in response.data["detail"]
-    assert "already been uploaded" in response.data["detail"]
+    assert response.status_code == 201
+    assert Release.objects.count() == 1
+    assert Release.objects.latest("id").id == "01AAA1AAAAAAA1AAAAA11A1AAA"
+
+
+def test_releaseworkspaceapi_post_release_already_created_by_another_user(
+    api_rf, slack_messages, project_membership
+):
+    user = UserFactory(roles=[OutputChecker], username="test-output-checker")
+    files = [
+        {
+            "name": "file.txt",
+            "url": "url",
+            "size": 7,
+            "sha256": "hash",
+            "date": timezone.now(),
+            "metadata": {},
+            "review": None,
+        }
+    ]
+    release = ReleaseFactory(requested_files=files, created_by=user)
+    BackendMembershipFactory(backend=release.backend, user=user)
+    project_membership(project=release.workspace.project, user=user)
+
+    data = {
+        "files": files,
+        "metadata": {},
+        "review": {},
+    }
+
+    other_user = UserFactory(roles=[OutputChecker], username="other-output-checker")
+    BackendMembershipFactory(backend=release.backend, user=other_user)
+    # Create release via API with same data but different user
+    request = api_rf.post(
+        "/",
+        data=data,
+        format="json",
+        headers={
+            "authorization": release.backend.auth_token,
+            "os-user": other_user.username,
+        },
+    )
+
+    response = ReleaseWorkspaceAPI.as_view(get_github_api=FakeGitHubAPI)(
+        request, workspace_name=release.workspace.name
+    )
+
+    assert response.status_code == 201
+    assert Release.objects.count() == 1
+    latest_release = Release.objects.latest("id")
+    assert latest_release.id == release.id
+    assert latest_release.created_by == user
 
 
 def test_releaseworkspaceapi_post_unknown_workspace(api_rf):

tests/unit/jobserver/test_releases.py

@@ -83,7 +83,7 @@ def test_build_outputs_zip_with_missing_files(build_release_with_files):
             assert "This file was redacted by" in zipped_contents, zipped_contents
 
 
-def test_create_release_reupload():
+def test_create_release_reupload_allowed():
     workspace = WorkspaceFactory(name="workspace")
     rfile = ReleaseFileFactory(workspace=workspace, name="file1.txt", filehash="hash")
 
@@ -99,18 +99,23 @@ def test_create_release_reupload():
         }
     ]
 
-    with pytest.raises(releases.ReleaseFileAlreadyExists):
-        releases.create_release(
-            workspace,
-            rfile.release.backend,
-            rfile.release.created_by,
-            files,
-        )
+    release = releases.create_release(
+        workspace,
+        rfile.release.backend,
+        rfile.release.created_by,
+        files,
+    )
 
+    assert release.requested_files == files
+    assert release.files.count() == 1
 
-def test_create_release_reupload_to_different_workspace():
+    rfile = release.files.first()
+    rfile.filehash == "hash"
+    rfile.size == 4
+
+
+def test_create_release_already_exists():
     workspace = WorkspaceFactory(name="workspace")
-    another_workspace = WorkspaceFactory(name="another_workspace")
     rfile = ReleaseFileFactory(workspace=workspace, name="file1.txt", filehash="hash")
 
     files = [
@@ -126,11 +131,12 @@ def test_create_release_reupload_to_different_workspace():
     ]
 
     release = releases.create_release(
-        another_workspace,
+        workspace,
         rfile.release.backend,
         rfile.release.created_by,
         files,
     )
+
     assert release.requested_files == files
     assert release.files.count() == 1