From ead92dc9c8c0549a880c870ab081f4cbd3c672dd Mon Sep 17 00:00:00 2001 From: AB Zainuddin Date: Fri, 24 Feb 2023 11:29:13 +0100 Subject: [PATCH 1/6] Refactor direct file access --- binder/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binder/views.py b/binder/views.py index 429cbd1c..ec6b2a53 100644 --- a/binder/views.py +++ b/binder/views.py @@ -2523,10 +2523,10 @@ def dispatch_file_field(self, request, pk=None, file_field=None): if not file_field: raise BinderNotFound(file_field_name) - guess = mimetypes.guess_type(file_field.path) + guess = mimetypes.guess_type(file_field.name) guess = guess[0] if guess and guess[0] else 'application/octet-stream' try: - resp = StreamingHttpResponse(open(file_field.path, 'rb'), content_type=guess) + resp = StreamingHttpResponse(file_field.open(), content_type=guess) except FileNotFoundError: logger.error('Expected file {} not found'.format(file_field.path)) raise BinderNotFound(file_field_name) From 514f1f7195d2410798b62b60b46fb25799d776fc Mon Sep 17 00:00:00 2001 From: AB Zainuddin Date: Fri, 24 Feb 2023 12:32:17 +0100 Subject: [PATCH 2/6] Found another one --- binder/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/models.py b/binder/models.py index ab1e05e1..3f50e3b7 100644 --- a/binder/models.py +++ b/binder/models.py @@ -682,7 +682,7 @@ def content_type(self): if not self.name: self._content_type = None elif self._content_type is None: - self._content_type, _ = mimetypes.guess_type(self.path) + self._content_type, _ = mimetypes.guess_type(self.name) return self._content_type # So here we have a bunch of methods that might alter the data or name of From 8f7477d59b21cfad4b085c277f936cd66b5fe88c Mon Sep 17 00:00:00 2001 From: AB Zainuddin Date: Mon, 27 Feb 2023 12:28:12 +0100 Subject: [PATCH 3/6] Found another one --- binder/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binder/views.py b/binder/views.py index ec6b2a53..25890ef1 100644 --- a/binder/views.py +++ b/binder/views.py @@ -2528,7 +2528,7 @@ def dispatch_file_field(self, request, pk=None, file_field=None): try: resp = StreamingHttpResponse(file_field.open(), content_type=guess) except FileNotFoundError: - logger.error('Expected file {} not found'.format(file_field.path)) + logger.error('Expected file {} not found'.format(file_field.name)) raise BinderNotFound(file_field_name) if 'download' in request.GET: @@ -2580,7 +2580,7 @@ def filefield_get_name(self, instance=None, request=None, file_field=None): try: method = getattr(self, 'filefield_get_name_' + file_field.field.name) except AttributeError: - return os.path.basename(file_field.path) + return os.path.basename(file_field.name) return method(instance=instance, request=request, file_field=file_field) From f0d61becd03af7271d590b907813cf6469a48723 Mon Sep 17 00:00:00 2001 From: Gergely Date: Wed, 21 Aug 2024 11:11:31 +0200 Subject: [PATCH 4/6] Add support for direct serving --- binder/models.py | 8 +++++--- binder/views.py | 21 +++++++++++++++------ tests/__init__.py | 5 +++-- tests/test_binder_file_field.py | 27 +++++++++++++++++++++++---- tests/testapp/models/zoo.py | 2 ++ tests/testapp/views/zoo.py | 3 ++- 6 files changed, 50 insertions(+), 16 deletions(-) diff --git a/binder/models.py b/binder/models.py index 3f50e3b7..c45e41ae 100644 --- a/binder/models.py +++ b/binder/models.py @@ -790,12 +790,13 @@ class BinderFileField(FileField): attr_class = BinderFieldFile descriptor_class = BinderFileDescriptor - def __init__(self, allowed_extensions=None, *args, **kwargs): + def __init__(self, allowed_extensions=None, serve_directly=False, *args, **kwargs): # Since we also need to store a content type and a hash in the field # we up the default max_length from 100 to 200. Now we store also # the original file name, so lets make it 400 chars. kwargs.setdefault('max_length', 400) self.allowed_extensions = allowed_extensions + self.serve_directly = serve_directly return super().__init__(*args, **kwargs) def get_prep_value(self, value): @@ -825,6 +826,7 @@ def deconstruct(self): if self.allowed_extensions: kwargs['allowed_extensions'] = self.allowed_extensions + kwargs['serve_directly'] = self.serve_directly return name, path, args, kwargs @@ -868,11 +870,11 @@ class BinderImageField(BinderFileField): descriptor_class = BinderImageFileDescriptor description = _("Image") - def __init__(self, verbose_name=None, name=None, width_field=None, height_field=None, allowed_extensions=None, **kwargs): + def __init__(self, verbose_name=None, name=None, width_field=None, height_field=None, allowed_extensions=None, serve_directly=False, **kwargs): self.width_field, self.height_field = width_field, height_field if allowed_extensions is None: allowed_extensions = ['png', 'gif', 'jpg', 'jpeg'] - super().__init__(allowed_extensions, verbose_name, name, **kwargs) + super().__init__(allowed_extensions, serve_directly, verbose_name, name, **kwargs) def check(self, **kwargs): return [ diff --git a/binder/views.py b/binder/views.py index 25890ef1..4cd681fb 100644 --- a/binder/views.py +++ b/binder/views.py @@ -14,6 +14,7 @@ import django from django.views.generic import View +from django.conf import settings from django.core.exceptions import ObjectDoesNotExist, FieldError, ValidationError, FieldDoesNotExist from django.core.files.base import File, ContentFile from django.http import HttpResponse, StreamingHttpResponse, HttpResponseForbidden @@ -442,7 +443,7 @@ def dispatch(self, request, *args, **kwargs): # Check if the TRANSACTION_DATABASES is set in the settings.py, and if so, use that instead try: - transaction_dbs = django.conf.settings.TRANSACTION_DATABASES + transaction_dbs = settings.TRANSACTION_DATABASES except AttributeError: pass @@ -1378,7 +1379,7 @@ def get(self, request, pk=None, withs=None, include_annotations=None): meta['comment'] = self.comment debug = {'request_id': request.request_id} - if django.conf.settings.DEBUG and 'debug' in request.GET: + if settings.DEBUG and 'debug' in request.GET: debug['queries'] = ['{}s: {}'.format(q['time'], q['sql'].replace('"', '')) for q in django.db.connection.queries] debug['query_count'] = len(django.db.connection.queries) @@ -2518,15 +2519,23 @@ def dispatch_file_field(self, request, pk=None, file_field=None): file_field_name = file_field file_field = getattr(obj, file_field_name) + field = self.model._meta.get_field(file_field_name) if request.method == 'GET': if not file_field: raise BinderNotFound(file_field_name) guess = mimetypes.guess_type(file_field.name) - guess = guess[0] if guess and guess[0] else 'application/octet-stream' + content_type = (guess and guess[0]) or 'application/octet-stream' + serve_directly = isinstance(field, BinderFileField) and field.serve_directly try: - resp = StreamingHttpResponse(file_field.open(), content_type=guess) + if serve_directly: + resp = HttpResponse(content_type=content_type) + resp[settings.INTERNAL_MEDIA_HEADER] = os.path.join(settings.INTERNAL_MEDIA_LOCATION, file_field.name) + if not file_field.url.startswith('/'): + resp['redirect_url'] = file_field.url + else: + resp = StreamingHttpResponse(file_field.open(), content_type=content_type) except FileNotFoundError: logger.error('Expected file {} not found'.format(file_field.name)) raise BinderNotFound(file_field_name) @@ -2591,7 +2600,7 @@ def view_history(self, request, pk=None, **kwargs): debug = kwargs['history'] == 'debug' - if debug and not django.conf.settings.ENABLE_DEBUG_ENDPOINTS: + if debug and not settings.ENABLE_DEBUG_ENDPOINTS: logger.warning('Debug endpoints disabled.') return HttpResponseForbidden('Debug endpoints disabled.') @@ -2621,7 +2630,7 @@ def debug_changesets_24h(request): logger.warning('Not authenticated.') return HttpResponseForbidden('Not authenticated.') - if not django.conf.settings.ENABLE_DEBUG_ENDPOINTS: + if not settings.ENABLE_DEBUG_ENDPOINTS: logger.warning('Debug endpoints disabled.') return HttpResponseForbidden('Debug endpoints disabled.') diff --git a/tests/__init__.py b/tests/__init__.py index 4dcaf8bd..0d8e9148 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -108,7 +108,9 @@ }, 'GROUP_CONTAINS': { 'admin': [] - } + }, + 'INTERNAL_MEDIA_HEADER': 'X-Accel-Redirect', + 'INTERNAL_MEDIA_LOCATION': '/internal/media/', }) setup() @@ -131,4 +133,3 @@ content_type = ContentType.objects.get_or_create(app_label='testapp', model='country')[0] Permission.objects.get_or_create(content_type=content_type, codename='view_country') call_command('define_groups') - diff --git a/tests/test_binder_file_field.py b/tests/test_binder_file_field.py index 9fc901eb..f6e8a0d7 100644 --- a/tests/test_binder_file_field.py +++ b/tests/test_binder_file_field.py @@ -142,10 +142,10 @@ def test_get(self): zoo.refresh_from_db() filename = basename(zoo.binder_picture.name) # Without folders foo/bar/ - self.assertEqual( - data['data']['binder_picture'], - '/zoo/{}/binder_picture/?h={}&content_type=image/jpeg&filename={}'.format(zoo.pk, JPG_HASH, filename), - ) + path = '/zoo/{}/binder_picture/?h={}&content_type=image/jpeg&filename={}'.format(zoo.pk, JPG_HASH, filename) + self.assertEqual(data['data']['binder_picture'], path) + response = self.client.get(path) + self.assertNotIn('X-Accel-Redirect', response.headers) def test_get_unknown_extension(self): filename = 'pic.unknown' @@ -166,6 +166,25 @@ def test_get_unknown_extension(self): '/zoo/{}/binder_picture/?h={}&content_type=&filename={}'.format(zoo.pk, UNKNOWN_TYPE_HASH, filename), ) + def test_get_direct(self): + filename = 'pic.jpg' + zoo = Zoo(name='Apenheul') + zoo.binder_picture_direct = ContentFile(JPG_CONTENT, name=filename) + zoo.save() + + response = self.client.get('/zoo/{}/'.format(zoo.pk)) + self.assertEqual(response.status_code, 200) + data = jsonloads(response.content) + + # Remove once Django 3 lands with: https://docs.djangoproject.com/en/3.1/howto/custom-file-storage/#django.core.files.storage.get_alternative_name + zoo.refresh_from_db() + filename = basename(zoo.binder_picture_direct.name) # Without folders foo/bar/ + + path = '/zoo/{}/binder_picture_direct/?h={}&content_type=image/jpeg&filename={}'.format(zoo.pk, JPG_HASH, filename) + self.assertEqual(data['data']['binder_picture_direct'], path) + response = self.client.get(path) + self.assertIn('X-Accel-Redirect', response.headers) + def test_setting_blank(self): zoo = Zoo(name='Apenheul') zoo.binder_picture = '' diff --git a/tests/testapp/models/zoo.py b/tests/testapp/models/zoo.py index a6b1737c..859f7099 100644 --- a/tests/testapp/models/zoo.py +++ b/tests/testapp/models/zoo.py @@ -35,6 +35,8 @@ class Zoo(BinderModel): binder_picture_custom_extensions = BinderImageField(allowed_extensions=['png'], blank=True, null=True) + binder_picture_direct = BinderImageField(serve_directly=True, blank=True, null=True) + def __str__(self): return 'zoo %d: %s' % (self.pk, self.name) diff --git a/tests/testapp/views/zoo.py b/tests/testapp/views/zoo.py index 813b5bde..a22f2d6c 100644 --- a/tests/testapp/views/zoo.py +++ b/tests/testapp/views/zoo.py @@ -11,12 +11,13 @@ class ZooView(PermissionView): m2m_fields = ['contacts', 'zoo_employees', 'most_popular_animals'] model = Zoo file_fields = ['floor_plan', 'django_picture', 'binder_picture', 'django_picture_not_null', - 'binder_picture_not_null', 'binder_picture_custom_extensions'] + 'binder_picture_not_null', 'binder_picture_custom_extensions', 'binder_picture_direct'] shown_properties = ['animal_count'] image_resize_threshold = { 'floor_plan': 500, 'binder_picture': 500, 'binder_picture_custom_extensions': 500, + 'binder_picture_direct': 500, } image_format_override = { 'floor_plan': 'jpeg', From e12a8a7cc47428b11e8294f276683103671a04a4 Mon Sep 17 00:00:00 2001 From: Marcin Knap <33230423+knmarcin@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:44:01 +0100 Subject: [PATCH 5/6] Close RabbitMQ connection after publishing to prevent file descriptor leaks --- binder/websocket.py | 32 ++++++++++++++++++-------------- setup.py | 1 + tests/test_websocket.py | 25 +++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/binder/websocket.py b/binder/websocket.py index 10c9c108..b1e4ca12 100644 --- a/binder/websocket.py +++ b/binder/websocket.py @@ -32,20 +32,24 @@ def list_rooms_for_user(self, user): def trigger(data, rooms): if 'rabbitmq' in getattr(settings, 'HIGH_TEMPLAR', {}): - import pika - from pika import BlockingConnection - - connection_credentials = pika.PlainCredentials(settings.HIGH_TEMPLAR['rabbitmq']['username'], - settings.HIGH_TEMPLAR['rabbitmq']['password']) - connection_parameters = pika.ConnectionParameters(settings.HIGH_TEMPLAR['rabbitmq']['host'], - credentials=connection_credentials) - connection = BlockingConnection(parameters=connection_parameters) - channel = connection.channel() - - channel.basic_publish('hightemplar', routing_key='*', body=jsondumps({ - 'data': data, - 'rooms': rooms, - })) + connection = None + try: + import pika + from pika import BlockingConnection + connection_credentials = pika.PlainCredentials(settings.HIGH_TEMPLAR['rabbitmq']['username'], + settings.HIGH_TEMPLAR['rabbitmq']['password']) + connection_parameters = pika.ConnectionParameters(settings.HIGH_TEMPLAR['rabbitmq']['host'], + credentials=connection_credentials) + connection = BlockingConnection(parameters=connection_parameters) + channel = connection.channel() + + channel.basic_publish('hightemplar', routing_key='*', body=jsondumps({ + 'data': data, + 'rooms': rooms, + })) + finally: + if connection and not connection.is_closed: + connection.close() if getattr(settings, 'HIGH_TEMPLAR_URL', None): url = getattr(settings, 'HIGH_TEMPLAR_URL') try: diff --git a/setup.py b/setup.py index 55196440..8a32da4d 100755 --- a/setup.py +++ b/setup.py @@ -41,6 +41,7 @@ 'Pillow >= 3.2.0', 'django-request-id >= 1.0.0', 'requests >= 2.13.0', + 'pika == 1.3.2', ], tests_require=[ 'django-hijack >= 2.1.10, < 3.0.0', diff --git a/tests/test_websocket.py b/tests/test_websocket.py index e3ddc627..1bbf8464 100644 --- a/tests/test_websocket.py +++ b/tests/test_websocket.py @@ -2,6 +2,7 @@ from django.contrib.auth.models import User from unittest import mock from binder.views import JsonResponse +from binder.websocket import trigger from .testapp.urls import room_controller from .testapp.models import Animal, Costume import requests @@ -68,3 +69,27 @@ def test_post_succeeds_when_trigger_fails(self): costume.save() self.assertIsNotNone(costume.pk) + + +class TriggerConnectionCloseTest(TestCase): + @override_settings( + HIGH_TEMPLAR={ + 'rabbitmq': { + 'host': 'localhost', + 'username': 'guest', + 'password': 'guest' + } + } + ) + @mock.patch('pika.BlockingConnection') + def test_trigger_calls_connection_close(self, mock_connection_class): + mock_connection = mock_connection_class.return_value + mock_connection.is_closed = False + + data = {'id': 123} + rooms = [{'costume': 123}] + + trigger(data, rooms) + + mock_connection.close.assert_called_once() + From 0b8c3c9cbb7e78ebb1b5d84f79ff682734790286 Mon Sep 17 00:00:00 2001 From: Bob Booij-Liewes Date: Sun, 12 Jan 2025 21:52:26 +0100 Subject: [PATCH 6/6] Fix linting --- binder/models.py | 2 +- binder/views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/binder/models.py b/binder/models.py index c45e41ae..de1cd5af 100644 --- a/binder/models.py +++ b/binder/models.py @@ -943,7 +943,7 @@ def update_dimension_fields(self, instance, force=False, *args, **kwargs): if not file and not force: return - dimension_fields_filled = not( + dimension_fields_filled = not ( (self.width_field and not getattr(instance, self.width_field)) or (self.height_field and not getattr(instance, self.height_field)) ) diff --git a/binder/views.py b/binder/views.py index 4cd681fb..9ec9f8df 100644 --- a/binder/views.py +++ b/binder/views.py @@ -616,7 +616,7 @@ def _annotate_objs(self, datas_by_id, objs_by_id): for obj_id, data in datas_by_id.items(): # TODO: Don't require OneToOneFields in the m2m_fields list if isinstance(local_field, models.OneToOneRel): - assert(len(idmap[obj_id]) <= 1) + assert (len(idmap[obj_id]) <= 1) data[field_name] = idmap[obj_id][0] if len(idmap[obj_id]) == 1 else None else: data[field_name] = idmap[obj_id] @@ -1476,7 +1476,7 @@ def store_m2m_field(obj, field, value, request): try: obj.save() - assert(obj.pk is not None) # At this point, the object must have been created. + assert (obj.pk is not None) # At this point, the object must have been created. except ValidationError as ve: validation_errors.append(self.binder_validation_error(obj, ve, pk=pk))