From 1c70ee67716a34c62082997fccbddfd076402137 Mon Sep 17 00:00:00 2001 From: Alex Pyrgiotis Date: Tue, 30 Apr 2024 13:03:52 +0300 Subject: [PATCH] Fix archiving the same doc twice on Windows On Windows, if we somehow attempt to archive the same document twice (e.g, because it got archived once, and then we copy it back), we will get an error, because Windows does not overwrite the target path, if it already exists. Fix this issue by always removing the previously archived version, when performing the next archival action, and update our tests. --- dangerzone/document.py | 2 ++ tests/test_document.py | 28 ++++++++++++++++------------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/dangerzone/document.py b/dangerzone/document.py index 4aa2eeb..66d2eac 100644 --- a/dangerzone/document.py +++ b/dangerzone/document.py @@ -145,6 +145,8 @@ class Document: new_file_path = archive_dir / old_file_path.name log.debug(f"Archiving doc {self.id} to {new_file_path}") Path.mkdir(archive_dir, exist_ok=True) + # On Windows, moving the file will fail if it already exists. + new_file_path.unlink(missing_ok=True) old_file_path.rename(new_file_path) @property diff --git a/tests/test_document.py b/tests/test_document.py index 23a9061..c999f42 100644 --- a/tests/test_document.py +++ b/tests/test_document.py @@ -97,24 +97,28 @@ def test_archive_unwriteable_dir(tmp_path: Path) -> None: def test_archive(mocker: MagicMock, tmp_path: Path) -> None: - test_string = "original file" original_doc_path = str(tmp_path / "doc.pdf") archived_doc_path = str(tmp_path / ARCHIVE_SUBDIR / "doc.pdf") - # write some content for later verifying content integrity - with open(original_doc_path, "w") as f: - f.write(test_string) + # Perform the archival operation two times: one with no archive dir, and one with an + # archive dir. + test_strings = ["original file 1", "original file 2"] + for test_string in test_strings: + # write some content for later verifying content integrity + with open(original_doc_path, "w") as f: + f.write(test_string) - d = Document(original_doc_path, archive=True) - d.archive() + # archive the document + d = Document(original_doc_path, archive=True) + d.archive() - # original document has been moved to unsafe/doc.pdf - assert not os.path.exists(original_doc_path) - assert os.path.exists(archived_doc_path) + # original document has been moved to unsafe/doc.pdf + assert not os.path.exists(original_doc_path) + assert os.path.exists(archived_doc_path) - # make sure it is the original file by comparing its content - with open(archived_doc_path) as f: - assert f.read() == test_string + # make sure it is the proper file by comparing its content + with open(archived_doc_path) as f: + assert f.read() == test_string def test_set_output_dir(sample_pdf: str, tmp_path: Path) -> None: