diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 449fc572f4..13435d5d47 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -91,6 +91,7 @@ class File(Document): def before_insert(self): self.set_folder_name() + self.set_is_private() self.set_file_name() self.validate_attachment_limit() self.set_file_type() @@ -106,12 +107,11 @@ class File(Document): self.flags.new_file = True frappe.db.after_rollback.add(self.on_rollback) + self.validate_duplicate_entry() # Hash is generated in save_file + def after_insert(self): if not self.is_folder: self.create_attachment_record() - self.set_is_private() - self.set_file_name() - self.validate_duplicate_entry() def validate(self): if self.is_folder: @@ -470,7 +470,12 @@ class File(Document): """If file not attached to any other record, delete it""" on_disk_file_not_shared = self.content_hash and not frappe.get_all( "File", - filters={"content_hash": self.content_hash, "name": ["!=", self.name]}, + filters={ + "content_hash": self.content_hash, + "name": ["!=", self.name], + # NOTE: Some old Files might share file_urls while not sharing the is_private value + # "is_private": self.is_private, + }, limit=1, ) if on_disk_file_not_shared: diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 03a8927dac..57159fa48d 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -920,3 +920,41 @@ class TestGuestFileAndAttachments(FrappeTestCase): ).insert(ignore_permissions=True) self.assertFalse(file.is_downloadable()) + + def test_private_remains_private_even_if_same_hash(self): + file_name = "test" + frappe.generate_hash() + content = file_name.encode() + + doc_pub: "File" = frappe.new_doc("File") # type: ignore + doc_pub.file_url = f"/files/{file_name}.txt" + doc_pub.content = content + doc_pub.save() + + doc_pri: "File" = frappe.new_doc("File") # type: ignore + doc_pri.file_url = f"/private/files/{file_name}.txt" + doc_pri.is_private = False + doc_pri.content = content + doc_pri.save() + + doc_pub.reload() + doc_pri.reload() + + self.assertEqual(doc_pub.is_private, 0) + self.assertEqual(doc_pri.is_private, 1) + + self.assertEqual(doc_pub.file_url, f"/files/{file_name}.txt") + self.assertEqual(doc_pri.file_url, f"/private/files/{file_name}.txt") + + self.assertEqual(doc_pub.get_content(), content) + self.assertEqual(doc_pri.get_content(), content) + + # Deleting a public File should not delete the private File's disk file + doc_pub.delete() + self.assertTrue(os.path.exists(doc_pri.get_full_path())) + + # TODO: Migrate existing Files that have a mismatch between `is_private` and `file_url` prefix? + # self.assertFalse(os.path.exists(doc_pub.get_full_path())) + + self.assertEqual(doc_pri.get_content(), content) + doc_pri.delete() + self.assertFalse(os.path.exists(doc_pri.get_full_path()))