From cad1a510683266dccd740f4f020f1d6425db3c2f Mon Sep 17 00:00:00 2001 From: Zach Mullen Date: Wed, 3 Feb 2021 10:45:25 -0500 Subject: [PATCH 1/6] Make file creation and validation happen prior to blob upload --- dkc/core/migrations/0009_file_pre_creation.py | 28 +++++++++++ dkc/core/models/file.py | 8 +-- dkc/core/rest/file.py | 12 +++-- dkc/core/tests/conftest.py | 5 ++ dkc/core/tests/factories.py | 1 + dkc/core/tests/test_file_rest.py | 49 +++++++++++++++++++ dkc/core/tests/test_permissions.py | 8 +-- 7 files changed, 97 insertions(+), 14 deletions(-) create mode 100644 dkc/core/migrations/0009_file_pre_creation.py diff --git a/dkc/core/migrations/0009_file_pre_creation.py b/dkc/core/migrations/0009_file_pre_creation.py new file mode 100644 index 0000000..80f7f78 --- /dev/null +++ b/dkc/core/migrations/0009_file_pre_creation.py @@ -0,0 +1,28 @@ +# Generated by Django 3.1.6 on 2021-02-03 15:40 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('core', '0008_quota'), + ] + + operations = [ + migrations.AlterField( + model_name='file', + name='size', + field=models.PositiveBigIntegerField(), + ), + migrations.AlterField( + model_name='quota', + name='user', + field=models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL + ), + ), + ] diff --git a/dkc/core/models/file.py b/dkc/core/models/file.py index 15cf5e9..2637456 100644 --- a/dkc/core/models/file.py +++ b/dkc/core/models/file.py @@ -34,7 +34,7 @@ class Meta: ], ) description = models.TextField(max_length=3000, blank=True) - size = models.PositiveBigIntegerField(editable=False) + size = models.PositiveBigIntegerField() content_type = models.CharField(max_length=255, default='application/octet-stream') blob = S3FileField() sha512 = models.CharField(max_length=128, blank=True, default='', db_index=True, editable=False) @@ -84,12 +84,8 @@ def has_permission(self, user: User, permission: Permission) -> bool: @receiver(models.signals.pre_save, sender=File) def _file_pre_save(sender: Type[File], instance: File, **kwargs): - # TODO if we allow changing a file's blob, we also need to update the size - + # TODO if we allow changing a file's blob & size, we'll need more logic here if not instance.pk: - # TODO: Test how this works in minio - instance.size = instance.blob.size - instance.folder.increment_size(instance.size) diff --git a/dkc/core/rest/file.py b/dkc/core/rest/file.py index 25f4c3d..95516d8 100644 --- a/dkc/core/rest/file.py +++ b/dkc/core/rest/file.py @@ -1,6 +1,8 @@ +from io import BytesIO + from django.contrib.auth.models import User from django.core.exceptions import PermissionDenied -from django.http import HttpResponseRedirect +from django.http import FileResponse, HttpResponseRedirect from django.shortcuts import get_object_or_404 from rest_framework import serializers from rest_framework.decorators import action @@ -22,7 +24,6 @@ class Meta: 'description', 'size', 'content_type', - 'blob', 'sha512', 'folder', 'creator', @@ -64,7 +65,8 @@ def _validate_unique_folder_siblings(self, attrs): class FileUpdateSerializer(FileSerializer): class Meta(FileSerializer.Meta): - read_only_fields = FileSerializer.Meta.read_only_fields + ['folder'] + fields = FileSerializer.Meta.fields + ['blob'] + read_only_fields = FileSerializer.Meta.read_only_fields + ['folder', 'size'] class FileViewSet(ModelViewSet): @@ -91,4 +93,6 @@ def perform_create(self, serializer: FileSerializer): @action(detail=True) def download(self, request, pk=None): file = get_object_or_404(File, pk=pk) - return HttpResponseRedirect(file.blob.url) + if file.blob: # FileFields are falsy when not populated with a file + return HttpResponseRedirect(file.blob.url) + return FileResponse(BytesIO(), filename=file.name, content_type=file.content_type) diff --git a/dkc/core/tests/conftest.py b/dkc/core/tests/conftest.py index 13e3077..ac8e959 100644 --- a/dkc/core/tests/conftest.py +++ b/dkc/core/tests/conftest.py @@ -24,6 +24,11 @@ def child_folder(folder, folder_factory): return folder_factory(parent=folder) +@pytest.fixture +def pending_file(file_factory): + return file_factory(size=42, blob=None, content_type='text/plain') + + @pytest.fixture def public_folder(folder_factory): return folder_factory(tree__public=True) diff --git a/dkc/core/tests/factories.py b/dkc/core/tests/factories.py index 8b0af2e..7d5061a 100644 --- a/dkc/core/tests/factories.py +++ b/dkc/core/tests/factories.py @@ -54,6 +54,7 @@ class Meta: name = factory.Faker('file_name') description = factory.Faker('paragraph') blob = factory.django.FileField(data=b'fakefilebytes', filename='fake.txt') + size = factory.LazyAttribute(lambda obj: obj.blob.size) user_metadata = _metadata_faker folder = factory.SubFactory(FolderFactory) creator = factory.SubFactory(UserFactory) diff --git a/dkc/core/tests/test_file_rest.py b/dkc/core/tests/test_file_rest.py index d00fb7a..7b56df9 100644 --- a/dkc/core/tests/test_file_rest.py +++ b/dkc/core/tests/test_file_rest.py @@ -1,5 +1,7 @@ import pytest +from dkc.core.models import File + @pytest.mark.django_db def test_file_rest_retrieve(admin_api_client, file): @@ -16,3 +18,50 @@ def test_file_list_default_ordering(admin_api_client, folder, file_factory): resp = admin_api_client.get('/api/v2/files', data={'folder': folder.id}) assert resp.status_code == 200 assert [f['name'] for f in resp.data['results']] == ['A', 'B', 'C'] + + +@pytest.mark.django_db +def test_file_rest_create_process(admin_api_client, folder): + """Test initialization of a file, without its blob.""" + resp = admin_api_client.post( + '/api/v2/files', + data={ + 'folder': folder.id, + 'name': 'test.txt', + 'size': 42, + }, + ) + assert resp.status_code == 201 + assert resp.data['size'] == 42 + folder.refresh_from_db() + assert folder.size == 42 + saved_file = File.objects.get(pk=resp.data['id']) + assert bool(saved_file.blob) is False + + +@pytest.mark.django_db +def test_file_rest_download_pending_file(admin_api_client, pending_file): + """Test downloading a file prior to its blob being set does something sane.""" + resp = admin_api_client.get(f'/api/v2/files/{pending_file.id}/download') + assert resp.status_code == 200 + assert b''.join(resp.streaming_content) == b'' + assert resp['content-type'] == pending_file.content_type + assert resp['content-length'] == '0' + assert pending_file.name in resp['content-disposition'] + + +@pytest.mark.django_db +def test_file_rest_cannot_update_size(admin_api_client, file): + resp = admin_api_client.patch(f'/api/v2/files/{file.id}', data={'size': file.size + 1}) + assert resp.data['size'] == file.size + + +@pytest.mark.django_db +def test_file_rest_set_blob(admin_api_client, pending_file, s3ff_field_value): + resp = admin_api_client.patch( + f'/api/v2/files/{pending_file.id}', data={'blob': s3ff_field_value} + ) + assert resp.status_code == 200 + assert resp.data['size'] == pending_file.size + pending_file.refresh_from_db() + assert bool(pending_file.blob) is True diff --git a/dkc/core/tests/test_permissions.py b/dkc/core/tests/test_permissions.py index f02b06d..0003ff9 100644 --- a/dkc/core/tests/test_permissions.py +++ b/dkc/core/tests/test_permissions.py @@ -293,13 +293,13 @@ def test_anonymous_user_cannot_create_root_folder(api_client): @pytest.mark.django_db -def test_anonymous_user_cannot_create_files(api_client, folder, s3ff_field_value): +def test_anonymous_user_cannot_create_files(api_client, folder): resp = api_client.post( '/api/v2/files', data={ 'name': 'test.txt', 'folder': folder.id, - 'blob': s3ff_field_value, + 'size': 42, }, ) assert resp.status_code == 401 @@ -313,14 +313,14 @@ def test_folder_create_permission_enforcement(api_client, folder, user): @pytest.mark.django_db -def test_file_create_permission_enforcement(api_client, folder, user, s3ff_field_value): +def test_file_create_permission_enforcement(api_client, folder, user): api_client.force_authenticate(user=user) resp = api_client.post( '/api/v2/files', data={ 'name': 'test.txt', 'folder': folder.id, - 'blob': s3ff_field_value, + 'size': 42, }, ) assert resp.status_code == 403 From 3d8d45166ec7cdef9646ac57861685339b58e1ff Mon Sep 17 00:00:00 2001 From: Zach Mullen Date: Wed, 3 Feb 2021 10:54:15 -0500 Subject: [PATCH 2/6] Fix file creation via admin page --- dkc/core/admin/file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dkc/core/admin/file.py b/dkc/core/admin/file.py index 13f1d1c..41b8540 100644 --- a/dkc/core/admin/file.py +++ b/dkc/core/admin/file.py @@ -37,7 +37,7 @@ class FileAdmin(admin.ModelAdmin): autocomplete_fields = ['folder'] def get_readonly_fields(self, request, obj=None): - fields = ['sha512', 'size', 'created', 'modified', 'creator'] + fields = ['sha512', 'created', 'modified'] # Allow setting of folder only on initial creation if obj is None: return fields From a438b4358a8c76b7f2affacfe4def0afee82864a Mon Sep 17 00:00:00 2001 From: Zach Mullen Date: Thu, 4 Feb 2021 19:11:17 -0500 Subject: [PATCH 3/6] Set blank=True on File.blob --- dkc/core/migrations/0009_file_pre_creation.py | 8 +++++++- dkc/core/models/file.py | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/dkc/core/migrations/0009_file_pre_creation.py b/dkc/core/migrations/0009_file_pre_creation.py index 80f7f78..3d47e6e 100644 --- a/dkc/core/migrations/0009_file_pre_creation.py +++ b/dkc/core/migrations/0009_file_pre_creation.py @@ -1,8 +1,9 @@ -# Generated by Django 3.1.6 on 2021-02-03 15:40 +# Generated by Django 3.1.6 on 2021-02-05 00:07 from django.conf import settings from django.db import migrations, models import django.db.models.deletion +import s3_file_field.fields class Migration(migrations.Migration): @@ -13,6 +14,11 @@ class Migration(migrations.Migration): ] operations = [ + migrations.AlterField( + model_name='file', + name='blob', + field=s3_file_field.fields.S3FileField(blank=True), + ), migrations.AlterField( model_name='file', name='size', diff --git a/dkc/core/models/file.py b/dkc/core/models/file.py index 2637456..36c8ab5 100644 --- a/dkc/core/models/file.py +++ b/dkc/core/models/file.py @@ -36,7 +36,7 @@ class Meta: description = models.TextField(max_length=3000, blank=True) size = models.PositiveBigIntegerField() content_type = models.CharField(max_length=255, default='application/octet-stream') - blob = S3FileField() + blob = S3FileField(blank=True) sha512 = models.CharField(max_length=128, blank=True, default='', db_index=True, editable=False) user_metadata = JSONObjectField() folder = models.ForeignKey(Folder, on_delete=models.CASCADE, related_name='files') From ef6a7bc8f1f6365e76bfeb5de903ca68c1690fa3 Mon Sep 17 00:00:00 2001 From: Zach Mullen Date: Thu, 4 Feb 2021 19:26:26 -0500 Subject: [PATCH 4/6] Change pending file download to HTTP 204 --- dkc/core/rest/file.py | 9 ++++----- dkc/core/tests/test_file_rest.py | 6 +----- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/dkc/core/rest/file.py b/dkc/core/rest/file.py index 95516d8..93bb454 100644 --- a/dkc/core/rest/file.py +++ b/dkc/core/rest/file.py @@ -1,11 +1,10 @@ -from io import BytesIO - from django.contrib.auth.models import User from django.core.exceptions import PermissionDenied -from django.http import FileResponse, HttpResponseRedirect +from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404 from rest_framework import serializers from rest_framework.decorators import action +from rest_framework.response import Response from rest_framework.viewsets import ModelViewSet from dkc.core.models import File, Folder @@ -93,6 +92,6 @@ def perform_create(self, serializer: FileSerializer): @action(detail=True) def download(self, request, pk=None): file = get_object_or_404(File, pk=pk) - if file.blob: # FileFields are falsy when not populated with a file + if file.blob: # FieldFiles are falsy when not populated with a file return HttpResponseRedirect(file.blob.url) - return FileResponse(BytesIO(), filename=file.name, content_type=file.content_type) + return Response(status=204) diff --git a/dkc/core/tests/test_file_rest.py b/dkc/core/tests/test_file_rest.py index 7b56df9..c9acd12 100644 --- a/dkc/core/tests/test_file_rest.py +++ b/dkc/core/tests/test_file_rest.py @@ -43,11 +43,7 @@ def test_file_rest_create_process(admin_api_client, folder): def test_file_rest_download_pending_file(admin_api_client, pending_file): """Test downloading a file prior to its blob being set does something sane.""" resp = admin_api_client.get(f'/api/v2/files/{pending_file.id}/download') - assert resp.status_code == 200 - assert b''.join(resp.streaming_content) == b'' - assert resp['content-type'] == pending_file.content_type - assert resp['content-length'] == '0' - assert pending_file.name in resp['content-disposition'] + assert resp.status_code == 204 @pytest.mark.django_db From d3c667da8bb053e68662829ff556b8e14f80bfd2 Mon Sep 17 00:00:00 2001 From: Zach Mullen Date: Thu, 4 Feb 2021 19:27:55 -0500 Subject: [PATCH 5/6] Use SelfAttribute rather than LazyAttribute --- dkc/core/tests/factories.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dkc/core/tests/factories.py b/dkc/core/tests/factories.py index 7d5061a..c64ef52 100644 --- a/dkc/core/tests/factories.py +++ b/dkc/core/tests/factories.py @@ -54,7 +54,7 @@ class Meta: name = factory.Faker('file_name') description = factory.Faker('paragraph') blob = factory.django.FileField(data=b'fakefilebytes', filename='fake.txt') - size = factory.LazyAttribute(lambda obj: obj.blob.size) + size = factory.SelfAttribute('blob.size') user_metadata = _metadata_faker folder = factory.SubFactory(FolderFactory) creator = factory.SubFactory(UserFactory) From 6e285496f7de730770004fa88588c5f54db4c88a Mon Sep 17 00:00:00 2001 From: Zach Mullen Date: Thu, 4 Feb 2021 19:30:33 -0500 Subject: [PATCH 6/6] Minor cleanups in test cases --- dkc/core/tests/test_file_rest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dkc/core/tests/test_file_rest.py b/dkc/core/tests/test_file_rest.py index c9acd12..824b327 100644 --- a/dkc/core/tests/test_file_rest.py +++ b/dkc/core/tests/test_file_rest.py @@ -35,7 +35,7 @@ def test_file_rest_create_process(admin_api_client, folder): assert resp.data['size'] == 42 folder.refresh_from_db() assert folder.size == 42 - saved_file = File.objects.get(pk=resp.data['id']) + saved_file = File.objects.get(id=resp.data['id']) assert bool(saved_file.blob) is False @@ -60,4 +60,4 @@ def test_file_rest_set_blob(admin_api_client, pending_file, s3ff_field_value): assert resp.status_code == 200 assert resp.data['size'] == pending_file.size pending_file.refresh_from_db() - assert bool(pending_file.blob) is True + assert pending_file.blob