From 4e7f1e63d2fb99665194a0dfc01780d6f3cc499e Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sat, 19 Oct 2019 18:10:42 +0200 Subject: [PATCH 1/6] Denormalized audio permission logic in a separate table to enhance performance --- api/config/settings/common.py | 4 +- api/config/settings/local.py | 16 ++ api/funkwhale_api/federation/factories.py | 3 + api/funkwhale_api/federation/models.py | 40 +++++ .../commands/rebuild_music_permissions.py | 59 +++++++ .../migrations/0040_auto_20191021_1318.py | 32 ++++ .../migrations/0041_auto_20191021_1705.py | 47 +++++ .../0042_denormalize_audio_permissions.py.bak | 40 +++++ api/funkwhale_api/music/models.py | 143 ++++++++++++++- api/funkwhale_api/music/tasks.py | 9 +- api/requirements/local.txt | 1 + api/setup.cfg | 1 + api/tests/federation/test_models.py | 81 +++++++++ api/tests/music/test_models.py | 167 ++++++++++++++++-- api/tests/music/test_tasks.py | 8 + .../changelog.d/denormalization.enhancement | 1 + changes/notes.rst | 17 ++ 17 files changed, 648 insertions(+), 21 deletions(-) create mode 100644 api/funkwhale_api/music/management/commands/rebuild_music_permissions.py create mode 100644 api/funkwhale_api/music/migrations/0040_auto_20191021_1318.py create mode 100644 api/funkwhale_api/music/migrations/0041_auto_20191021_1705.py create mode 100644 api/funkwhale_api/music/migrations/0042_denormalize_audio_permissions.py.bak create mode 100644 changes/changelog.d/denormalization.enhancement diff --git a/api/config/settings/common.py b/api/config/settings/common.py index 43a7a29f5..5d8826075 100644 --- a/api/config/settings/common.py +++ b/api/config/settings/common.py @@ -851,7 +851,9 @@ MUSIC_DIRECTORY_PATH = env("MUSIC_DIRECTORY_PATH", default=None) MUSIC_DIRECTORY_SERVE_PATH = env( "MUSIC_DIRECTORY_SERVE_PATH", default=MUSIC_DIRECTORY_PATH ) - +# When this is set to default=True, we need to reenable migration music/0042 +# to ensure data is populated correctly on existing pods +MUSIC_USE_DENORMALIZATION = env.bool("MUSIC_USE_DENORMALIZATION", default=False) USERS_INVITATION_EXPIRATION_DAYS = env.int( "USERS_INVITATION_EXPIRATION_DAYS", default=14 ) diff --git a/api/config/settings/local.py b/api/config/settings/local.py index 632eb3201..24af04e5a 100644 --- a/api/config/settings/local.py +++ b/api/config/settings/local.py @@ -41,6 +41,22 @@ DEBUG_TOOLBAR_CONFIG = { "SHOW_TOOLBAR_CALLBACK": lambda request: True, "JQUERY_URL": "/staticfiles/admin/js/vendor/jquery/jquery.js", } +# DEBUG_TOOLBAR_PANELS = [ +# 'debug_toolbar.panels.versions.VersionsPanel', +# 'debug_toolbar.panels.timer.TimerPanel', +# 'debug_toolbar.panels.settings.SettingsPanel', +# 'debug_toolbar.panels.headers.HeadersPanel', +# 'debug_toolbar.panels.request.RequestPanel', +# 'debug_toolbar.panels.sql.SQLPanel', +# 'debug_toolbar.panels.staticfiles.StaticFilesPanel', +# 'debug_toolbar.panels.templates.TemplatesPanel', +# 'debug_toolbar.panels.cache.CachePanel', +# 'debug_toolbar.panels.signals.SignalsPanel', +# 'debug_toolbar.panels.logging.LoggingPanel', +# 'debug_toolbar.panels.redirects.RedirectsPanel', +# 'debug_toolbar.panels.profiling.ProfilingPanel', +# 'debug_toolbar_line_profiler.panel.ProfilingPanel' +# ] # django-extensions # ------------------------------------------------------------------------------ diff --git a/api/funkwhale_api/federation/factories.py b/api/funkwhale_api/federation/factories.py index 95d68779b..4f12729fc 100644 --- a/api/funkwhale_api/federation/factories.py +++ b/api/funkwhale_api/federation/factories.py @@ -165,6 +165,9 @@ class MusicLibraryFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): class Meta: model = "music.Library" + class Params: + local = factory.Trait(actor=factory.SubFactory(ActorFactory, local=True)) + @registry.register class LibraryScanFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): diff --git a/api/funkwhale_api/federation/models.py b/api/funkwhale_api/federation/models.py index c734939b3..fd52e17c8 100644 --- a/api/funkwhale_api/federation/models.py +++ b/api/funkwhale_api/federation/models.py @@ -9,6 +9,8 @@ from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ObjectDoesNotExist from django.core.serializers.json import DjangoJSONEncoder from django.db import models +from django.db.models.signals import post_save, pre_save, post_delete +from django.dispatch import receiver from django.utils import timezone from django.urls import reverse @@ -554,3 +556,41 @@ class LibraryTrack(models.Model): def get_metadata(self, key): return self.metadata.get(key) + + +@receiver(pre_save, sender=LibraryFollow) +def set_approved_updated(sender, instance, update_fields, **kwargs): + if not instance.pk or not instance.actor.is_local: + return + if update_fields is not None and "approved" not in update_fields: + return + db_value = instance.__class__.objects.filter(pk=instance.pk).values_list( + "approved", flat=True + )[0] + if db_value != instance.approved: + # Needed to update denormalized permissions + setattr(instance, "_approved_updated", True) + + +@receiver(post_save, sender=LibraryFollow) +def update_denormalization_follow_approved(sender, instance, created, **kwargs): + from funkwhale_api.music import models as music_models + + updated = getattr(instance, "_approved_updated", False) + + if (created or updated) and instance.actor.is_local: + music_models.TrackActor.create_entries( + instance.target, + actor_ids=[instance.actor.pk], + delete_existing=not instance.approved, + ) + + +@receiver(post_delete, sender=LibraryFollow) +def update_denormalization_follow_deleted(sender, instance, **kwargs): + from funkwhale_api.music import models as music_models + + if instance.actor.is_local: + music_models.TrackActor.objects.filter( + actor=instance.actor, upload__in=instance.target.uploads.all() + ).delete() diff --git a/api/funkwhale_api/music/management/commands/rebuild_music_permissions.py b/api/funkwhale_api/music/management/commands/rebuild_music_permissions.py new file mode 100644 index 000000000..9a3e849d2 --- /dev/null +++ b/api/funkwhale_api/music/management/commands/rebuild_music_permissions.py @@ -0,0 +1,59 @@ +from argparse import RawTextHelpFormatter + +from django.core.management.base import BaseCommand +from django.core.management.base import CommandError + +from django.db import transaction +from django.db.models import Q + +from funkwhale_api.music.models import TrackActor, Library +from funkwhale_api.federation.models import Actor + + +class Command(BaseCommand): + help = """ + Rebuild audio permission table. You shouldn't have to do this by hand, but if you face + any weird things (tracks still shown when they shouldn't, or tracks not shown when they should), + this may help. + + """ + + def create_parser(self, *args, **kwargs): + parser = super().create_parser(*args, **kwargs) + parser.formatter_class = RawTextHelpFormatter + return parser + + def add_arguments(self, parser): + parser.add_argument( + "username", + nargs='*', + help="Rebuild only for given users", + ) + + @transaction.atomic + def handle(self, *args, **options): + actor_ids = [] + if options['username']: + actors = Actor.objects.all().local(True) + actor_ids = list(actors.filter(preferred_username__in=options['username']).values_list('id', flat=True)) + if len(actor_ids) < len(options['username']): + raise CommandError('Invalid username') + print('Emptying permission table for specified users…') + qs = TrackActor.objects.all().filter(Q(actor__pk__in=actor_ids) | Q(actor=None)) + qs._raw_delete(qs.db) + else: + print('Emptying permission table…') + qs = TrackActor.objects.all() + qs._raw_delete(qs.db) + libraries = Library.objects.all() + objs = [] + total_libraries = len(libraries) + for i, library in enumerate(libraries): + print('[{}/{}] Populating permission table for library {}'.format(i + 1, total_libraries, library.pk)) + objs += TrackActor.get_objs( + library=library, + actor_ids=actor_ids, + upload_and_track_ids=[], + ) + print('Commiting changes…') + TrackActor.objects.bulk_create(objs, batch_size=5000, ignore_conflicts=True) diff --git a/api/funkwhale_api/music/migrations/0040_auto_20191021_1318.py b/api/funkwhale_api/music/migrations/0040_auto_20191021_1318.py new file mode 100644 index 000000000..eb5e6abc0 --- /dev/null +++ b/api/funkwhale_api/music/migrations/0040_auto_20191021_1318.py @@ -0,0 +1,32 @@ +# Generated by Django 2.2.5 on 2019-10-21 13:18 + +from django.conf import settings +import django.contrib.postgres.fields.jsonb +import django.core.serializers.json +from django.db import migrations, models +import django.db.models.deletion +import funkwhale_api.music.models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('music', '0039_auto_20190423_0820'), + ] + + operations = [ + migrations.CreateModel( + name='TrackActor', + fields=[ + ('id', models.BigAutoField(primary_key=True, serialize=False)), + ('internal', models.BooleanField(db_index=True, default=False)), + ('track', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='track_actor_items', to='music.Track')), + ('upload', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='track_actor_items', to='music.Upload')), + ('actor', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='track_actor_items', to='federation.Actor')), + ], + options={ + 'unique_together': {('track', 'actor', 'internal', 'upload')}, + }, + ), + ] diff --git a/api/funkwhale_api/music/migrations/0041_auto_20191021_1705.py b/api/funkwhale_api/music/migrations/0041_auto_20191021_1705.py new file mode 100644 index 000000000..cd98e7707 --- /dev/null +++ b/api/funkwhale_api/music/migrations/0041_auto_20191021_1705.py @@ -0,0 +1,47 @@ +# Generated by Django 2.2.5 on 2019-10-21 17:05 + +import django.contrib.postgres.fields.jsonb +import django.core.serializers.json +from django.db import migrations, models +import django.db.models.deletion +import funkwhale_api.music.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('music', '0040_auto_20191021_1318'), + ] + + operations = [ + migrations.AlterField( + model_name='album', + name='release_date', + field=models.DateField(blank=True, db_index=True, null=True), + ), + migrations.AlterField( + model_name='upload', + name='from_activity', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to='federation.Activity'), + ), + migrations.AlterField( + model_name='upload', + name='import_details', + field=django.contrib.postgres.fields.jsonb.JSONField(blank=True, default=funkwhale_api.music.models.empty_dict, encoder=django.core.serializers.json.DjangoJSONEncoder, max_length=50000), + ), + migrations.AlterField( + model_name='upload', + name='import_metadata', + field=django.contrib.postgres.fields.jsonb.JSONField(blank=True, default=funkwhale_api.music.models.empty_dict, encoder=django.core.serializers.json.DjangoJSONEncoder, max_length=50000), + ), + migrations.AlterField( + model_name='upload', + name='metadata', + field=django.contrib.postgres.fields.jsonb.JSONField(blank=True, default=funkwhale_api.music.models.empty_dict, encoder=django.core.serializers.json.DjangoJSONEncoder, max_length=50000), + ), + migrations.AlterField( + model_name='uploadversion', + name='mimetype', + field=models.CharField(choices=[('video/ogg', 'ogg'), ('audio/ogg', 'ogg'), ('audio/opus', 'opus'), ('audio/mpeg', 'mp3'), ('audio/x-m4a', 'aac'), ('audio/x-m4a', 'm4a'), ('audio/x-flac', 'flac'), ('audio/flac', 'flac')], max_length=50), + ), + ] diff --git a/api/funkwhale_api/music/migrations/0042_denormalize_audio_permissions.py.bak b/api/funkwhale_api/music/migrations/0042_denormalize_audio_permissions.py.bak new file mode 100644 index 000000000..625035bf7 --- /dev/null +++ b/api/funkwhale_api/music/migrations/0042_denormalize_audio_permissions.py.bak @@ -0,0 +1,40 @@ +# -*- coding: utf-8 -*- +""" +This migration is disabled until settings.MUSIC_USE_DENORMALIZATION is set to default=True +""" +from __future__ import unicode_literals + +from django.db import migrations +from funkwhale_api.music.utils import guess_mimetype + + +def denormalize(apps, schema_editor): + Upload = apps.get_model("music", "Upload") + if not Upload.objects.count(): + print('Skipping…') + + from funkwhale_api.music.models import TrackActor, Library + libraries = Library.objects.all() + objs = [] + total_libraries = len(libraries) + for i, library in enumerate(libraries): + print('[{}/{}] Populating permission table for library {}'.format(i+1, total_libraries, library.pk)) + objs += TrackActor.get_objs( + library=library, + actor_ids=[], + upload_and_track_ids=[], + ) + print('Commiting changes…') + TrackActor.objects.bulk_create(objs, batch_size=5000, ignore_conflicts=True) + + + +def rewind(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + + dependencies = [("music", "0041_auto_20191021_1705")] + + operations = [migrations.RunPython(denormalize, rewind)] diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 49d22fa85..75126eda2 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -14,7 +14,7 @@ from django.contrib.postgres.fields import JSONField from django.core.files.base import ContentFile from django.core.serializers.json import DjangoJSONEncoder from django.db import models, transaction -from django.db.models.signals import post_save +from django.db.models.signals import post_save, pre_save from django.dispatch import receiver from django.urls import reverse from django.utils import timezone @@ -185,8 +185,8 @@ class ArtistQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet): return self.annotate(is_playable_by_actor=subquery) def playable_by(self, actor, include=True): - tracks = Track.objects.playable_by(actor, include) - matches = self.filter(tracks__in=tracks).values_list("pk") + tracks = Track.objects.playable_by(actor) + matches = self.filter(pk__in=tracks.values("artist_id")).values_list("pk") if include: return self.filter(pk__in=matches) else: @@ -269,8 +269,8 @@ class AlbumQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet): return self.annotate(is_playable_by_actor=subquery) def playable_by(self, actor, include=True): - tracks = Track.objects.playable_by(actor, include) - matches = self.filter(tracks__in=tracks).values_list("pk") + tracks = Track.objects.playable_by(actor) + matches = self.filter(pk__in=tracks.values("album_id")).values_list("pk") if include: return self.filter(pk__in=matches) else: @@ -418,6 +418,7 @@ class TrackQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet): return self.select_related().select_related("album__artist", "artist") def annotate_playable_by_actor(self, actor): + files = ( Upload.objects.playable_by(actor) .filter(track=models.OuterRef("id")) @@ -428,6 +429,15 @@ class TrackQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet): return self.annotate(is_playable_by_actor=subquery) def playable_by(self, actor, include=True): + + if settings.MUSIC_USE_DENORMALIZATION: + if actor is not None: + query = models.Q(actor=None) | models.Q(actor=actor) + else: + query = models.Q(actor=None, internal=False) + if not include: + query = ~query + return self.filter(pk__in=TrackActor.objects.filter(query).values("track")) files = Upload.objects.playable_by(actor, include) matches = self.filter(uploads__in=files).values_list("pk") if include: @@ -1147,11 +1157,134 @@ class LibraryScan(models.Model): modification_date = models.DateTimeField(null=True, blank=True) +class TrackActor(models.Model): + """ + Denormalization table to store all playable tracks for a given user + Empty user means the track is public or internal (cf internal flag too) + """ + + id = models.BigAutoField(primary_key=True) + actor = models.ForeignKey( + "federation.Actor", + on_delete=models.CASCADE, + related_name="track_actor_items", + blank=True, + null=True, + ) + track = models.ForeignKey( + Track, on_delete=models.CASCADE, related_name="track_actor_items" + ) + upload = models.ForeignKey( + Upload, on_delete=models.CASCADE, related_name="track_actor_items" + ) + internal = models.BooleanField(default=False, db_index=True) + + class Meta: + unique_together = ("track", "actor", "internal", "upload") + + @classmethod + def get_objs(cls, library, actor_ids, upload_and_track_ids): + upload_and_track_ids = upload_and_track_ids or library.uploads.filter( + import_status="finished", track__isnull=False + ).values_list("id", "track") + objs = [] + if library.privacy_level == "me": + follow_queryset = library.received_follows.filter(approved=True).exclude( + actor__user__isnull=True + ) + if actor_ids: + follow_queryset = follow_queryset.filter(actor__pk__in=actor_ids) + final_actor_ids = list(follow_queryset.values_list("actor", flat=True)) + + owner = library.actor if library.actor.is_local else None + if owner and (not actor_ids or owner in final_actor_ids): + final_actor_ids.append(owner.pk) + for actor_id in final_actor_ids: + for upload_id, track_id in upload_and_track_ids: + objs.append( + cls(actor_id=actor_id, track_id=track_id, upload_id=upload_id) + ) + + elif library.privacy_level == "instance": + for upload_id, track_id in upload_and_track_ids: + objs.append( + cls( + actor_id=None, + track_id=track_id, + upload_id=upload_id, + internal=True, + ) + ) + elif library.privacy_level == "everyone": + for upload_id, track_id in upload_and_track_ids: + objs.append(cls(actor_id=None, track_id=track_id, upload_id=upload_id)) + return objs + + @classmethod + def create_entries( + cls, library, delete_existing=True, actor_ids=None, upload_and_track_ids=None + ): + if not settings.MUSIC_USE_DENORMALIZATION: + # skip + return + if delete_existing: + to_delete = cls.objects.filter(upload__library=library) + if actor_ids: + to_delete = to_delete.filter(actor__pk__in=actor_ids) + # we don't use .delete() here because we don't want signals to fire + to_delete._raw_delete(to_delete.db) + + objs = cls.get_objs( + library, actor_ids=actor_ids, upload_and_track_ids=upload_and_track_ids + ) + return cls.objects.bulk_create(objs, ignore_conflicts=True, batch_size=5000) + + @receiver(post_save, sender=ImportJob) def update_batch_status(sender, instance, **kwargs): instance.batch.update_status() +@receiver(post_save, sender=Upload) +def update_denormalization_track_actor(sender, instance, created, **kwargs): + if ( + created + and settings.MUSIC_USE_DENORMALIZATION + and instance.track_id + and instance.import_status == "finished" + ): + TrackActor.create_entries( + instance.library, + delete_existing=False, + upload_and_track_ids=[(instance.pk, instance.track_id)], + ) + + +@receiver(pre_save, sender=Library) +def set_privacy_level_updated(sender, instance, update_fields, **kwargs): + if not instance.pk: + return + if update_fields is not None and "privacy_level" not in update_fields: + return + db_value = instance.__class__.objects.filter(pk=instance.pk).values_list( + "privacy_level", flat=True + )[0] + if db_value != instance.privacy_level: + # Needed to update denormalized permissions + setattr(instance, "_privacy_level_updated", True) + + +@receiver(post_save, sender=Library) +def update_denormalization_track_user_library_privacy_level( + sender, instance, created, **kwargs +): + if created: + return + updated = getattr(instance, "_privacy_level_updated", False) + if updated: + TrackActor.create_entries(instance) + + @receiver(post_save, sender=ImportBatch) def update_request_status(sender, instance, created, **kwargs): update_fields = kwargs.get("update_fields", []) or [] diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index ca32c808e..20265b320 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -170,7 +170,7 @@ def fail_import(upload, error_code, detail=None, **fields): ), "upload", ) -def process_upload(upload): +def process_upload(upload, update_denormalization=True): import_metadata = upload.import_metadata or {} old_status = upload.import_status audio_file = upload.get_audio_file() @@ -249,6 +249,13 @@ def process_upload(upload): ] ) + if update_denormalization: + models.TrackActor.create_entries( + library=upload.library, + upload_and_track_ids=[(upload.pk, upload.track_id)], + delete_existing=False, + ) + # update album cover, if needed if not track.album.cover: update_album_cover( diff --git a/api/requirements/local.txt b/api/requirements/local.txt index dcedb43e7..d60e07e7b 100644 --- a/api/requirements/local.txt +++ b/api/requirements/local.txt @@ -14,3 +14,4 @@ profiling asynctest==0.12.2 aioresponses==0.6.0 +https://github.com/dmclain/django-debug-toolbar-line-profiler/archive/master.zip diff --git a/api/setup.cfg b/api/setup.cfg index 3f7d2f7f0..f50bd5473 100644 --- a/api/setup.cfg +++ b/api/setup.cfg @@ -26,3 +26,4 @@ env = FORCE_HTTPS_URLS=False FUNKWHALE_SPA_HTML_ROOT=http://noop/ PROXY_MEDIA=true + MUSIC_USE_DENORMALIZATION=true diff --git a/api/tests/federation/test_models.py b/api/tests/federation/test_models.py index a7460b01c..667caaa78 100644 --- a/api/tests/federation/test_models.py +++ b/api/tests/federation/test_models.py @@ -2,6 +2,7 @@ import pytest from django import db from funkwhale_api.federation import models +from funkwhale_api.music import models as music_models def test_cannot_duplicate_actor(factories): @@ -174,3 +175,83 @@ def test_can_create_fetch_for_object(factories): assert fetch.status == "pending" assert fetch.detail == {} assert fetch.object == track + + +@pytest.mark.parametrize( + "initial_approved, updated_approved, initial_playable_tracks, updated_playable_tracks", + [ + ( + True, + False, + {"owner": [0], "follower": [0], "local_actor": [], None: []}, + {"owner": [0], "follower": [], "local_actor": [], None: []}, + ), + ( + False, + True, + {"owner": [0], "follower": [], "local_actor": [], None: []}, + {"owner": [0], "follower": [0], "local_actor": [], None: []}, + ), + ], +) +def test_update_library_follow_approved_create_entries( + initial_approved, + updated_approved, + initial_playable_tracks, + updated_playable_tracks, + factories, +): + actors = { + "owner": factories["federation.Actor"](local=True), + "follower": factories["federation.Actor"](local=True), + "local_actor": factories["federation.Actor"](local=True), + None: None, + } + library = factories["music.Library"](actor=actors["owner"], privacy_level="me") + + tracks = [ + factories["music.Upload"](playable=True, library=library).track, + factories["music.Upload"](library=library, import_status="pending").track, + ] + + follow = factories["federation.LibraryFollow"]( + target=library, actor=actors["follower"], approved=initial_approved + ) + + for actor_name, expected in initial_playable_tracks.items(): + actor = actors[actor_name] + expected_tracks = [tracks[i] for i in expected] + assert list(music_models.Track.objects.playable_by(actor)) == expected_tracks + + follow.approved = updated_approved + follow.save() + + for actor_name, expected in updated_playable_tracks.items(): + actor = actors[actor_name] + expected_tracks = [tracks[i] for i in expected] + assert list(music_models.Track.objects.playable_by(actor)) == expected_tracks + + +def test_update_library_follow_delete_delete_denormalization_entries(factories,): + updated_playable_tracks = {"owner": [0], "follower": []} + actors = { + "owner": factories["federation.Actor"](local=True), + "follower": factories["federation.Actor"](local=True), + } + library = factories["music.Library"](actor=actors["owner"], privacy_level="me") + + tracks = [ + factories["music.Upload"](playable=True, library=library).track, + factories["music.Upload"](library=library, import_status="pending").track, + ] + + follow = factories["federation.LibraryFollow"]( + target=library, actor=actors["follower"], approved=True + ) + + follow.delete() + + for actor_name, expected in updated_playable_tracks.items(): + actor = actors[actor_name] + expected_tracks = [tracks[i] for i in expected] + assert list(music_models.Track.objects.playable_by(actor)) == expected_tracks diff --git a/api/tests/music/test_models.py b/api/tests/music/test_models.py index 344273673..a5090e89a 100644 --- a/api/tests/music/test_models.py +++ b/api/tests/music/test_models.py @@ -212,7 +212,7 @@ def test_library(factories): ) def test_playable_by_correct_status(status, expected, factories): upload = factories["music.Upload"]( - library__privacy_level="everyone", import_status=status + library__privacy_level="everyone", import_status=status, library__local=True ) queryset = upload.library.uploads.playable_by(None) match = upload in list(queryset) @@ -224,7 +224,9 @@ def test_playable_by_correct_status(status, expected, factories): ) def test_playable_by_correct_actor(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) queryset = upload.library.uploads.playable_by(upload.library.actor) match = upload in list(queryset) @@ -236,7 +238,9 @@ def test_playable_by_correct_actor(privacy_level, expected, factories): ) def test_playable_by_instance_actor(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) instance_actor = factories["federation.Actor"](domain=upload.library.actor.domain) queryset = upload.library.uploads.playable_by(instance_actor) @@ -249,7 +253,9 @@ def test_playable_by_instance_actor(privacy_level, expected, factories): ) def test_playable_by_anonymous(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) queryset = upload.library.uploads.playable_by(None) match = upload in list(queryset) @@ -259,7 +265,7 @@ def test_playable_by_anonymous(privacy_level, expected, factories): @pytest.mark.parametrize("approved", [True, False]) def test_playable_by_follower(approved, factories): upload = factories["music.Upload"]( - library__privacy_level="me", import_status="finished" + library__privacy_level="me", import_status="finished", library__local=True ) actor = factories["federation.Actor"](local=True) factories["federation.LibraryFollow"]( @@ -275,7 +281,7 @@ def test_playable_by_follower(approved, factories): "privacy_level,expected", [("me", True), ("instance", True), ("everyone", True)] ) def test_track_playable_by_correct_actor(privacy_level, expected, factories): - upload = factories["music.Upload"](import_status="finished") + upload = factories["music.Upload"](import_status="finished", library__local=True) queryset = models.Track.objects.playable_by( upload.library.actor ).annotate_playable_by_actor(upload.library.actor) @@ -290,7 +296,9 @@ def test_track_playable_by_correct_actor(privacy_level, expected, factories): ) def test_track_playable_by_instance_actor(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) instance_actor = factories["federation.Actor"](domain=upload.library.actor.domain) queryset = models.Track.objects.playable_by( @@ -307,7 +315,9 @@ def test_track_playable_by_instance_actor(privacy_level, expected, factories): ) def test_track_playable_by_anonymous(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) queryset = models.Track.objects.playable_by(None).annotate_playable_by_actor(None) match = upload.track in list(queryset) @@ -320,7 +330,7 @@ def test_track_playable_by_anonymous(privacy_level, expected, factories): "privacy_level,expected", [("me", True), ("instance", True), ("everyone", True)] ) def test_album_playable_by_correct_actor(privacy_level, expected, factories): - upload = factories["music.Upload"](import_status="finished") + upload = factories["music.Upload"](import_status="finished", library__local=True) queryset = models.Album.objects.playable_by( upload.library.actor @@ -336,7 +346,9 @@ def test_album_playable_by_correct_actor(privacy_level, expected, factories): ) def test_album_playable_by_instance_actor(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) instance_actor = factories["federation.Actor"](domain=upload.library.actor.domain) queryset = models.Album.objects.playable_by( @@ -353,7 +365,9 @@ def test_album_playable_by_instance_actor(privacy_level, expected, factories): ) def test_album_playable_by_anonymous(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) queryset = models.Album.objects.playable_by(None).annotate_playable_by_actor(None) match = upload.track.album in list(queryset) @@ -366,7 +380,11 @@ def test_album_playable_by_anonymous(privacy_level, expected, factories): "privacy_level,expected", [("me", True), ("instance", True), ("everyone", True)] ) def test_artist_playable_by_correct_actor(privacy_level, expected, factories): - upload = factories["music.Upload"](import_status="finished") + upload = factories["music.Upload"]( + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, + ) queryset = models.Artist.objects.playable_by( upload.library.actor @@ -382,7 +400,9 @@ def test_artist_playable_by_correct_actor(privacy_level, expected, factories): ) def test_artist_playable_by_instance_actor(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) instance_actor = factories["federation.Actor"](domain=upload.library.actor.domain) queryset = models.Artist.objects.playable_by( @@ -399,7 +419,9 @@ def test_artist_playable_by_instance_actor(privacy_level, expected, factories): ) def test_artist_playable_by_anonymous(privacy_level, expected, factories): upload = factories["music.Upload"]( - library__privacy_level=privacy_level, import_status="finished" + library__privacy_level=privacy_level, + import_status="finished", + library__local=True, ) queryset = models.Artist.objects.playable_by(None).annotate_playable_by_actor(None) match = upload.track.artist in list(queryset) @@ -554,3 +576,120 @@ def test_api_model_mixin_domain_name(): obj = models.Track(fid="https://test.domain:543/something") assert obj.domain_name == "test.domain" + + +@pytest.mark.parametrize( + "initial, updated, expected", + [ + ({"name": "hello"}, {"name": "world"}, False), + ({"privacy_level": "internal"}, {"name": "world"}, False), + ({"privacy_level": "internal"}, {"privacy_level": "me"}, True), + ({"privacy_level": "internal"}, {"privacy_level": "internal"}, False), + ], +) +def test_saving_library_sets_privacy_level_updated_flag( + initial, updated, expected, factories +): + library = factories["music.Library"](**initial) + for key, value in updated.items(): + setattr(library, key, value) + + library.save() + + assert getattr(library, "_privacy_level_updated", False) is expected + + +@pytest.mark.parametrize("value, expected", [(True, True), (False, False)]) +def test_saving_library_with_privacy_level_updated_flag( + value, expected, factories, mocker +): + library = factories["music.Library"]() + create_entries = mocker.patch.object(models.TrackActor, "create_entries") + setattr(library, "_privacy_level_updated", value) + library.save() + + called = create_entries.call_count > 0 + assert called is expected + if expected: + create_entries.assert_called_once_with(library) + + +@pytest.mark.parametrize( + "initial_privacy_level, updated_privacy_level, initial_playable_tracks, updated_playable_tracks", + [ + ( + "me", + "everyone", + {"owner": [0], "follower": [0], "local_actor": [], None: []}, + {"owner": [0], "follower": [0], "local_actor": [0], None: [0]}, + ), + ( + "me", + "instance", + {"owner": [0], "follower": [0], "local_actor": [], None: []}, + {"owner": [0], "follower": [0], "local_actor": [0], None: []}, + ), + ( + "instance", + "me", + {"owner": [0], "follower": [0], "local_actor": [0], None: []}, + {"owner": [0], "follower": [0], "local_actor": [], None: []}, + ), + ( + "instance", + "everyone", + {"owner": [0], "follower": [0], "local_actor": [0], None: []}, + {"owner": [0], "follower": [0], "local_actor": [0], None: [0]}, + ), + ( + "everyone", + "me", + {"owner": [0], "follower": [0], "local_actor": [0], None: [0]}, + {"owner": [0], "follower": [0], "local_actor": [], None: []}, + ), + ( + "everyone", + "instance", + {"owner": [0], "follower": [0], "local_actor": [0], None: [0]}, + {"owner": [0], "follower": [0], "local_actor": [0], None: []}, + ), + ], +) +def test_update_library_privacy_level_create_entries( + initial_privacy_level, + updated_privacy_level, + initial_playable_tracks, + updated_playable_tracks, + factories, +): + actors = { + "owner": factories["federation.Actor"](local=True), + "follower": factories["federation.Actor"](local=True), + "local_actor": factories["federation.Actor"](local=True), + None: None, + } + library = factories["music.Library"]( + actor=actors["owner"], privacy_level=initial_privacy_level + ) + factories["federation.LibraryFollow"]( + target=library, actor=actors["follower"], approved=True + ) + + tracks = [ + factories["music.Upload"](playable=True, library=library).track, + factories["music.Upload"](library=library, import_status="pending").track, + ] + + for actor_name, expected in initial_playable_tracks.items(): + actor = actors[actor_name] + expected_tracks = [tracks[i] for i in expected] + assert list(models.Track.objects.playable_by(actor)) == expected_tracks + + library.privacy_level = updated_privacy_level + + models.TrackActor.create_entries(library) + + for actor_name, expected in updated_playable_tracks.items(): + actor = actors[actor_name] + expected_tracks = [tracks[i] for i in expected] + assert list(models.Track.objects.playable_by(actor)) == expected_tracks diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 8eae4a3be..fbbb0197a 100644 --- a/api/tests/music/test_tasks.py +++ b/api/tests/music/test_tasks.py @@ -316,6 +316,9 @@ def test_upload_import(now, factories, temp_signal, mocker): upload = factories["music.Upload"]( track=None, import_metadata={"funkwhale": {"track": {"uuid": str(track.uuid)}}} ) + create_entries = mocker.patch( + "funkwhale_api.music.models.TrackActor.create_entries" + ) with temp_signal(signals.upload_import_status_updated) as handler: tasks.process_upload(upload_id=upload.pk) @@ -343,6 +346,11 @@ def test_upload_import(now, factories, temp_signal, mocker): outbox.assert_called_once_with( {"type": "Create", "object": {"type": "Audio"}}, context={"upload": upload} ) + create_entries.assert_called_once_with( + library=upload.library, + delete_existing=False, + upload_and_track_ids=[(upload.pk, upload.track_id)], + ) def test_upload_import_get_audio_data(factories, mocker): diff --git a/changes/changelog.d/denormalization.enhancement b/changes/changelog.d/denormalization.enhancement new file mode 100644 index 000000000..57b750a4f --- /dev/null +++ b/changes/changelog.d/denormalization.enhancement @@ -0,0 +1 @@ +Denormalized audio permission logic in a separate table to enhance performance diff --git a/changes/notes.rst b/changes/notes.rst index 96ac3d765..6a37478e2 100644 --- a/changes/notes.rst +++ b/changes/notes.rst @@ -5,3 +5,20 @@ Next release notes Those release notes refer to the current development branch and are reset after each release. + +Denormalized audio permission logic in a separate table to enhance performance +------------------------------------------------------------------------------ + +With this release, we're introducing a performance enhancement that should drastically reduce the load on the database and API +servers (cf https://dev.funkwhale.audio/funkwhale/funkwhale/merge_requests/939). + +Under the hood, we now maintain a separate table to link users to the tracks they are allowed to see. This change is **disabled** +by default, but will be enabled by default starting in Funkwhale 0.21. + +If you want to try it now, add +``MUSIC_USE_DENORMALIZATION=True`` to your ``.env`` file, restart Funkwhale, and run the following command:: + + python manage.py rebuild_music_permissions + +This shouldn't cause any regression, but we'd appreciate if you could test this before the 0.21 release and report any unusual +behaviour regarding tracks, albums and artists visibility. From 6b5cb9759a469708e45ad59ed4ec2ef31253875c Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 20 Oct 2019 19:08:18 +0200 Subject: [PATCH 2/6] Improved SQL generated by ORM --- api/funkwhale_api/music/models.py | 4 ++-- api/funkwhale_api/music/views.py | 25 ++++++++++++++----------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 75126eda2..ba29a066b 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -284,7 +284,7 @@ class AlbumQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet): class Album(APIModelMixin): title = models.CharField(max_length=MAX_LENGTHS["ALBUM_TITLE"]) artist = models.ForeignKey(Artist, related_name="albums", on_delete=models.CASCADE) - release_date = models.DateField(null=True, blank=True) + release_date = models.DateField(null=True, blank=True, db_index=True) release_group_id = models.UUIDField(null=True, blank=True) cover = VersatileImageField( upload_to="albums/covers/%Y/%m/%d", null=True, blank=True @@ -415,7 +415,7 @@ def import_album(v): class TrackQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet): def for_nested_serialization(self): - return self.select_related().select_related("album__artist", "artist") + return self.prefetch_related("artist", "album__artist") def annotate_playable_by_actor(self, actor): diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py index 9de785116..580ac28b7 100644 --- a/api/funkwhale_api/music/views.py +++ b/api/funkwhale_api/music/views.py @@ -50,7 +50,7 @@ def get_libraries(filter_uploads): qs = models.Library.objects.filter( pk__in=uploads.values_list("library", flat=True) ).annotate(_uploads_count=Count("uploads")) - qs = qs.select_related("actor") + qs = qs.prefetch_related("actor") page = self.paginate_queryset(qs) if page is not None: serializer = federation_api_serializers.LibrarySerializer(page, many=True) @@ -104,6 +104,7 @@ class ArtistViewSet(common_views.SkipFilterForGetObject, viewsets.ReadOnlyModelV to_attr="_prefetched_tracks", ) ) + .order_by("-id") ) serializer_class = serializers.ArtistWithAlbumsSerializer permission_classes = [oauth_permissions.ScopePermission] @@ -147,8 +148,8 @@ class ArtistViewSet(common_views.SkipFilterForGetObject, viewsets.ReadOnlyModelV class AlbumViewSet(common_views.SkipFilterForGetObject, viewsets.ReadOnlyModelViewSet): queryset = ( models.Album.objects.all() - .order_by("artist", "release_date") - .select_related("artist", "attributed_to") + .order_by("-creation_date") + .prefetch_related("artist", "attributed_to") ) serializer_class = serializers.AlbumSerializer permission_classes = [oauth_permissions.ScopePermission] @@ -173,7 +174,7 @@ class AlbumViewSet(common_views.SkipFilterForGetObject, viewsets.ReadOnlyModelVi def get_queryset(self): queryset = super().get_queryset() tracks = ( - models.Track.objects.select_related("artist") + models.Track.objects.prefetch_related("artist") .with_playable_uploads(utils.get_actor_from_request(self.request)) .order_for_album() ) @@ -235,7 +236,7 @@ class LibraryViewSet( library = self.get_object() queryset = ( library.received_follows.filter(target__actor=self.request.user.actor) - .select_related("actor", "target__actor") + .prefetch_related("actor", "target__actor") .order_by("-creation_date") ) page = self.paginate_queryset(queryset) @@ -257,7 +258,8 @@ class TrackViewSet(common_views.SkipFilterForGetObject, viewsets.ReadOnlyModelVi queryset = ( models.Track.objects.all() .for_nested_serialization() - .select_related("attributed_to") + .prefetch_related("attributed_to") + .order_by("-creation_date") ) serializer_class = serializers.TrackSerializer permission_classes = [oauth_permissions.ScopePermission] @@ -454,7 +456,9 @@ class ListenViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet): def retrieve(self, request, *args, **kwargs): track = self.get_object() actor = utils.get_actor_from_request(request) - queryset = track.uploads.select_related("track__album__artist", "track__artist") + queryset = track.uploads.prefetch_related( + "track__album__artist", "track__artist" + ) explicit_file = request.GET.get("upload") if explicit_file: queryset = queryset.filter(uuid=explicit_file) @@ -493,7 +497,7 @@ class UploadViewSet( queryset = ( models.Upload.objects.all() .order_by("-creation_date") - .select_related("library", "track__artist", "track__album__artist") + .prefetch_related("library", "track__artist", "track__album__artist") ) serializer_class = serializers.UploadForOwnerSerializer permission_classes = [ @@ -577,7 +581,7 @@ class Search(views.APIView): qs = ( models.Track.objects.all() .filter(query_obj) - .select_related("artist", "album__artist") + .prefetch_related("artist", "album__artist") ) return common_utils.order_for_search(qs, "title")[: self.max_results] @@ -587,8 +591,7 @@ class Search(views.APIView): qs = ( models.Album.objects.all() .filter(query_obj) - .select_related() - .prefetch_related("tracks__artist") + .prefetch_related("tracks__artist", "artist", "attributed_to") ) return common_utils.order_for_search(qs, "title")[: self.max_results] From d1fcea563296b26126aa254bb814f15edc0097c8 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 Oct 2019 11:01:50 +0200 Subject: [PATCH 3/6] Added load testing test case --- api/tests/loadtesting/library.py | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 api/tests/loadtesting/library.py diff --git a/api/tests/loadtesting/library.py b/api/tests/loadtesting/library.py new file mode 100644 index 000000000..6413a161f --- /dev/null +++ b/api/tests/loadtesting/library.py @@ -0,0 +1,38 @@ +import os +import urllib.parse + +from locust import HttpLocust, TaskSet, task + +JWT_TOKEN = os.environ.get("JWT_TOKEN") + +DATA = {"playable": True} +HEADERS = {} +if JWT_TOKEN: + print("Starting authenticated session") + HEADERS["authorization"] = "JWT {}".format(JWT_TOKEN) + + +class WebsiteTasks(TaskSet): + @task + def albums(self): + self.client.get( + "/api/v1/albums?" + urllib.parse.urlencode(DATA), headers=HEADERS + ) + + @task + def artists(self): + self.client.get( + "/api/v1/artists?" + urllib.parse.urlencode(DATA), headers=HEADERS + ) + + @task + def tracks(self): + self.client.get( + "/api/v1/tracks?" + urllib.parse.urlencode(DATA), headers=HEADERS + ) + + +class WebsiteUser(HttpLocust): + task_set = WebsiteTasks + min_wait = 1000 + max_wait = 3000 From 61976319606d44d9561202b3aaa19ffe4128ea01 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 Oct 2019 23:02:46 +0200 Subject: [PATCH 4/6] Make postgres configurable via env vars in dev --- dev.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev.yml b/dev.yml index eaa7ca8d3..4d781ed9f 100644 --- a/dev.yml +++ b/dev.yml @@ -23,7 +23,7 @@ services: - .env.dev - .env image: postgres:${POSTGRES_VERSION-11} - command: postgres -c log_min_duration_statement=0 + command: postgres ${POSTGRES_ARGS-} volumes: - "./data/${COMPOSE_PROJECT_NAME-node1}/postgres:/var/lib/postgresql/data" networks: From 44ccf6ae6b3a34530c7f7a9efe49cd1c4a58cbcc Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 Oct 2019 23:52:06 +0200 Subject: [PATCH 5/6] Ensure play button only include playable tracks in queue --- front/src/components/audio/PlayButton.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/front/src/components/audio/PlayButton.vue b/front/src/components/audio/PlayButton.vue index 96e210e7d..77fffddd3 100644 --- a/front/src/components/audio/PlayButton.vue +++ b/front/src/components/audio/PlayButton.vue @@ -149,6 +149,7 @@ export default { params['page_size'] = 100 params['page'] = page params['hidden'] = '' + params['playable'] = 'true' tracks = tracks || [] axios.get('tracks/', {params: params}).then((response) => { response.data.results.forEach(t => { From 69795b5ca2d8a1ad4b37a4cc334bc3a6ea7e8244 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 28 Oct 2019 08:58:32 +0100 Subject: [PATCH 6/6] Resolve "Pagination of results in genres in Subsonic API does not work" --- api/funkwhale_api/subsonic/views.py | 8 +++++++- api/tests/subsonic/test_views.py | 17 +++++++++++++++++ changes/changelog.d/954.bugfix | 1 + 3 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 changes/changelog.d/954.bugfix diff --git a/api/funkwhale_api/subsonic/views.py b/api/funkwhale_api/subsonic/views.py index 60209a92d..db2620100 100644 --- a/api/funkwhale_api/subsonic/views.py +++ b/api/funkwhale_api/subsonic/views.py @@ -351,6 +351,12 @@ class SubsonicViewSet(viewsets.GenericViewSet): ) ) queryset = queryset.playable_by(actor) + try: + offset = int(data.get("offset", 0)) + except (TypeError, ValueError): + + offset = 0 + try: size = int( data["count"] @@ -369,7 +375,7 @@ class SubsonicViewSet(viewsets.GenericViewSet): ) .prefetch_related("uploads") .distinct() - .order_by("-creation_date")[:size] + .order_by("-creation_date")[offset : offset + size] ) data = { "songsByGenre": { diff --git a/api/tests/subsonic/test_views.py b/api/tests/subsonic/test_views.py index d58cc3932..f56cfc1a9 100644 --- a/api/tests/subsonic/test_views.py +++ b/api/tests/subsonic/test_views.py @@ -520,6 +520,23 @@ def test_get_songs_by_genre(f, tags_field, db, logged_in_api_client, factories): assert response.data == expected +def test_get_songs_by_genre_offset(logged_in_api_client, factories): + url = reverse("api:subsonic-get_songs_by_genre") + assert url.endswith("getSongsByGenre") is True + track1 = factories["music.Track"](playable=True, set_tags=["Rock"]) + factories["music.Track"](playable=True, set_tags=["Rock"]) + factories["music.Track"](playable=True, set_tags=["Pop"]) + # the API order tracks by creation date DESC, so the second one + # returned by the API is the first one to be created in the test. + expected = {"songsByGenre": {"song": serializers.get_song_list_data([track1])}} + + response = logged_in_api_client.get( + url, {"f": "json", "count": 1, "offset": 1, "genre": "rock"} + ) + assert response.status_code == 200 + assert response.data == expected + + @pytest.mark.parametrize("f", ["json"]) def test_search3(f, db, logged_in_api_client, factories): url = reverse("api:subsonic-search3") diff --git a/changes/changelog.d/954.bugfix b/changes/changelog.d/954.bugfix new file mode 100644 index 000000000..eacb9603a --- /dev/null +++ b/changes/changelog.d/954.bugfix @@ -0,0 +1 @@ +Fixed pagination in subsonic getSongsByGenre endpoint (#954)