From 069e51d54bf746ddc4256c1b49268f9660f0127a Mon Sep 17 00:00:00 2001 From: Ismael Date: Fri, 10 Jan 2025 14:00:12 -0500 Subject: [PATCH 1/2] More delete file handling --- CHANGELOG.rst | 5 ++- guillotina/contrib/redis/dm.py | 4 +-- guillotina/files/adapter.py | 55 ++++++++++++++++------------- guillotina/files/manager.py | 8 +++-- guillotina/test_package.py | 8 +++-- guillotina/tests/test_attachment.py | 16 +++++++++ 6 files changed, 65 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b1ae22e86..589515901 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,8 +1,11 @@ CHANGELOG ========= -5.5.4 (2025-01-09) +5.5.5 (2025-01-10) ------------------- +- More delete file handling improvements +5.5.4 (2025-01-09) +------------------- - Add delete file endpoint 5.5.3 (2024-10-09) diff --git a/guillotina/contrib/redis/dm.py b/guillotina/contrib/redis/dm.py index 5682efaae..99be6933d 100644 --- a/guillotina/contrib/redis/dm.py +++ b/guillotina/contrib/redis/dm.py @@ -85,8 +85,8 @@ def get_key(self): async def update(self, **kwargs): self._data.update(kwargs) - async def finish(self, values=None): - val = await super().finish(values=values) + async def finish(self, values=None, clear_data=False): + val = await super().finish(values=values, clear_data=clear_data) txn = get_transaction() txn.add_after_commit_hook(self._delete_key) return val diff --git a/guillotina/files/adapter.py b/guillotina/files/adapter.py index 2b7a227e6..8cd19dd4b 100644 --- a/guillotina/files/adapter.py +++ b/guillotina/files/adapter.py @@ -89,41 +89,45 @@ async def update(self, **kwargs): async def save(self, **kwargs): pass - async def finish(self, values=None): + async def finish(self, values=None, clear_data=False): # create file object with new data from finished upload try: file = self.field.get(self.real_context) except AttributeError: file = None - if file is None: - file = self.file_storage_manager.file_class() + if clear_data: + file = None else: - # save previous data on file. - # we do this instead of creating a new file object on every - # save just in case other implementations want to use the file - # object to store different data - file._old_uri = file.uri - file._old_size = file.size - file._old_filename = file.filename - file._old_md5 = file.md5 - file._old_content_type = file.guess_content_type() - - if getattr(file, "_blob", None): - cleanup = IFileCleanup(self.context, None) - if cleanup is None or cleanup.should_clean(file=file): - bfile = file._blob.open("r") - await bfile.async_del() - else: - file._previous_blob = getattr(file, "_blob", None) + if file is None: + file = self.file_storage_manager.file_class() + else: + # save previous data on file. + # we do this instead of creating a new file object on every + # save just in case other implementations want to use the file + # object to store different data + file._old_uri = file.uri + file._old_size = file.size + file._old_filename = file.filename + file._old_md5 = file.md5 + file._old_content_type = file.guess_content_type() + + if getattr(file, "_blob", None): + cleanup = IFileCleanup(self.context, None) + if cleanup is None or cleanup.should_clean(file=file): + bfile = file._blob.open("r") + await bfile.async_del() + else: + file._previous_blob = getattr(file, "_blob", None) + + if values is None: + values = self._data + for key, value in values.items(): + setattr(file, key, value) await notify(FileBeforeUploadFinishedEvent(self.context, field=self.field, file=file, dm=self)) - if values is None: - values = self._data self.field.set(self.real_context, file) - for key, value in values.items(): - setattr(file, key, value) if self.field.__name__ in getattr(self.context, "__uploads__", {}): del self.context.__uploads__[self.field.__name__] @@ -261,8 +265,11 @@ async def delete(self): blob = file._blob bfile = blob.open("r") await bfile.async_del() + + # Set the file attribute to None self.field.set(self.field.context or self.context, None) self.field.context.register() + return True except AttributeError: pass diff --git a/guillotina/files/manager.py b/guillotina/files/manager.py index 7ef67c1e8..37d858615 100644 --- a/guillotina/files/manager.py +++ b/guillotina/files/manager.py @@ -475,7 +475,11 @@ async def delete(self): await self.dm.load() await self.dm.start() - result = await self.file_storage_manager.delete() - await self.dm.finish() + result = False + file = self.field.get(self.field.context or self.context) + if file: + result = await self.file_storage_manager.delete() + + await self.dm.finish(clear_data=True) return result diff --git a/guillotina/test_package.py b/guillotina/test_package.py index 22a88a3ae..05e322e3c 100644 --- a/guillotina/test_package.py +++ b/guillotina/test_package.py @@ -445,7 +445,7 @@ async def append(self, dm, iterable, offset) -> int: await dm.update(_chunks=chunk_count) return count - async def finish(self, dm): + async def finish(self, dm, clear_data=False): await dm.update(uri=dm.get("upload_file_id"), upload_file_id=None) async def exists(self): @@ -470,9 +470,13 @@ async def copy(self, to_storage_manager, to_dm): async def delete(self): file = self.field.get(self.field.context or self.context) - if file.uri in _tmp_files: + if file and file.uri in _tmp_files: os.remove(_tmp_files[file.uri]) del _tmp_files[file.uri] + + self.field.set(self.field.context or self.context, None) + self.field.context.register() + return True return False diff --git a/guillotina/tests/test_attachment.py b/guillotina/tests/test_attachment.py index 3bd811389..f101d8da2 100644 --- a/guillotina/tests/test_attachment.py +++ b/guillotina/tests/test_attachment.py @@ -877,6 +877,11 @@ async def test_delete_upload(manager_type, redis_container, container_requester) ) assert status == 201 + # No file to start with + response, status = await requester("GET", "/db/guillotina/foobar") + assert status == 200 + assert response["guillotina.behaviors.attachment.IAttachment"]["file"] is None + response, status = await requester( "PATCH", "/db/guillotina/foobar/@upload/file", @@ -885,6 +890,11 @@ async def test_delete_upload(manager_type, redis_container, container_requester) ) assert status == 200 + # File attribute is now not empty + response, status = await requester("GET", "/db/guillotina/foobar") + assert status == 200 + assert response["guillotina.behaviors.attachment.IAttachment"]["file"] is not None + response, status = await requester("GET", "/db/guillotina/foobar/@download/file") assert status == 200 @@ -892,6 +902,12 @@ async def test_delete_upload(manager_type, redis_container, container_requester) assert status == 200 assert response is True + # Check the file attribute is now empty + response, status = await requester("GET", "/db/guillotina/foobar") + assert status == 200 + assert response["guillotina.behaviors.attachment.IAttachment"]["file"] is None + + # False if we try to delete again response, status = await requester("PATCH", "/db/guillotina/foobar/@delete/file") assert status == 200 assert response is False From 3e9a7474ce4ed8ebbc29730a5b6ddbeee4978a97 Mon Sep 17 00:00:00 2001 From: Ismael Date: Fri, 10 Jan 2025 14:00:30 -0500 Subject: [PATCH 2/2] More delete file handling --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 0413736d6..393072c7a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -5.5.4 \ No newline at end of file +5.5.5 \ No newline at end of file