diff --git a/api/funkwhale_api/federation/activity.py b/api/funkwhale_api/federation/activity.py index a56c4c41c..361b136b3 100644 --- a/api/funkwhale_api/federation/activity.py +++ b/api/funkwhale_api/federation/activity.py @@ -346,6 +346,7 @@ def activity_pass_object_privacy_level(context, routing): if object_type in MUSIC_OBJECT_TYPE: return True + # to do : support sending activity to followers only if object and obj_privacy_level and obj_privacy_level in ["me", "instance"]: return False diff --git a/api/funkwhale_api/federation/routes.py b/api/funkwhale_api/federation/routes.py index b41da63d9..91970f3b2 100644 --- a/api/funkwhale_api/federation/routes.py +++ b/api/funkwhale_api/federation/routes.py @@ -814,11 +814,12 @@ def inbox_update_playlist(payload, context): serializer = serializers.PlaylistSerializer(data=payload["object"]) if serializer.is_valid(raise_exception=True): playlist = serializer.save() - # we trigger a scan since we use this activity to avoid sending many PlaylistTracks activities - playlist.schedule_scan(actors.get_service_actor(), force=True) - # we update the playlist.library + # we update the playlist.library to get the plt.track.uploads locally if follows := playlist.library.received_follows.filter(approved=True): playlist.library.schedule_scan(follows[0].actor, force=True) + # we trigger a scan since we use this activity to avoid sending many PlaylistTracks activities + playlist.schedule_scan(actors.get_service_actor(), force=True) + return else: logger.debug( diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index a2abb227d..c9e146ed3 100644 --- a/api/funkwhale_api/federation/serializers.py +++ b/api/funkwhale_api/federation/serializers.py @@ -1133,7 +1133,12 @@ class CollectionPageSerializer(jsonld.JsonLdSerializer): "last": last, "items": [ conf["item_serializer"]( - i, context={"actor": conf["actor"], "include_ap_context": False} + i, + context={ + "actor": conf["actor"], + "library": conf.get("library", None), + "include_ap_context": False, + }, ).data for i in page.object_list ], @@ -1674,7 +1679,8 @@ class UploadSerializer(jsonld.JsonLdSerializer): def validate_library(self, v): lb = self.context.get("library") if lb: - if lb.fid != v and not lb.playlist: + # the upload can come from a playlist lib + if lb.fid != v and not lb.playlist.library and lb.playlist.library.fid != v: raise serializers.ValidationError("Invalid library fid") return lb @@ -1741,11 +1747,12 @@ class UploadSerializer(jsonld.JsonLdSerializer): return music_models.Upload.objects.create(**data) def to_representation(self, instance): + lib = instance.library if instance.library else self.context.get("library") track = instance.track d = { "type": "Audio", "id": instance.get_federation_id(), - "library": instance.library.fid, + "library": lib.fid, "name": track.full_name, "published": instance.creation_date.isoformat(), "bitrate": instance.bitrate, @@ -1764,12 +1771,8 @@ class UploadSerializer(jsonld.JsonLdSerializer): }, ], "track": TrackSerializer(track, context={"include_ap_context": False}).data, - "to": ( - contexts.AS.Public - if instance.library.privacy_level == "everyone" - else "" - ), - "attributedTo": instance.library.actor.fid, + "to": (contexts.AS.Public if lib.privacy_level == "everyone" else ""), + "attributedTo": lib.actor.fid, } if instance.modification_date: d["updated"] = instance.modification_date.isoformat() @@ -2350,7 +2353,6 @@ class PlaylistTrackSerializer(jsonld.JsonLdSerializer): "fid": validated_data["id"], }, ) - return plt diff --git a/api/funkwhale_api/federation/views.py b/api/funkwhale_api/federation/views.py index eadf870a1..7c75e3594 100644 --- a/api/funkwhale_api/federation/views.py +++ b/api/funkwhale_api/federation/views.py @@ -416,8 +416,8 @@ class MusicLibraryViewSet( ) ), "item_serializer": serializers.UploadSerializer, + "library": lb, } - return get_collection_response( conf=conf, querystring=request.GET, diff --git a/api/funkwhale_api/music/admin.py b/api/funkwhale_api/music/admin.py index 8fc334754..81535d273 100644 --- a/api/funkwhale_api/music/admin.py +++ b/api/funkwhale_api/music/admin.py @@ -1,5 +1,4 @@ from funkwhale_api.common import admin -from funkwhale_api.playlists import models as playlist_models from . import models @@ -85,9 +84,9 @@ class UploadAdmin(admin.ModelAdmin): def formfield_for_manytomany(self, db_field, request, **kwargs): if db_field.name == "playlist_libraries": object_id = request.resolver_match.kwargs.get("object_id") - kwargs["queryset"] = playlist_models.Playlist.objects.filter( - playlist_tracks__track__uploads=object_id - ) + kwargs["queryset"] = models.Library.objects.filter( + playlist_uploads=object_id + ).distinct() return super().formfield_for_foreignkey(db_field, request, **kwargs) diff --git a/api/funkwhale_api/music/migrations/0061_migrate_libraries_to_playlist.py b/api/funkwhale_api/music/migrations/0061_migrate_libraries_to_playlist.py index 756938c18..fa40e1ee2 100644 --- a/api/funkwhale_api/music/migrations/0061_migrate_libraries_to_playlist.py +++ b/api/funkwhale_api/music/migrations/0061_migrate_libraries_to_playlist.py @@ -42,6 +42,7 @@ def migrate_libraries_to_playlist(apps, schema_editor): or library.name.startswith("playlist_") or Playlist.objects.filter(library=library).exists() or not federation_utils.is_local(library.fid) + or library.name in ["me", "instance", "everyone"] ): continue @@ -74,7 +75,7 @@ def migrate_libraries_to_playlist(apps, schema_editor): # migrate uploads to new built-in libraries for actor in Actor.objects.all(): - if not federation_utils.is_local(actor.fid): + if not federation_utils.is_local(actor.fid) or actor.name == "service": continue privacy_levels = ["me", "instance", "everyone"] diff --git a/api/funkwhale_api/playlists/migrations/0009_playlist_library.py b/api/funkwhale_api/playlists/migrations/0009_playlist_library.py index dc724e36b..02c7ab410 100644 --- a/api/funkwhale_api/playlists/migrations/0009_playlist_library.py +++ b/api/funkwhale_api/playlists/migrations/0009_playlist_library.py @@ -1,6 +1,8 @@ import django.db.models.deletion from django.db import migrations, models, transaction from funkwhale_api.federation import utils as federation_utils +from django.urls import reverse +import uuid # to do : test migration @@ -26,6 +28,13 @@ def create_playlist_libraries(apps, schema_editor): name="playlist_" + playlist.name, privacy_level="me", actor=playlist.actor, + uuid=(new_uuid := uuid.uuid4()), + fid=federation_utils.full_url( + reverse( + "federation:music:playlist-tracks-detail", + kwargs={"uuid": new_uuid}, + ) + ), ) library.save() playlist.library = library diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index e93957780..0e7ff6667 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -53,9 +53,12 @@ class PlaylistSerializer(serializers.ModelSerializer): ) read_only_fields = ["id", "modification_date", "creation_date"] - @extend_schema_field(OpenApiTypes.UUID) + @extend_schema_field(OpenApiTypes.URI) def get_library(self, obj): - return obj.library.fid + if obj.library: + return obj.library.fid + else: + return None @extend_schema_field(OpenApiTypes.BOOL) def get_is_playable(self, obj): diff --git a/api/funkwhale_api/playlists/tasks.py b/api/funkwhale_api/playlists/tasks.py index 24a7b2b5d..09d7a5212 100644 --- a/api/funkwhale_api/playlists/tasks.py +++ b/api/funkwhale_api/playlists/tasks.py @@ -55,6 +55,7 @@ def get_playlist_page(playlist, page_url, actor): context={ "playlist": playlist, "item_serializer": serializers.PlaylistTrackSerializer, + "conf": {"library": playlist.library}, }, ) serializer.is_valid(raise_exception=True) @@ -67,6 +68,8 @@ def get_playlist_page(playlist, page_url, actor): "playlist_scan", ) def start_playlist_scan(playlist_scan): + playlist_scan.playlist.playlist_tracks.all().delete() + try: data = get_playlist_data(playlist_scan.playlist.fid, actor=playlist_scan.actor) except Exception: @@ -97,18 +100,30 @@ def start_playlist_scan(playlist_scan): ) def scan_playlist_page(playlist_scan, page_url): data = get_playlist_page(playlist_scan.playlist, page_url, playlist_scan.actor) - tracks = [] + plts = [] for item_serializer in data["items"]: try: - track = item_serializer.save(playlist=playlist_scan.playlist.fid) - tracks.append(track) + plt = item_serializer.save(playlist=playlist_scan.playlist.fid) + # we get any upload owned by the playlist.actor and add a m2m with playlist_libraries + upload_qs = plt.track.uploads.filter( + library__actor=playlist_scan.playlist.actor + ) + if not upload_qs: + logger.debug( + f"Could not find a upload for the playlist track {plt.track.title}. Probably the \ + playlist.library library_scan failed or was not launched by inbox_update_playlist ?" + ) + else: + upload_qs[0].playlist_libraries.add(playlist_scan.playlist.library) + logger.debug(f"Added {plt.track.title} to playlist library") + plts.append(plt) except Exception as e: logger.info( f"Error while saving track to playlist {playlist_scan.playlist}: {e}" ) continue - playlist_scan.processed_files = F("processed_files") + len(tracks) + playlist_scan.processed_files = F("processed_files") + len(plts) playlist_scan.modification_date = timezone.now() update_fields = ["modification_date", "processed_files"] diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index 19a2d4e90..39f07d17f 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -170,7 +170,7 @@ class PlaylistViewSet( playlist.playlist_tracks.all().delete() playlist.save(update_fields=["modification_date"]) playlist.library.uploads.filter().delete() - playlist.schedule_scan(playlist.actor) + playlist.schedule_scan(playlist.actor, force=True) return Response(status=204) def get_queryset(self): diff --git a/api/tests/playlists/test_tasks.py b/api/tests/playlists/test_tasks.py index 4af75c766..1d43e6e0b 100644 --- a/api/tests/playlists/test_tasks.py +++ b/api/tests/playlists/test_tasks.py @@ -17,6 +17,8 @@ def test_scan_playlist_page_fetches_page_and_creates_tracks( for i in range(5) ] + for plt in tracks: + factories["music.Upload"](track=plt.track, library__actor=scan.playlist.actor) page_conf = { "actor": scan.playlist.actor, "id": scan.playlist.fid, @@ -35,7 +37,8 @@ def test_scan_playlist_page_fetches_page_and_creates_tracks( assert len(plts) == 3 for track in tracks[:3]: - scan.playlist.playlist_tracks.get(fid=track.fid) + plt = scan.playlist.playlist_tracks.get(fid=track.fid) + scan.playlist.library in plt.track.uploads.all()[0].playlist_libraries.all() assert scan.status == "scanning" assert scan.processed_files == 3 diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py index de314b494..4237fe37c 100644 --- a/api/tests/playlists/test_views.py +++ b/api/tests/playlists/test_views.py @@ -197,8 +197,15 @@ def test_add_multiple_tracks_at_once_update_pl_library( for track in tracks: factories["music.Upload"](track=track, library__actor=actor) + track_already_in_playlist = factories["music.Track"]() + upload_already_in_playlist = factories["music.Upload"]( + track=track_already_in_playlist, library__actor=actor + ) + upload_already_in_playlist.playlist_libraries.add(playlist.library) + track_ids = [t.id for t in tracks] track_ids.append(not_user_track.id) + track_ids.append(track_already_in_playlist.id) mocker.spy(playlist, "insert_many") url = reverse("api:v1:playlists-add", kwargs={"pk": playlist.pk}) response = logged_in_api_client.post(url, {"tracks": track_ids}) @@ -209,7 +216,8 @@ def test_add_multiple_tracks_at_once_update_pl_library( assert not_user_upload not in playlist.library.uploads.all() for plt in playlist.playlist_tracks.all(): for upload in playlist.library.uploads.all(): - upload.tracks.filter(id=plt.track.id).exists() + assert upload.tracks.filter(id=plt.track.id).exists() + assert len(upload.playlist_libraries.all()) == 1 def test_honor_max_playlist_size(factories, mocker, logged_in_api_client, preferences): diff --git a/docs/specs/playlist-library-federation/index.md b/docs/specs/playlist-library-federation/index.md index 33bc736d8..0a366a492 100644 --- a/docs/specs/playlist-library-federation/index.md +++ b/docs/specs/playlist-library-federation/index.md @@ -19,7 +19,13 @@ Users will be able to click on a "Request access to playlist audios files" butto `Playlist` one_to_one with `Library` through `library` field `Upload` many_to_one with `Library` through `library` (reverse is `library.uploads`) -`Upload` has also a many_to_many with `Library` through `playlist_libraries` (the same upload can be share various time in through various playlists). Reverse relation is `library.playlist_uploads` +`Upload` has also a many_to_many with `Library` through `playlist_libraries` (the same upload can be share various time through various playlists). Reverse relation is `library.playlist_uploads` + +We could migrate from O2M to M2M, but this is super complicated since : - it adds a lot of extra logic (you can't query the m2m if the instance is not save -> this generated problem to validate incoming AP objects) - having a built-in lib and playlist libs make verifications easier (only three built-in lib, playlist_lib are always private) + +##### Workflow + +Playlist activity -> library_scan(get the uploads) -> playlist_scan (set the upload.playlist_relation and create plts) ##### Federation @@ -41,7 +47,12 @@ There is no other reason to share the playlit.library to remote. - [x] make sure only owned upload are added to the playlist.library - [x] update the "drop library" migrations to use the playlist.library instead of user follow - [ ] make sure user get the new libraries created after library drop -- [ ] update the federation api to send the playlist_library info +- [x] update the federation api : when we receive a fetch for a library the upload serializer need to know which lib (playlist lib or user lib) +- [x] Support library.playlist_uploads in library scan -> add playlist_uploads in items in library federation viewset +- [x] investigate library scan bug : don't delete old content of the lib (local cache?): we need to empty the playlist before the scan(not ideal but less work) +- [ ] if the user change the upload to another built-in lib, make sure the upload is not delete (we would loose the playlist_library relation) but only updated. +- [ ] enforce actor.libraries to only have three entries. +- [ ] enforce upload.playlist_libraries to always be private ### Follow up @@ -49,3 +60,5 @@ There is no other reason to share the playlit.library to remote. - [ ] Finish library drop (delete libraries endpoints) - [ ] Playlist discovery : fetch federation endpoint for playlists - [ ] Playlist discovery : add the playlist to my playlist collection = follow request to playlist +- [ ] PLaylist Track activity (to avoid having to refetch the whole playlist) +- [ ] Document : The user that want to federate need to activate remote activities in it's user settings. Even if the library is public the playlist activities will not be sended to remote -> We need to implement a followers activity setting diff --git a/front/src/locales/en_US.json b/front/src/locales/en_US.json index 0249b4956..6270025f3 100644 --- a/front/src/locales/en_US.json +++ b/front/src/locales/en_US.json @@ -4241,7 +4241,7 @@ "showStatus": "Show information about the upload status for this track" }, "empty": { - "noTracks": "No tracks have been added to this libray yet" + "noTracks": "No tracks have been added to this library yet" }, "label": { "importStatus": "Import status",