From 177113466c7d42938e6c54f30a369c845344967b Mon Sep 17 00:00:00 2001 From: Joel Hernandez Date: Wed, 11 Sep 2019 12:53:31 +0200 Subject: [PATCH 01/12] :construction: first attempt at using storages --- video_encoding/files.py | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/video_encoding/files.py b/video_encoding/files.py index 5480618..a15998e 100644 --- a/video_encoding/files.py +++ b/video_encoding/files.py @@ -41,9 +41,31 @@ def _get_video_info(self): """ if not hasattr(self, '_info_cache'): encoding_backend = get_backend() - try: - path = os.path.abspath(self.path) - except AttributeError: - path = os.path.abspath(self.name) - self._info_cache = encoding_backend.get_media_info(path) + + if hasattr(self, 'file'): + # Its an actual file + try: + path = os.path.abspath(self.path) + except AttributeError: + path = os.path.abspath(self.name) + + info_cache = encoding_backend.get_media_info(path) + else: + # Its not an actual file, so assume storage abstraction + storage_path = getattr(self, 'path', self.name) + if not hasattr(self, 'storage'): + raise Exception('VideoFile uses storages yet has no self.storage') + + storage = self.storage + + try: + # If its a storage with file system implementation + storage_local_path = storage.path(storage_path) + except NotImplementedError: + storage_local_path = storage.url(storage_path) + + info_cache = encoding_backend.get_media_info(storage_local_path) + + self._info_cache = info_cache + return self._info_cache From 09d06afdfa879e3cf9e5f3634909a5321463d188 Mon Sep 17 00:00:00 2001 From: Joel Hernandez Date: Wed, 11 Sep 2019 13:23:03 +0200 Subject: [PATCH 02/12] :lipstick: fix style issues --- video_encoding/files.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/video_encoding/files.py b/video_encoding/files.py index a15998e..ed1ebec 100644 --- a/video_encoding/files.py +++ b/video_encoding/files.py @@ -54,7 +54,9 @@ def _get_video_info(self): # Its not an actual file, so assume storage abstraction storage_path = getattr(self, 'path', self.name) if not hasattr(self, 'storage'): - raise Exception('VideoFile uses storages yet has no self.storage') + raise Exception( + 'VideoFile uses storages yet has no self.storage' + ) storage = self.storage From 161fd0c0c6b7c485ca6c4df6dab26be19c700b42 Mon Sep 17 00:00:00 2001 From: Joel Hernandez Date: Wed, 11 Sep 2019 15:20:23 +0200 Subject: [PATCH 03/12] :sparkles: make package compatible with remote storages --- video_encoding/files.py | 33 ++++++++------------------------- video_encoding/tasks.py | 11 +++++++++-- video_encoding/utils.py | 20 ++++++++++++++++++++ 3 files changed, 37 insertions(+), 27 deletions(-) create mode 100644 video_encoding/utils.py diff --git a/video_encoding/files.py b/video_encoding/files.py index ed1ebec..16f3ac2 100644 --- a/video_encoding/files.py +++ b/video_encoding/files.py @@ -2,6 +2,7 @@ from django.core.files import File +from video_encoding.utils import get_fieldfile_local_path from .backends import get_backend @@ -42,31 +43,13 @@ def _get_video_info(self): if not hasattr(self, '_info_cache'): encoding_backend = get_backend() - if hasattr(self, 'file'): - # Its an actual file - try: - path = os.path.abspath(self.path) - except AttributeError: - path = os.path.abspath(self.name) - - info_cache = encoding_backend.get_media_info(path) - else: - # Its not an actual file, so assume storage abstraction - storage_path = getattr(self, 'path', self.name) - if not hasattr(self, 'storage'): - raise Exception( - 'VideoFile uses storages yet has no self.storage' - ) - - storage = self.storage - - try: - # If its a storage with file system implementation - storage_local_path = storage.path(storage_path) - except NotImplementedError: - storage_local_path = storage.url(storage_path) - - info_cache = encoding_backend.get_media_info(storage_local_path) + local_path, local_tmp_file = get_fieldfile_local_path(fieldfile=self) + + info_cache = encoding_backend.get_media_info(local_path) + + if local_tmp_file: + os.unlink(local_tmp_file.name) + local_tmp_file.close() self._info_cache = info_cache diff --git a/video_encoding/tasks.py b/video_encoding/tasks.py index 40e62a5..56528e2 100644 --- a/video_encoding/tasks.py +++ b/video_encoding/tasks.py @@ -5,6 +5,7 @@ from django.contrib.contenttypes.models import ContentType from django.core.files import File +from video_encoding.utils import get_fieldfile_local_path from .backends import get_backend from .config import settings from .exceptions import VideoEncodingError @@ -40,8 +41,10 @@ def convert_video(fieldfile, force=False): instance = fieldfile.instance field = fieldfile.field - filename = os.path.basename(fieldfile.path) - source_path = fieldfile.path + local_path, temp_file = get_fieldfile_local_path(fieldfile=fieldfile) + + filename = os.path.basename(local_path) + source_path = local_path encoding_backend = get_backend() @@ -92,3 +95,7 @@ def convert_video(fieldfile, force=False): # remove temporary file os.remove(target_path) + + if temp_file: + os.unlink(temp_file.name) + temp_file.close() diff --git a/video_encoding/utils.py b/video_encoding/utils.py new file mode 100644 index 0000000..d53fe1d --- /dev/null +++ b/video_encoding/utils.py @@ -0,0 +1,20 @@ +import tempfile + + +def get_fieldfile_local_path(fieldfile): + storage = fieldfile.storage + local_temp_file = None + + try: + # Try to access with path + storage_local_path = storage.path(fieldfile.path) + except (NotImplementedError, AttributeError): + # Storage doesnt support absolute paths, download file to a temp local dir + storage_file = storage.open(fieldfile.name, 'r') + local_temp_file = tempfile.NamedTemporaryFile(delete=False) + local_temp_file.write(storage_file.read()) + local_temp_file.seek(0) + + storage_local_path = local_temp_file.name + + return storage_local_path, local_temp_file From 18a1a83711e171a81fbd97b29317d3c9553a1a69 Mon Sep 17 00:00:00 2001 From: Joel Hernandez Date: Wed, 11 Sep 2019 15:25:13 +0200 Subject: [PATCH 04/12] :bug: handle non storage based files path --- video_encoding/utils.py | 42 +++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/video_encoding/utils.py b/video_encoding/utils.py index d53fe1d..f6c3726 100644 --- a/video_encoding/utils.py +++ b/video_encoding/utils.py @@ -1,20 +1,38 @@ +import os import tempfile def get_fieldfile_local_path(fieldfile): - storage = fieldfile.storage - local_temp_file = None + if hasattr(fieldfile, 'storage'): + storage = fieldfile.storage + local_temp_file = None - try: - # Try to access with path - storage_local_path = storage.path(fieldfile.path) - except (NotImplementedError, AttributeError): - # Storage doesnt support absolute paths, download file to a temp local dir - storage_file = storage.open(fieldfile.name, 'r') - local_temp_file = tempfile.NamedTemporaryFile(delete=False) - local_temp_file.write(storage_file.read()) - local_temp_file.seek(0) + try: + # Try to access with path + storage_local_path = storage.path( + fieldfile.path + ) + except (NotImplementedError, AttributeError): + # Storage doesnt support absolute paths, download + # file to a temp local dir + storage_file = storage.open( + fieldfile.name, 'r' + ) - storage_local_path = local_temp_file.name + local_temp_file = tempfile.NamedTemporaryFile( + delete=False + ) + local_temp_file.write(storage_file.read()) + local_temp_file.seek(0) + + storage_local_path = local_temp_file.name + else: + # Its a local file with no storage abstraction + try: + path = os.path.abspath(fieldfile.path) + except AttributeError: + path = os.path.abspath(fieldfile.name) + + storage_local_path = path return storage_local_path, local_temp_file From 98898c8beb64762582d12f8903bffa0da58572ee Mon Sep 17 00:00:00 2001 From: Joel Hernandez Date: Wed, 11 Sep 2019 15:30:31 +0200 Subject: [PATCH 05/12] :lipstick: fix linting issues --- video_encoding/files.py | 3 ++- video_encoding/utils.py | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/video_encoding/files.py b/video_encoding/files.py index 16f3ac2..e3207a3 100644 --- a/video_encoding/files.py +++ b/video_encoding/files.py @@ -43,7 +43,8 @@ def _get_video_info(self): if not hasattr(self, '_info_cache'): encoding_backend = get_backend() - local_path, local_tmp_file = get_fieldfile_local_path(fieldfile=self) + local_path, local_tmp_file = \ + get_fieldfile_local_path(fieldfile=self) info_cache = encoding_backend.get_media_info(local_path) diff --git a/video_encoding/utils.py b/video_encoding/utils.py index f6c3726..3d38db9 100644 --- a/video_encoding/utils.py +++ b/video_encoding/utils.py @@ -1,12 +1,12 @@ import os import tempfile - def get_fieldfile_local_path(fieldfile): + + local_temp_file = None + if hasattr(fieldfile, 'storage'): storage = fieldfile.storage - local_temp_file = None - try: # Try to access with path storage_local_path = storage.path( From 2f5e16b03bd76d03cc0efc9cd3038a005f5e0d2a Mon Sep 17 00:00:00 2001 From: Joel Hernandez Date: Wed, 11 Sep 2019 15:35:33 +0200 Subject: [PATCH 06/12] :lipstick: even more picky linter issues --- video_encoding/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/video_encoding/utils.py b/video_encoding/utils.py index 3d38db9..daf8918 100644 --- a/video_encoding/utils.py +++ b/video_encoding/utils.py @@ -1,8 +1,8 @@ import os import tempfile -def get_fieldfile_local_path(fieldfile): +def get_fieldfile_local_path(fieldfile): local_temp_file = None if hasattr(fieldfile, 'storage'): From 5e865f3ed514a2a51c08981dabc20d9c2d5e0047 Mon Sep 17 00:00:00 2001 From: Alexander Frenzel Date: Tue, 20 Oct 2020 17:27:16 -0700 Subject: [PATCH 07/12] refactor: linting :) --- video_encoding/files.py | 4 ++-- video_encoding/tasks.py | 1 + video_encoding/utils.py | 12 +++--------- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/video_encoding/files.py b/video_encoding/files.py index e3207a3..62538e4 100644 --- a/video_encoding/files.py +++ b/video_encoding/files.py @@ -3,6 +3,7 @@ from django.core.files import File from video_encoding.utils import get_fieldfile_local_path + from .backends import get_backend @@ -43,8 +44,7 @@ def _get_video_info(self): if not hasattr(self, '_info_cache'): encoding_backend = get_backend() - local_path, local_tmp_file = \ - get_fieldfile_local_path(fieldfile=self) + local_path, local_tmp_file = get_fieldfile_local_path(fieldfile=self) info_cache = encoding_backend.get_media_info(local_path) diff --git a/video_encoding/tasks.py b/video_encoding/tasks.py index 56528e2..b11eb47 100644 --- a/video_encoding/tasks.py +++ b/video_encoding/tasks.py @@ -6,6 +6,7 @@ from django.core.files import File from video_encoding.utils import get_fieldfile_local_path + from .backends import get_backend from .config import settings from .exceptions import VideoEncodingError diff --git a/video_encoding/utils.py b/video_encoding/utils.py index daf8918..02f68e2 100644 --- a/video_encoding/utils.py +++ b/video_encoding/utils.py @@ -9,19 +9,13 @@ def get_fieldfile_local_path(fieldfile): storage = fieldfile.storage try: # Try to access with path - storage_local_path = storage.path( - fieldfile.path - ) + storage_local_path = storage.path(fieldfile.path) except (NotImplementedError, AttributeError): # Storage doesnt support absolute paths, download # file to a temp local dir - storage_file = storage.open( - fieldfile.name, 'r' - ) + storage_file = storage.open(fieldfile.name, 'r') - local_temp_file = tempfile.NamedTemporaryFile( - delete=False - ) + local_temp_file = tempfile.NamedTemporaryFile(delete=False) local_temp_file.write(storage_file.read()) local_temp_file.seek(0) From 628220c60a8d434cce3fdc2430298e90dea87223 Mon Sep 17 00:00:00 2001 From: Alexander Frenzel Date: Sat, 7 Nov 2020 00:25:32 -0800 Subject: [PATCH 08/12] docs: update changelog and add contributor --- CHANGELOG.md | 4 ++++ CONTRIBUTORS.md | 1 + 2 files changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 867056a..56a5ab2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +* support for django-storages, @lifenautjoe & @bashu + ### Changed * remove deprecation warnings diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 407350c..3518f08 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -3,5 +3,6 @@ * [@bashu](https://github.com/bashu) * [@escaped](https://github.com/escaped/) * [@goranpavlovic](https://github.com/goranpavlovic) +* [@lifenautjoe](https://github.com/lifenautjoe) * [@mabuelhagag](https://github.com/mabuelhagag) From 65316eeedc17f739ac21c397ff74c7592f098dc0 Mon Sep 17 00:00:00 2001 From: Alexander Frenzel Date: Wed, 11 Nov 2020 22:36:08 -0800 Subject: [PATCH 09/12] test: test remote storage --- test_proj/conftest.py | 73 +++++++++++++++++++- test_proj/media_library/tests/test_fields.py | 4 +- test_proj/media_library/tests/test_files.py | 12 ++++ video_encoding/files.py | 1 - video_encoding/tasks.py | 5 +- video_encoding/utils.py | 5 +- 6 files changed, 90 insertions(+), 10 deletions(-) diff --git a/test_proj/conftest.py b/test_proj/conftest.py index 9c5aff9..0fdd955 100644 --- a/test_proj/conftest.py +++ b/test_proj/conftest.py @@ -1,12 +1,22 @@ +import enum import os +import shutil +from pathlib import Path +from typing import IO, Any, Generator import pytest from django.core.files import File +from django.core.files.storage import FileSystemStorage from test_proj.media_library.models import Video from video_encoding.backends.ffmpeg import FFmpegBackend +class StorageType(enum.Enum): + LOCAL = enum.auto() + REMOTE = enum.auto() + + @pytest.fixture def video_path(): path = os.path.dirname(os.path.abspath(__file__)) @@ -19,9 +29,70 @@ def ffmpeg(): @pytest.fixture -def video(video_path): +def local_video(video_path): video = Video() video.file.save('test.MTS', File(open(video_path, 'rb')), save=True) video.save() video.refresh_from_db() return video + + +@pytest.fixture +def remote_video(mocker, local_video): + video = Video.objects.get(pk=local_video.pk) + video_path = Path(video.file.path) + + def path(name: str) -> Path: + return video_path.parent / name + + def remote_exists(name: str) -> bool: + return path(name).exists() + + def remote_open(name: str, mode: str) -> IO[Any]: + return open(video_path, mode) + + def remote_path(*args, **kwargs): + raise NotImplementedError("Remote storage does not implement path()") + + def remote_save(name: str, content: File) -> str: + file_path = path(name) + folder_path = file_path.parent + + if not folder_path.is_dir(): + file_path.parent.mkdir(parents=True) + + if hasattr(content, 'temporary_file_path'): + shutil.move(content.temporary_file_path(), file_path) + else: + with open(file_path, 'wb') as fp: + fp.write(content.read()) + + return str(file_path) + + def remote_delete(name: str) -> None: + file_path = path(name) + file_path.unlink() + + storage = FileSystemStorage() + mocker.patch.object(storage, 'exists', remote_exists) + mocker.patch.object(storage, 'open', remote_open) + mocker.patch.object(storage, 'path', remote_path) + mocker.patch.object(storage, '_save', remote_save) + mocker.patch.object(storage, 'delete', remote_delete) + + video.file.storage = storage + yield video + + +@pytest.fixture(params=StorageType) +def video( + request, local_video: Video, remote_video: Video +) -> Generator[Video, None, None]: + storage_type = request.param + + if storage_type == StorageType.LOCAL: + yield local_video + elif storage_type == StorageType.REMOTE: + yield remote_video + else: + raise ValueError(f"Invalid storage type {storage_type}") diff --git a/test_proj/media_library/tests/test_fields.py b/test_proj/media_library/tests/test_fields.py index ff28c23..df4cff7 100644 --- a/test_proj/media_library/tests/test_fields.py +++ b/test_proj/media_library/tests/test_fields.py @@ -8,8 +8,8 @@ @pytest.mark.django_db -def test_info_forward(ffmpeg, video): - media_info = ffmpeg.get_media_info(video.file.path) +def test_info_forward(ffmpeg, video, video_path): + media_info = ffmpeg.get_media_info(video_path) assert video.duration == media_info['duration'] assert video.width == media_info['width'] diff --git a/test_proj/media_library/tests/test_files.py b/test_proj/media_library/tests/test_files.py index 8c32618..f2b2950 100644 --- a/test_proj/media_library/tests/test_files.py +++ b/test_proj/media_library/tests/test_files.py @@ -1,3 +1,5 @@ +import pytest + from video_encoding.files import VideoFile @@ -8,3 +10,13 @@ def test_videofile(ffmpeg, video_path): assert video_file.duration == media_info['duration'] assert video_file.width == media_info['width'] assert video_file.height == media_info['height'] + + +@pytest.mark.django_db +def test_videofile__with_storages(ffmpeg, video, video_path): + media_info = ffmpeg.get_media_info(video_path) + + video_file = video.file + assert video_file.duration == media_info['duration'] + assert video_file.width == media_info['width'] + assert video_file.height == media_info['height'] diff --git a/video_encoding/files.py b/video_encoding/files.py index 62538e4..7fa580f 100644 --- a/video_encoding/files.py +++ b/video_encoding/files.py @@ -50,7 +50,6 @@ def _get_video_info(self): if local_tmp_file: os.unlink(local_tmp_file.name) - local_tmp_file.close() self._info_cache = info_cache diff --git a/video_encoding/tasks.py b/video_encoding/tasks.py index b11eb47..77bf810 100644 --- a/video_encoding/tasks.py +++ b/video_encoding/tasks.py @@ -97,6 +97,5 @@ def convert_video(fieldfile, force=False): # remove temporary file os.remove(target_path) - if temp_file: - os.unlink(temp_file.name) - temp_file.close() + if temp_file: + os.unlink(temp_file.name) diff --git a/video_encoding/utils.py b/video_encoding/utils.py index 02f68e2..2004983 100644 --- a/video_encoding/utils.py +++ b/video_encoding/utils.py @@ -13,12 +13,11 @@ def get_fieldfile_local_path(fieldfile): except (NotImplementedError, AttributeError): # Storage doesnt support absolute paths, download # file to a temp local dir - storage_file = storage.open(fieldfile.name, 'r') + storage_file = storage.open(fieldfile.name, 'rb') local_temp_file = tempfile.NamedTemporaryFile(delete=False) local_temp_file.write(storage_file.read()) - local_temp_file.seek(0) - + local_temp_file.close() storage_local_path = local_temp_file.name else: # Its a local file with no storage abstraction From 3df072d495b32770b6105f73078fee1f2824a21b Mon Sep 17 00:00:00 2001 From: Alexander Frenzel Date: Wed, 11 Nov 2020 22:55:49 -0800 Subject: [PATCH 10/12] refactor: implement fake remote storage --- test_proj/conftest.py | 113 +++++++++++------- .../media_library/tests/test_managers.py | 12 +- 2 files changed, 75 insertions(+), 50 deletions(-) diff --git a/test_proj/conftest.py b/test_proj/conftest.py index 0fdd955..ca8bd08 100644 --- a/test_proj/conftest.py +++ b/test_proj/conftest.py @@ -29,33 +29,80 @@ def ffmpeg(): @pytest.fixture -def local_video(video_path): - video = Video() +def local_video(video_path) -> Generator[Video, None, None]: + """ + Return a video object which is stored locally. + """ + video = Video.objects.create() video.file.save('test.MTS', File(open(video_path, 'rb')), save=True) - video.save() - video.refresh_from_db() - return video + try: + yield video + finally: + try: + video.file.delete() + except ValueError: + # file has already been deleted + pass + + for format in video.format_set.all(): + format.file.delete() + + video.delete() @pytest.fixture -def remote_video(mocker, local_video): - video = Video.objects.get(pk=local_video.pk) - video_path = Path(video.file.path) +def remote_video(local_video) -> Generator[Video, None, None]: + """ + Return a video which is stored "remotely". + """ + storage_path = Path(local_video.file.path).parent + + remote_video = local_video + remote_video.file.storage = FakeRemoteStorage(storage_path) + yield remote_video + + +@pytest.fixture(params=StorageType) +def video( + request, local_video: Video, remote_video: Video +) -> Generator[Video, None, None]: + """ + Return a locally and a remotely stored video. + """ + storage_type = request.param + + if storage_type == StorageType.LOCAL: + yield local_video + elif storage_type == StorageType.REMOTE: + yield remote_video + else: + raise ValueError(f"Invalid storage type {storage_type}") + + +class FakeRemoteStorage(FileSystemStorage): + """ + Fake remote storage which does not support accessing a file by path. + """ - def path(name: str) -> Path: - return video_path.parent / name + def __init__(self, root_path: Path) -> None: + super().__init__() + self.root_path = root_path - def remote_exists(name: str) -> bool: - return path(name).exists() + def delete(self, name: str) -> None: + file_path = self.__path(name) + file_path.unlink() + + def exists(self, name: str) -> bool: + return self.__path(name).exists() - def remote_open(name: str, mode: str) -> IO[Any]: - return open(video_path, mode) + def open(self, name: str, mode: str) -> IO[Any]: + return open(self.__path(name), mode) - def remote_path(*args, **kwargs): + def path(self, *args, **kwargs): raise NotImplementedError("Remote storage does not implement path()") - def remote_save(name: str, content: File) -> str: - file_path = path(name) + def _save(self, name: str, content: File) -> str: + file_path = self.__path(name) folder_path = file_path.parent if not folder_path.is_dir(): @@ -69,30 +116,8 @@ def remote_save(name: str, content: File) -> str: return str(file_path) - def remote_delete(name: str) -> None: - file_path = path(name) - file_path.unlink() - - storage = FileSystemStorage() - mocker.patch.object(storage, 'exists', remote_exists) - mocker.patch.object(storage, 'open', remote_open) - mocker.patch.object(storage, 'path', remote_path) - mocker.patch.object(storage, '_save', remote_save) - mocker.patch.object(storage, 'delete', remote_delete) - - video.file.storage = storage - yield video - - -@pytest.fixture(params=StorageType) -def video( - request, local_video: Video, remote_video: Video -) -> Generator[Video, None, None]: - storage_type = request.param - - if storage_type == StorageType.LOCAL: - yield local_video - elif storage_type == StorageType.REMOTE: - yield remote_video - else: - raise ValueError(f"Invalid storage type {storage_type}") + def __path(self, name: str) -> Path: + """ + Return path to local file. + """ + return self.root_path / name diff --git a/test_proj/media_library/tests/test_managers.py b/test_proj/media_library/tests/test_managers.py index dad87e4..4a64a60 100644 --- a/test_proj/media_library/tests/test_managers.py +++ b/test_proj/media_library/tests/test_managers.py @@ -5,10 +5,10 @@ @pytest.fixture -def video_format(video): +def video_format(local_video): return Format.objects.create( - object_id=video.pk, - content_type=ContentType.objects.get_for_model(video), + object_id=local_video.pk, + content_type=ContentType.objects.get_for_model(local_video), field_name='file', format='mp4_hd', progress=100, @@ -16,9 +16,9 @@ def video_format(video): @pytest.mark.django_db -def test_related_manager(video): - assert hasattr(video.format_set, 'complete') - assert hasattr(video.format_set, 'in_progress') +def test_related_manager(local_video): + assert hasattr(local_video.format_set, 'complete') + assert hasattr(local_video.format_set, 'in_progress') @pytest.mark.django_db From 346c2d8e1eb343eedaabf86015c1747728a478ad Mon Sep 17 00:00:00 2001 From: Alexander Frenzel Date: Thu, 12 Nov 2020 00:02:05 -0800 Subject: [PATCH 11/12] refactor: auto-remove temporary files --- video_encoding/files.py | 13 ++---- video_encoding/tasks.py | 92 +++++++++++++++++++++-------------------- video_encoding/utils.py | 41 +++++++++--------- 3 files changed, 71 insertions(+), 75 deletions(-) diff --git a/video_encoding/files.py b/video_encoding/files.py index 7fa580f..4dd54f6 100644 --- a/video_encoding/files.py +++ b/video_encoding/files.py @@ -1,10 +1,7 @@ -import os - from django.core.files import File -from video_encoding.utils import get_fieldfile_local_path - from .backends import get_backend +from .utils import get_local_path class VideoFile(File): @@ -44,12 +41,8 @@ def _get_video_info(self): if not hasattr(self, '_info_cache'): encoding_backend = get_backend() - local_path, local_tmp_file = get_fieldfile_local_path(fieldfile=self) - - info_cache = encoding_backend.get_media_info(local_path) - - if local_tmp_file: - os.unlink(local_tmp_file.name) + with get_local_path(self) as local_path: + info_cache = encoding_backend.get_media_info(local_path) self._info_cache = info_cache diff --git a/video_encoding/tasks.py b/video_encoding/tasks.py index 77bf810..c74a4f8 100644 --- a/video_encoding/tasks.py +++ b/video_encoding/tasks.py @@ -5,13 +5,13 @@ from django.contrib.contenttypes.models import ContentType from django.core.files import File -from video_encoding.utils import get_fieldfile_local_path - from .backends import get_backend +from .backends.base import BaseEncodingBackend from .config import settings from .exceptions import VideoEncodingError from .fields import VideoField from .models import Format +from .utils import get_local_path def convert_all_videos(app_label, model_name, object_pk): @@ -42,60 +42,64 @@ def convert_video(fieldfile, force=False): instance = fieldfile.instance field = fieldfile.field - local_path, temp_file = get_fieldfile_local_path(fieldfile=fieldfile) + with get_local_path(fieldfile) as source_path: - filename = os.path.basename(local_path) - source_path = local_path + encoding_backend = get_backend() - encoding_backend = get_backend() + for options in settings.VIDEO_ENCODING_FORMATS[encoding_backend.name]: + video_format, created = Format.objects.get_or_create( + object_id=instance.pk, + content_type=ContentType.objects.get_for_model(instance), + field_name=field.name, + format=options['name'], + ) - for options in settings.VIDEO_ENCODING_FORMATS[encoding_backend.name]: - video_format, created = Format.objects.get_or_create( - object_id=instance.pk, - content_type=ContentType.objects.get_for_model(instance), - field_name=field.name, - format=options['name'], - ) + # do not reencode if not requested + if video_format.file and not force: + continue - # do not reencode if not requested - if video_format.file and not force: - continue - else: - # set progress to 0 - video_format.reset_progress() + try: + _encode(source_path, video_format, encoding_backend, options) + except VideoEncodingError: + # TODO handle with more care + video_format.delete() + continue - # TODO do not upscale videos - _, target_path = tempfile.mkstemp( - suffix='_{name}.{extension}'.format(**options) - ) +def _encode( + source_path: str, + video_format: Format, + encoding_backend: BaseEncodingBackend, + options: dict, +) -> None: + """ + Encode video and continously report encoding progress. + """ + # TODO do not upscale videos + # TODO move logic git VideoFormat class - try: - encoding = encoding_backend.encode( - source_path, target_path, options['params'] - ) - while encoding: - try: - progress = next(encoding) - except StopIteration: - break - video_format.update_progress(progress) - except VideoEncodingError: - # TODO handle with more care - video_format.delete() - os.remove(target_path) - continue + with tempfile.NamedTemporaryFile( + suffix='_{name}.{extension}'.format(**options) + ) as file_handler: + target_path = file_handler.name + + # set progress to 0 + video_format.reset_progress() + + encoding = encoding_backend.encode(source_path, target_path, options['params']) + while encoding: + try: + progress = next(encoding) + except StopIteration: + break + video_format.update_progress(progress) # save encoded file + filename = os.path.basename(source_path) + # TODO remove existing file? video_format.file.save( '{filename}_{name}.{extension}'.format(filename=filename, **options), File(open(target_path, mode='rb')), ) video_format.update_progress(100) # now we are ready - - # remove temporary file - os.remove(target_path) - - if temp_file: - os.unlink(temp_file.name) diff --git a/video_encoding/utils.py b/video_encoding/utils.py index 2004983..18e890c 100644 --- a/video_encoding/utils.py +++ b/video_encoding/utils.py @@ -1,31 +1,30 @@ +import contextlib import os import tempfile +from typing import Generator +from django.core.files import File -def get_fieldfile_local_path(fieldfile): - local_temp_file = None - if hasattr(fieldfile, 'storage'): +@contextlib.contextmanager +def get_local_path(fieldfile: File) -> Generator[str, None, None]: + if not hasattr(fieldfile, 'storage'): + # Its a local file with no storage abstraction + try: + yield os.path.abspath(fieldfile.path) + except AttributeError: + yield os.path.abspath(fieldfile.name) + else: storage = fieldfile.storage try: # Try to access with path - storage_local_path = storage.path(fieldfile.path) + yield storage.path(fieldfile.path) except (NotImplementedError, AttributeError): - # Storage doesnt support absolute paths, download - # file to a temp local dir - storage_file = storage.open(fieldfile.name, 'rb') - - local_temp_file = tempfile.NamedTemporaryFile(delete=False) - local_temp_file.write(storage_file.read()) - local_temp_file.close() - storage_local_path = local_temp_file.name - else: - # Its a local file with no storage abstraction - try: - path = os.path.abspath(fieldfile.path) - except AttributeError: - path = os.path.abspath(fieldfile.name) - - storage_local_path = path + # Storage doesnt support absolute paths, + # download file to a temp local dir + with tempfile.NamedTemporaryFile(mode="wb", delete=False) as temp_file: + storage_file = storage.open(fieldfile.name, 'rb') - return storage_local_path, local_temp_file + temp_file.write(storage_file.read()) + temp_file.flush() + yield temp_file.name From 1af89ba3a516e26300f34c36fae10b3f2f904591 Mon Sep 17 00:00:00 2001 From: Alexander Frenzel Date: Thu, 12 Nov 2020 23:17:40 -0800 Subject: [PATCH 12/12] docs: add some more documentation --- video_encoding/tasks.py | 2 +- video_encoding/utils.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/video_encoding/tasks.py b/video_encoding/tasks.py index c74a4f8..2e1709c 100644 --- a/video_encoding/tasks.py +++ b/video_encoding/tasks.py @@ -76,7 +76,7 @@ def _encode( Encode video and continously report encoding progress. """ # TODO do not upscale videos - # TODO move logic git VideoFormat class + # TODO move logic to Format model with tempfile.NamedTemporaryFile( suffix='_{name}.{extension}'.format(**options) diff --git a/video_encoding/utils.py b/video_encoding/utils.py index 18e890c..62c59cf 100644 --- a/video_encoding/utils.py +++ b/video_encoding/utils.py @@ -8,6 +8,9 @@ @contextlib.contextmanager def get_local_path(fieldfile: File) -> Generator[str, None, None]: + """ + Get a local file to work with from a file retrieved from a FileField. + """ if not hasattr(fieldfile, 'storage'): # Its a local file with no storage abstraction try: