Merge pull request metabrainz#2872 from metabrainz/jspf-cleanup

Jspf cleanup
Aerozol · May 29, 2024 · 106119b · 106119b
2 parents 6685d24 + d7ff5ae
commit 106119b
Show file tree

Hide file tree

Showing 10 changed files with 94 additions and 54 deletions.
diff --git a/frontend/js/src/player/PlayerPage.tsx b/frontend/js/src/player/PlayerPage.tsx
@@ -46,14 +46,6 @@ export default class PlayerPage extends React.Component<
 > {
   static contextType = GlobalAppContext;
 
-  static makeJSPFTrack(track: ACRMSearchResult): JSPFTrack {
-    return {
-      identifier: `${PLAYLIST_TRACK_URI_PREFIX}${track.recording_mbid}`,
-      title: track.recording_name,
-      creator: track.artist_credit_name,
-    };
-  }
-
   declare context: React.ContextType<typeof GlobalAppContext>;
 
   constructor(props: PlayerPageProps) {

diff --git a/frontend/js/src/playlists/Playlist.tsx b/frontend/js/src/playlists/Playlist.tsx
@@ -53,10 +53,12 @@ export default class PlaylistPage extends React.Component<
 
   static makeJSPFTrack(trackMetadata: TrackMetadata): JSPFTrack {
     return {
-      identifier: `${PLAYLIST_TRACK_URI_PREFIX}${
-        trackMetadata.recording_mbid ??
-        trackMetadata.additional_info?.recording_mbid
-      }`,
+      identifier: [
+        `${PLAYLIST_TRACK_URI_PREFIX}${
+          trackMetadata.recording_mbid ??
+          trackMetadata.additional_info?.recording_mbid
+        }`,
+      ],
       title: trackMetadata.track_name,
       creator: trackMetadata.artist_name,
     };

diff --git a/frontend/js/src/playlists/utils.tsx b/frontend/js/src/playlists/utils.tsx
@@ -46,7 +46,18 @@ export function getPlaylistId(playlist?: JSPFPlaylist): string {
 }
 
 export function getRecordingMBIDFromJSPFTrack(track: JSPFTrack): string {
-  return track.identifier?.replace(PLAYLIST_TRACK_URI_PREFIX, "") ?? "";
+  const { identifier } = track;
+  let identifiers: string[];
+  if (typeof identifier === "string") {
+    identifiers = [identifier];
+  } else {
+    identifiers = identifier;
+  }
+  return (
+    identifiers
+      ?.find((iden) => iden.startsWith(PLAYLIST_TRACK_URI_PREFIX))
+      ?.replace(PLAYLIST_TRACK_URI_PREFIX, "") ?? ""
+  );
 }
 export function getArtistMBIDFromURI(URI: string): string {
   return URI?.replace(PLAYLIST_ARTIST_URI_PREFIX, "") ?? "";

diff --git a/frontend/js/src/user/year-in-music/2021/YearInMusic2021.tsx b/frontend/js/src/user/year-in-music/2021/YearInMusic2021.tsx
@@ -144,7 +144,13 @@ export default class YearInMusic extends React.Component<
       playlist.jspf.playlist.track = playlist.jspf.playlist.track.map(
         (track: JSPFTrack) => {
           const newTrack = { ...track };
-          const track_id = track.identifier;
+          let track_id;
+          if (Array.isArray(track.identifier)) {
+            // eslint-disable-next-line prefer-destructuring
+            track_id = track.identifier[0];
+          } else {
+            track_id = track.identifier;
+          }
           const found = track_id.match(uuidMatch);
           if (found) {
             const recording_mbid = found[0];

diff --git a/frontend/js/src/user/year-in-music/2022/YearInMusic2022.tsx b/frontend/js/src/user/year-in-music/2022/YearInMusic2022.tsx
@@ -151,7 +151,13 @@ export default class YearInMusic extends React.Component<
       /* Add a track image if it exists in the `{playlistName}-coverart` key */
       playlist.track = playlist.track.map((track: JSPFTrack) => {
         const newTrack = { ...track };
-        const track_id = track.identifier;
+        let track_id;
+        if (Array.isArray(track.identifier)) {
+          // eslint-disable-next-line prefer-destructuring
+          track_id = track.identifier[0];
+        } else {
+          track_id = track.identifier;
+        }
         const found = track_id.match(uuidMatch);
         if (found) {
           const recording_mbid = found[0];

diff --git a/frontend/js/src/utils/types.d.ts b/frontend/js/src/utils/types.d.ts
@@ -549,7 +549,7 @@ declare type JSPFPlaylist = {
 declare type JSPFTrack = {
   id?: string; // React-sortable library expects an id attribute, this is not part of JSPF specification
   location?: string[];
-  identifier: string;
+  identifier: string | string[];
   title: string;
   creator: string;
   annotation?: string;

diff --git a/listenbrainz/db/model/playlist.py b/listenbrainz/db/model/playlist.py
@@ -5,7 +5,6 @@
 from pydantic import BaseModel, validator, NonNegativeInt, constr
 from data.model.validators import check_valid_uuid
 
-
 PLAYLIST_TRACK_URI_PREFIX = "https://musicbrainz.org/recording/"
 PLAYLIST_ARTIST_URI_PREFIX = "https://musicbrainz.org/artist/"
 PLAYLIST_RELEASE_URI_PREFIX = "https://musicbrainz.org/release/"
@@ -152,7 +151,7 @@ def serialize_jspf(self):
 
         tracks = []
         for rec in self.recordings:
-            tr = {"identifier": PLAYLIST_TRACK_URI_PREFIX + str(rec.mbid)}
+            tr = {"identifier": [PLAYLIST_TRACK_URI_PREFIX + str(rec.mbid)]}
             if rec.artist_credit:
                 tr["creator"] = rec.artist_credit
 
@@ -165,8 +164,7 @@ def serialize_jspf(self):
             if rec.duration_ms:
                 tr["duration"] = rec.duration_ms
 
-            extension = {"added_by": rec.added_by,
-                         "added_at": rec.created.astimezone(datetime.timezone.utc).isoformat()}
+            extension = {"added_by": rec.added_by, "added_at": rec.created.astimezone(datetime.timezone.utc).isoformat()}
             if rec.artist_mbids:
                 extension["artist_identifiers"] = [PLAYLIST_ARTIST_URI_PREFIX + str(mbid) for mbid in rec.artist_mbids]
 

diff --git a/listenbrainz/tests/integration/test_playlist_api.py b/listenbrainz/tests/integration/test_playlist_api.py
@@ -31,7 +31,7 @@ def get_test_data():
             },
             "track": [
                 {
-                    "identifier": "https://musicbrainz.org/recording/e8f9b188-f819-4e43-ab0f-4bd26ce9ff56"
+                    "identifier": ["https://musicbrainz.org/recording/e8f9b188-f819-4e43-ab0f-4bd26ce9ff56"]
                 }
             ],
         }
@@ -115,7 +115,7 @@ def test_playlist_create_with_created_for(self):
 
         # Submit a playlist on a different user's behalf
         playlist = get_test_data()
-        playlist["playlist"]["created_for"] = self.user["musicbrainz_id"]
+        playlist["playlist"]["extension"][PLAYLIST_EXTENSION_URI]["created_for"] = self.user["musicbrainz_id"]
 
         response = self.client.post(
             self.custom_url_for("playlist_api_v1.create_playlist"),
@@ -362,8 +362,11 @@ def test_playlist_json_with_missing_keys(self):
             headers={"Authorization": "Token {}".format(self.user["auth_token"])}
         )
         self.assert400(response)
-        self.assertEqual(response.json["error"],
-                         "JSPF playlist track 0 identifier must have the namespace 'https://musicbrainz.org/recording/' prepended to it.")
+        self.assertEqual(
+            response.json["error"],
+            "JSPF playlist track 0 must contain a identifier field having a fully qualified URI to a"
+            " recording_mbid. (e.g. https://musicbrainz.org/recording/8f3471b5-7e6a-48da-86a9-c1c07a0f47ae)"
+        )
 
     def test_playlist_create_with_collaborators(self):
         """ Test to ensure creating a playlist with collaborators works """
@@ -487,7 +490,7 @@ def test_playlist_recording_add(self):
             "playlist": {
                 "track": [
                     {
-                        "identifier": PLAYLIST_TRACK_URI_PREFIX + "4a77a078-e91a-4522-a409-3b58aa7de3ae"
+                        "identifier": [PLAYLIST_TRACK_URI_PREFIX + "4a77a078-e91a-4522-a409-3b58aa7de3ae"]
                     }
                 ],
                 "extension": {
@@ -539,10 +542,10 @@ def test_playlist_recording_move(self):
                 "title": "1980s flashback jams",
                 "track": [
                     {
-                        "identifier": PLAYLIST_TRACK_URI_PREFIX + "e8f9b188-f819-4e43-ab0f-4bd26ce9ff56"
+                        "identifier": [PLAYLIST_TRACK_URI_PREFIX + "e8f9b188-f819-4e43-ab0f-4bd26ce9ff56"]
                     },
                     {
-                        "identifier": PLAYLIST_TRACK_URI_PREFIX + "57ef4803-5181-4b3d-8dd6-8b9d9ca83e2a"
+                        "identifier": [PLAYLIST_TRACK_URI_PREFIX + "57ef4803-5181-4b3d-8dd6-8b9d9ca83e2a"]
                     }
                 ],
                 "extension": {
@@ -586,10 +589,10 @@ def test_playlist_recording_delete(self):
                 "title": "1980s flashback jams",
                 "track": [
                     {
-                        "identifier": PLAYLIST_TRACK_URI_PREFIX + "e8f9b188-f819-4e43-ab0f-4bd26ce9ff56"
+                        "identifier": [PLAYLIST_TRACK_URI_PREFIX + "e8f9b188-f819-4e43-ab0f-4bd26ce9ff56"]
                     },
                     {
-                        "identifier": PLAYLIST_TRACK_URI_PREFIX + "57ef4803-5181-4b3d-8dd6-8b9d9ca83e2a"
+                        "identifier": [PLAYLIST_TRACK_URI_PREFIX + "57ef4803-5181-4b3d-8dd6-8b9d9ca83e2a"]
                     }
                 ],
                 "extension": {

diff --git a/listenbrainz/webserver/views/playlist_api.py b/listenbrainz/webserver/views/playlist_api.py
@@ -44,6 +44,22 @@ def validate_create_playlist_required_items(jspf):
         log_raise_400("JSPF playlist.extension.https://musicbrainz.org/doc/jspf#playlist.public field must be given.")
 
 
+def get_track_recording_mbid(track):
+    recording_uris = track.get("identifier", [])
+    # This allows identifier to be a list, tuple or string. The string support is a leftover and should
+    # be removed after 2025-06, which marks a year or backward compatibility.
+    if isinstance(recording_uris, str):
+        recording_uris = [recording_uris]
+
+    for recording_uri in recording_uris:
+        if recording_uri.startswith(PLAYLIST_TRACK_URI_PREFIX):
+            recording_mbid = recording_uri[len(PLAYLIST_TRACK_URI_PREFIX):]
+            if is_valid_uuid(recording_mbid):
+                return recording_mbid
+
+    return None
+
+
 def validate_playlist(jspf):
     """
         Given a JSPF dict, ensure that title is present and that if tracks are present
@@ -74,19 +90,15 @@ def validate_playlist(jspf):
     if "track" not in jspf["playlist"]:
         return
 
-    for i, track in enumerate(jspf["playlist"].get("track", [])):
-        recording_uri = track.get("identifier")
-        if not recording_uri:
-            log_raise_400("JSPF playlist track %d must contain an identifier element with recording MBID." % i)
-
-        if recording_uri.startswith(PLAYLIST_TRACK_URI_PREFIX):
-            recording_mbid = recording_uri[len(PLAYLIST_TRACK_URI_PREFIX):]
-        else:
-            log_raise_400("JSPF playlist track %d identifier must have the namespace '%s' prepended to it." %
-                          (i, PLAYLIST_TRACK_URI_PREFIX))
+    recording_uri_error = (
+        "JSPF playlist track %d must contain a identifier field having a fully qualified URI to a recording_mbid. "
+        "(e.g. https://musicbrainz.org/recording/8f3471b5-7e6a-48da-86a9-c1c07a0f47ae)"
+    )
 
-        if not is_valid_uuid(recording_mbid):
-            log_raise_400("JSPF playlist track %d does not contain a valid track identifier field." % i)
+    for i, track in enumerate(jspf["playlist"].get("track", [])):
+        recording_mbid = get_track_recording_mbid(track)
+        if not recording_mbid:
+            log_raise_400(recording_uri_error % i)
 
 
 def serialize_xspf(playlist: Playlist):
@@ -311,9 +323,14 @@ def create_playlist():
         collaborators.remove(user["musicbrainz_id"])
 
     username_lookup = collaborators
-    created_for = data["playlist"].get("created_for", None)
+    created_for = data["playlist"]["extension"][PLAYLIST_EXTENSION_URI].get("created_for", None)
     if created_for:
         username_lookup.append(created_for)
+    # The else: clause below can be removed as of 2025-06.
+    else:
+        created_for = data["playlist"].get("created_for", None)
+        if created_for:
+            username_lookup.append(created_for)
 
     users = {}
     if username_lookup:
@@ -347,20 +364,26 @@ def create_playlist():
                                 public=public,
                                 additional_metadata=additional_metadata)
 
-    if data["playlist"].get("created_for", None):
+    if data["playlist"]["extension"][PLAYLIST_EXTENSION_URI].get("created_for", None):
         if user["musicbrainz_id"] not in current_app.config["APPROVED_PLAYLIST_BOTS"]:
             raise APIForbidden("Playlist contains a created_for field, but submitting user is not an approved playlist bot.")
-        created_for_user = users.get(data["playlist"]["created_for"].lower())
+        created_for_user = users.get(data["playlist"]["extension"][PLAYLIST_EXTENSION_URI]["created_for"].lower())
         if not created_for_user:
             log_raise_400("created_for user does not exist.")
         playlist.created_for_id = created_for_user["id"]
 
     if "track" in data["playlist"]:
         for track in data["playlist"]["track"]:
             try:
-                playlist.recordings.append(
-                    WritablePlaylistRecording(mbid=UUID(track['identifier'][len(PLAYLIST_TRACK_URI_PREFIX):]),
-                                              added_by_id=user["id"]))
+                # This if statement can be replaced with the first option (is list) as of 2025-06.
+                if isinstance(track['identifier'], list) or isinstance(track['identifier'], tuple):
+                    playlist.recordings.append(
+                        WritablePlaylistRecording(mbid=UUID(track['identifier'][0][len(PLAYLIST_TRACK_URI_PREFIX):]),
+                                                  added_by_id=user["id"]))
+                else:
+                    playlist.recordings.append(
+                        WritablePlaylistRecording(mbid=UUID(track['identifier'][len(PLAYLIST_TRACK_URI_PREFIX):]),
+                                                  added_by_id=user["id"]))
             except ValueError:
                 log_raise_400("Invalid recording MBID found in submitted recordings")
 
@@ -594,11 +617,10 @@ def add_playlist_item(playlist_mbid, offset):
     precordings = []
     if "track" in data["playlist"]:
         for track in data["playlist"]["track"]:
-            try:
-                mbid = UUID(track['identifier'][len(PLAYLIST_TRACK_URI_PREFIX):])
-            except (KeyError, ValueError):
+            recording_mbid = get_track_recording_mbid(track)
+            if not recording_mbid:
                 log_raise_400("Track %d has an invalid identifier field, it must be a complete URI.")
-            precordings.append(WritablePlaylistRecording(mbid=mbid, added_by_id=user["id"]))
+            precordings.append(WritablePlaylistRecording(mbid=UUID(recording_mbid), added_by_id=user["id"]))
 
     try:
         db_playlist.add_recordings_to_playlist(db_conn, ts_conn, playlist, precordings, offset)
@@ -857,7 +879,7 @@ def import_playlist_from_spotify(service):
         raise APIBadRequest(f"Missing scopes playlist-modify-public and playlist-modify-private to export playlists."
                             f" Please relink your {service} account from ListenBrainz settings with appropriate scopes"
                             f" to use this feature.")
-    
+
     try:
         sp = spotipy.Spotify(token["access_token"])
         playlists = sp.current_user_playlists()
@@ -897,7 +919,7 @@ def import_tracks_from_spotify_to_playlist(service, playlist_id):
         raise APIBadRequest(f"Missing scopes playlist-modify-public and playlist-modify-private to export playlists."
                             f" Please relink your {service} account from ListenBrainz settings with appropriate scopes"
                             f" to use this feature.")
-        
+
     try:
         playlist = import_from_spotify(token["access_token"], user["auth_token"], playlist_id)
         return playlist

diff --git a/requirements.txt b/requirements.txt
@@ -22,7 +22,7 @@ pika == 1.2.1
 brainzutils@git+https://github.com/metabrainz/brainzutils-python.git@upgrade-deps
 spotipy>=2.22.1
 datasethoster@git+https://github.com/metabrainz/data-set-hoster.git@830ecb2b2120acbd5deed2dab4587784c7be04b6
-troi@git+https://github.com/metabrainz/troi-recommendation-playground.git@v-2024-05-03.0
+troi@git+https://github.com/metabrainz/troi-recommendation-playground.git@v-2024-05-28.0
 PyYAML==6.0
 eventlet == 0.35.2
 # eventlet fails to patch dnspython >= 2.3 https://github.com/eventlet/eventlet/issues/781