From 012094d5242a8978ac459c070734ecb131c28d30 Mon Sep 17 00:00:00 2001 From: Vivek Agrawal Date: Wed, 19 Apr 2023 23:11:17 +0530 Subject: [PATCH 1/8] Defensive file duration check --- contentcuration/contentcuration/viewsets/file.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contentcuration/contentcuration/viewsets/file.py b/contentcuration/contentcuration/viewsets/file.py index 132bfe0769..922d6f276d 100644 --- a/contentcuration/contentcuration/viewsets/file.py +++ b/contentcuration/contentcuration/viewsets/file.py @@ -148,6 +148,8 @@ def upload_url(self, request): if not isinstance(duration, (int, float)): return HttpResponseBadRequest(reason="File duration must be a number") duration = math.floor(duration) + if duration <= 0: + return HttpResponseBadRequest(reason="File duration is equal to or less than 0") try: request.user.check_space(float(size), checksum) From 87b522edc3cdfcc3eae182e9cfecba0f21077a89 Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Wed, 26 Apr 2023 11:54:54 -0700 Subject: [PATCH 2/8] Add dependency types to MEDIA_PRESETS --- contentcuration/contentcuration/models.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index f9f5852f31..0baabfb7ce 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -2076,7 +2076,13 @@ class StagedFile(models.Model): FILE_DISTINCT_INDEX_NAME = "file_checksum_file_size_idx" FILE_MODIFIED_DESC_INDEX_NAME = "file_modified_desc_idx" FILE_DURATION_CONSTRAINT = "file_media_duration_int" -MEDIA_PRESETS = [format_presets.AUDIO, format_presets.VIDEO_HIGH_RES, format_presets.VIDEO_LOW_RES] +MEDIA_PRESETS = [ + format_presets.AUDIO, + format_presets.AUDIO_DEPENDENCY, + format_presets.VIDEO_HIGH_RES, + format_presets.VIDEO_LOW_RES, + format_presets.VIDEO_DEPENDENCY, +] class File(models.Model): From 91cf944ad68fbcf56eef503175027add6c36ccc8 Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Wed, 26 Apr 2023 11:57:36 -0700 Subject: [PATCH 3/8] Temporarily remove check constraint --- .../0142_remove_file_file_media_duration_int.py | 16 ++++++++++++++++ contentcuration/contentcuration/models.py | 4 +++- 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 contentcuration/contentcuration/migrations/0142_remove_file_file_media_duration_int.py diff --git a/contentcuration/contentcuration/migrations/0142_remove_file_file_media_duration_int.py b/contentcuration/contentcuration/migrations/0142_remove_file_file_media_duration_int.py new file mode 100644 index 0000000000..e497fbd398 --- /dev/null +++ b/contentcuration/contentcuration/migrations/0142_remove_file_file_media_duration_int.py @@ -0,0 +1,16 @@ +# Generated by Django 3.2.18 on 2023-04-26 18:55 +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('contentcuration', '0141_add_task_signature'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='file', + name='file_media_duration_int', + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 0baabfb7ce..8bf951e5a3 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -2219,7 +2219,9 @@ class Meta: models.Index(fields=["-modified"], name=FILE_MODIFIED_DESC_INDEX_NAME), ] constraints = [ - models.CheckConstraint(check=(Q(preset__in=MEDIA_PRESETS, duration__gt=0) | Q(duration__isnull=True)), name=FILE_DURATION_CONSTRAINT) + # Commented out to temporarily remove the constraint until we can re-run `set_file_duration` + # on all media files with the dependency preset in the database. + # models.CheckConstraint(check=(Q(preset__in=MEDIA_PRESETS, duration__gt=0) | Q(duration__isnull=True)), name=FILE_DURATION_CONSTRAINT) ] From 9eebeff174bdb8c3e687dce3d9bd6dbae667135c Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Wed, 12 Apr 2023 10:02:14 -0700 Subject: [PATCH 4/8] Remove deprecated and no longer used codecov library. --- requirements-dev.in | 1 - requirements-dev.txt | 4 ---- 2 files changed, 5 deletions(-) diff --git a/requirements-dev.in b/requirements-dev.in index 6c3b1033f8..b2bfb0931c 100644 --- a/requirements-dev.in +++ b/requirements-dev.in @@ -14,7 +14,6 @@ pytest-pythonpath pytest-timeout pytest-watch pre-commit==1.15.1 -codecov coverage pytest-cov nodeenv diff --git a/requirements-dev.txt b/requirements-dev.txt index 383ee77801..07d875e98f 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -35,8 +35,6 @@ click==8.1.3 # -c requirements.txt # flask # pip-tools -codecov==2.1.12 - # via -r requirements-dev.in colorama==0.4.4 # via pytest-watch configargparse==1.5.3 @@ -50,7 +48,6 @@ coreschema==0.0.4 coverage[toml]==6.2 # via # -r requirements-dev.in - # codecov # pytest-cov customizable-django-profiler @ git+https://github.com/someshchaturvedi/customizable-django-profiler.git # via -r requirements-dev.in @@ -220,7 +217,6 @@ pyzmq==23.1.0 requests==2.25.1 # via # -c requirements.txt - # codecov # coreapi # locust roundrobin==0.0.2 From 3cc1292ee4db3d4c11d04531aa144fc304148e89 Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Wed, 26 Apr 2023 12:56:53 -0700 Subject: [PATCH 5/8] Skip tests for now --- contentcuration/contentcuration/tests/test_models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contentcuration/contentcuration/tests/test_models.py b/contentcuration/contentcuration/tests/test_models.py index 97d3359ce7..b8dfada4f8 100644 --- a/contentcuration/contentcuration/tests/test_models.py +++ b/contentcuration/contentcuration/tests/test_models.py @@ -651,6 +651,7 @@ def test_duration_check_constraint__acceptable(self): duration=1123123, ) + @pytest.mark.skip(reason="Temporarily removed constraint") def test_duration_check_constraint__negative(self): channel = testdata.channel() with self.assertRaises(IntegrityError, msg=FILE_DURATION_CONSTRAINT): @@ -660,6 +661,7 @@ def test_duration_check_constraint__negative(self): duration=-10, ) + @pytest.mark.skip(reason="Temporarily removed constraint") def test_duration_check_constraint__not_media(self): channel = testdata.channel() with self.assertRaises(IntegrityError, msg=FILE_DURATION_CONSTRAINT): From 2f2c28ef8e88776112ad82380a0596276ae1b335 Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Wed, 3 May 2023 08:29:35 -0700 Subject: [PATCH 6/8] Re-add constraint for file-duration --- .../0143_file_file_media_duration_int.py | 17 +++++++++++++++++ contentcuration/contentcuration/models.py | 9 ++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) create mode 100644 contentcuration/contentcuration/migrations/0143_file_file_media_duration_int.py diff --git a/contentcuration/contentcuration/migrations/0143_file_file_media_duration_int.py b/contentcuration/contentcuration/migrations/0143_file_file_media_duration_int.py new file mode 100644 index 0000000000..3a7dbae1a0 --- /dev/null +++ b/contentcuration/contentcuration/migrations/0143_file_file_media_duration_int.py @@ -0,0 +1,17 @@ +# Generated by Django 3.2.18 on 2023-05-03 15:28 +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contentcuration', '0142_remove_file_file_media_duration_int'), + ] + + operations = [ + migrations.AddConstraint( + model_name='file', + constraint=models.CheckConstraint(check=models.Q(models.Q(('duration__gt', 0), ('preset__in', ['audio', 'audio_dependency', 'high_res_video', 'low_res_video', 'video_dependency'])), ('duration__isnull', True), _connector='OR'), name='file_media_duration_int'), + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 8bf951e5a3..133aebcb0c 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -2219,9 +2219,12 @@ class Meta: models.Index(fields=["-modified"], name=FILE_MODIFIED_DESC_INDEX_NAME), ] constraints = [ - # Commented out to temporarily remove the constraint until we can re-run `set_file_duration` - # on all media files with the dependency preset in the database. - # models.CheckConstraint(check=(Q(preset__in=MEDIA_PRESETS, duration__gt=0) | Q(duration__isnull=True)), name=FILE_DURATION_CONSTRAINT) + # enforces that duration is null when not a media preset, but the duration may be null for media presets + # but if not-null, should be greater than 0 + models.CheckConstraint( + check=(Q(preset__in=MEDIA_PRESETS, duration__gt=0) | Q(duration__isnull=True)), + name=FILE_DURATION_CONSTRAINT + ) ] From 5c6183626c8ca088f230901356042cb39e55d50f Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Wed, 3 May 2023 08:30:28 -0700 Subject: [PATCH 7/8] Revert "Skip tests for now" This reverts commit 3cc1292ee4db3d4c11d04531aa144fc304148e89. --- contentcuration/contentcuration/tests/test_models.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/contentcuration/contentcuration/tests/test_models.py b/contentcuration/contentcuration/tests/test_models.py index b8dfada4f8..97d3359ce7 100644 --- a/contentcuration/contentcuration/tests/test_models.py +++ b/contentcuration/contentcuration/tests/test_models.py @@ -651,7 +651,6 @@ def test_duration_check_constraint__acceptable(self): duration=1123123, ) - @pytest.mark.skip(reason="Temporarily removed constraint") def test_duration_check_constraint__negative(self): channel = testdata.channel() with self.assertRaises(IntegrityError, msg=FILE_DURATION_CONSTRAINT): @@ -661,7 +660,6 @@ def test_duration_check_constraint__negative(self): duration=-10, ) - @pytest.mark.skip(reason="Temporarily removed constraint") def test_duration_check_constraint__not_media(self): channel = testdata.channel() with self.assertRaises(IntegrityError, msg=FILE_DURATION_CONSTRAINT): From 774b59ba208e94fbfecdd3507915891166594ea8 Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Thu, 11 May 2023 07:30:04 -0700 Subject: [PATCH 8/8] Fix migration ordering --- .../{0142_add_task_signature.py => 0141_add_task_signature.py} | 2 +- .../{0141_soft_delete_user.py => 0144_soft_delete_user.py} | 2 +- contentcuration/kolibri_public/migrations/0001_initial.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename contentcuration/contentcuration/migrations/{0142_add_task_signature.py => 0141_add_task_signature.py} (94%) rename contentcuration/contentcuration/migrations/{0141_soft_delete_user.py => 0144_soft_delete_user.py} (94%) diff --git a/contentcuration/contentcuration/migrations/0142_add_task_signature.py b/contentcuration/contentcuration/migrations/0141_add_task_signature.py similarity index 94% rename from contentcuration/contentcuration/migrations/0142_add_task_signature.py rename to contentcuration/contentcuration/migrations/0141_add_task_signature.py index 194580211c..4e182e8fa1 100644 --- a/contentcuration/contentcuration/migrations/0142_add_task_signature.py +++ b/contentcuration/contentcuration/migrations/0141_add_task_signature.py @@ -11,7 +11,7 @@ def __init__(self, name, app_label): super(Migration, self).__init__(name, 'django_celery_results') dependencies = [ - ('contentcuration', '0141_soft_delete_user'), + ('contentcuration', '0140_delete_task'), ('django_celery_results', '0011_taskresult_periodic_task_name'), ] diff --git a/contentcuration/contentcuration/migrations/0141_soft_delete_user.py b/contentcuration/contentcuration/migrations/0144_soft_delete_user.py similarity index 94% rename from contentcuration/contentcuration/migrations/0141_soft_delete_user.py rename to contentcuration/contentcuration/migrations/0144_soft_delete_user.py index df66bafcc0..a04040df69 100644 --- a/contentcuration/contentcuration/migrations/0141_soft_delete_user.py +++ b/contentcuration/contentcuration/migrations/0144_soft_delete_user.py @@ -9,7 +9,7 @@ class Migration(migrations.Migration): dependencies = [ - ('contentcuration', '0140_delete_task'), + ('contentcuration', '0143_file_file_media_duration_int'), ] operations = [ diff --git a/contentcuration/kolibri_public/migrations/0001_initial.py b/contentcuration/kolibri_public/migrations/0001_initial.py index d364eac438..f2186f6236 100644 --- a/contentcuration/kolibri_public/migrations/0001_initial.py +++ b/contentcuration/kolibri_public/migrations/0001_initial.py @@ -11,7 +11,7 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ("contentcuration", "0142_add_task_signature"), + ("contentcuration", "0141_add_task_signature"), ] operations = [