Add album version to AlbumID3

[mgdigital]

Navidrome now supports albumversion in Vorbis comments (in the BFR PR), but this has no mapping in OpenSubsonic.

[netlify]

✅ Deploy Preview for opensubsonic ready!

Name	Link
🔨 Latest commit	3bb416f
🔍 Latest deploy log	https://app.netlify.com/sites/opensubsonic/deploys/679fcd706363b2000825f1b5
😎 Deploy Preview	https://deploy-preview-129--opensubsonic.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[Tolriq]

From a couple of discussions about this, I think we should take the time to see the big picture about album groups that is tied to that (Release group in MusicBrainz).

The idea is that if you have multiple versions of the same albums and need to use a differentiator (in this case VERSION from vorbis but there's others) then those albums should be groupable somehow with ways to navigate between them.

There's different ways to achieve that either via a version object with name and groupId to get all the others, or a version object that have the version name, the group name and the ids of the other similar.

[mgdigital]

There's different ways to achieve that either via a version object with name and groupId to get all the others, or a version object that have the version name, the group name and the ids of the other similar.

It can possibly be resolved by adding musicBrainzReleaseGroupId - it seems this could be supported independently from the version field. For now it would be nice not to shoehorn [Deluxe Edition] into the album title. After that, grouping of releases would be a further win?

[dweymouth]

Yeah I think this is forwards-compatible with adding album groupings later.

[Tolriq]

Well no it's not depending on what we want to achieve. There's not only musicBrainzReleaseGroupId to do the grouping.

So then how will you convert those later in a proper backward compatible way? It's better to think the API surface before than after, that's the whole point of OS. How will you properly navigate between the album groups with just adding an musicBrainzReleaseGroupId without having the client requesting all the albums to find the group content? How do you name the group or define the first item of the group?

So please let's take time to think and not rush things and regret later. Even if grouping is not added now, how it will be added later need to be taken in account here to avoid issues later.

Version without grouping is just moving [Deluxe] from the title to another field to have the client then do title [Deluxe]. While anticipating the actual grouping clients can properly handle things from the start.

[mgdigital]

So please let's take time to think and not rush things and regret later. Even if grouping is not added now, how it will be added later need to be taken in account here to avoid issues later.

I agree on not rushing changes that are later regretted, but I don't believe this change precludes any future solution for grouping on the backend. It would also immediately allow grouping on the client side, because album names from the same release group will become the same once they've lost the [Deluxe] suffix.

How will you properly navigate between the album groups with just adding an musicBrainzReleaseGroupId without having the client requesting all the albums to find the group content?

I guess this might be supported in future by adding a parameter on the search3 endpoint or elsewhere. Can you foresee any other issues this change creates, for supporting backend grouping in future?

You've suggested putting version within a nested object alongside a group ID. Whether nested or not seems a cosmetic decision and I'm fairly indifferent to that, but having it flattened maintains closer consistency to the current design.

Version without grouping is just moving [Deluxe] from the title to another field to have the client then do title [Deluxe]. While anticipating the actual grouping clients can properly handle things from the start.

Since [Deluxe] is not part of the canonical album title, removing it to its own field seems like a good change as it currently causes several annoyances: info and cover lookup, scrobbling etc are adversely affected by having it as a suffix on the album name.

Unfortunately once users have corrected tags on their Navidrome server, the version info will be completely lost without something to map it to in OS.

[Tolriq]

Read again the other OS additions, this is not a cosmetic decision at all ..... This is about maintaining a public API correctly by properly defining it's surface in a future proof way and not keep doing the same mistakes as the original. And yes changing a raw field from string to object would be breaking. And no there's won't be addition to search3 but new endpoints. There's a lot of history in OS and discussion for a way to handle things.

Versions and groups are highly tied. As said taking 3 minutes to think is necessary and better than rushing.

It's way more in OS way to add "group" : { "versionName": "Deluxe" } that covers the needs and still allow later definition of how we handle groups with IDs or not, than just adding a field quickly without thinking at what is related to it and how it will be used in the future.

And no clients don't merge groups via album name else good luck with Greatest hits and similar ;)

[dweymouth]

It would also immediately allow grouping on the client side, because album names from the same release group will become the same once they've lost the [Deluxe] suffix.

This actually isn't the case, since unrelated albums can have the same name (and even the same album artist too such as in the case of bands who release multiple eponymous albums). A grouping ID would be the way to go, plus an API to query by grouping ID. But, I agree with you that exposing the version should not preclude a grouping solution later. Though maybe we could do it as:

// album
{
   // ...
    "releaseGroup": {
        "version": "Deluxe Edition",
        "ID": "" // later, we add a grouping id here, and API to get all albums within a release group
      },
 }

[mgdigital]

It would also immediately allow grouping on the client side, because album names from the same release group will become the same once they've lost the [Deluxe] suffix.

This actually isn't the case, since unrelated albums can have the same name

I guess it's not perfect but clients might be able to rely on a combo of artist, album, version and original release date. I agree it's ambiguous without all 4 of these and that a backend solution is best for the future.

[dweymouth]

It's way more in OS way to add "group" : { "versionName": "Deluxe" }

I'd be happy to update the PR accordingly, what do you think @dweymouth ?

Sounds good to me, I'd suggest ReleaseGroup as the name for the new object we're creating, since it is a concept already from MusicBrainz Picard (but of course OS won't be requiring that the id come from MusicBrainz, it's our own version of the same concept).

And we can probably omit the ID until we later add the APIs to query by that

[Tolriq]

@dweymouth we don't need to add the ID definition yet, but having a the object already allows any variations later.

I do not have strong opinion about the namings.

[mgdigital]

Updated accordingly...

[Tolriq]

No need for ... ;) If version is mandatory we should mention that the main version is "" to avoid questions about that.

[mgdigital]

I've added the empty string example. I didn't call it "main" as there is sometimes not a "main" version. I have a bad habit of ending my sentences with ellipses...

[deluan]

I didn't read the whole discussion, but just looking at the proposed changes, how the client is supposed to know the release group the album belongs to? Or are we leaving it open for future improvements (maybe add an ID to the releaseGroup object?)

EDIT: Anyway, IMHO, conceptually the album version should not be an attribute of the releaseGroup, it should be an album attribute

[mgdigital]

Or are we leaving it open for future improvements (maybe add an ID to the releaseGroup object?)

Exactly, the release group will hopefully have an ID added in future.

EDIT: Anyway, IMHO, conceptually the album version should not be an attribute of the releaseGroup, it should be an album attribute

It was originally at the album top level but was put in the releaseGroup object following @Tolriq 's input.

[Tolriq]

It was all discussed :) Apparently there's an urgency to have this without the grouping, so the most future proof solution was proposed (and dweymouth proposed the same at the same time). Conceptually the version is what make the album part of a group so it kinda works.

I have no objection to put that back to album level IF we define already how the groups will work to avoid any future issue with the API surface.

Since both are fully related, either we go future proof, or we already define the rest of the API even if we don't implement it right now. At least all the related things have been exposed and we know where we'll be going.

[mgdigital]

Apparently there's an urgency to have this without the grouping

There's no urgency, happy to take the time to get this right :)

It seemed the version name could be done independently of the grouping and in a forwards compatible way.

I'd be happy to either:

1. Revert to version at the top level, like this: https://github.com/opensubsonic/open-subsonic-api/compare/main..5c50f073374c608defccbab2e4a42fe0fd3a88c4
2. Stick with the releaseGroup object and discuss how the group ID can be added as part of this PR
3. Stick with the releaseGroup object and come back later to add the group ID

[Tolriq]

seemed

That's the problem this is not the good term when publishing and maintaining a public API used by many different servers and clients. We can't rely on seem, but needs to be sure of what we do as I try to explain since the start in this PR.

I'm mostly never opposed to anything that fit OpenSubsonic as long as the API definition make sense and is thought enough to ensure stability in the future.

So again, as long as we define how the group API will be done with all that is tied to that (like the mbreleasegroupid, the open subsonic ID or not, how we publish the other members of the group, ...) and we are sure this is not problematic with whatever solution we decide for the version label itself then it's ok.

When looking at the groups from mbid, there seems to be plenty of cases where version is empty but the albums are the same. So just a version field does not seems to work as differentiator for example, or there's multiple Deluxe edition.

So IMO it's worth doing this right with proper grouping handling as it's the most useful globally for the users. We should also see how to handle those for the other tag formats and the relation with the mbreleasegroupID

[deluan]

I may not be seeing other future impediments, but as I see it, we just need two new attributes:

• albumVersion - A optional label to be added to the album name, for display purposes
• releaseGroup - An opaque ID provided by the server, to allow grouping albums by the client (client can group all albums that have the same value).

I don't see a reason why releaseGroup needs to be tied to mbreleasegroupid. It can be anything defined by the server, and it should be an opaque value for the client.

Likewise, I don't see a problem with albums having the same name, as long as they are identified by a different ID. The albumVersion is a "nice to have", but does not block the server or the client to have different versions of the same album. This is already achieved by having different IDs for each version.

[Tolriq]

Release group are tied to the mbids even if not mandatory when present they are the first natural way to split he groups..

For the rest as explained earlier, one of the possibility was that the group object directly returned the album IDs of the other albums of the group, to allow direct grouping and navigation and everything easily without the need for new endpoints and without the need to expose groupie that stays internal to the server.

So there's many way la to tackle this hence the long discussion that you skipped and make us write again ;)

[deluan]

Release group are tied to the mbids even if not mandatory when present they are the first natural way to split he groups..

Yes, but not all albums are in MB database, and lots of users use Discogs as their source of truth. And some users may want to group it themselves for artists/albums that are not represented in any online DB

For the rest as explained earlier, one of the possibility was that the group object directly returned the album IDs of the other albums of the group, to allow direct grouping and navigation and everything easily without the need for new endpoints and without the need to expose groupie that stays internal to the server.

I still think it is simpler if we just expose an opaque release group ID from the server, and the client can simply group albums from a given artist by this ID. A simple logic without increasing the already large payloads. But this is just my 2cents, any solution works for me, I'm not writing a OS client :P

So there's many way la to tackle this hence the long discussion that you skipped and make us write again ;)

This is good for all, so you can reevaluate if your opinions make sense :P

[mgdigital]

I don't see a reason why releaseGroup needs to be tied to mbreleasegroupid. It can be anything defined by the server, and it should be an opaque value for the client.

I can see the benefit of musicBrainzReleaseGroupId (perhaps alongside an opaque releaseGroupId) as it could permit enrichment from other data sources using the MB IDs. Either could then be used for disambiguation.

one of the possibility was that the group object directly returned the album IDs of the other albums of the group

I like that possibility, so:

{
  "releaseGroup": {
     "version": "Deluxe Edition (or just an empty string)",
     "releaseGroupId": "maybe an opaque ID",
     "musicBrainzReleaseGroupId": "maybe a mbrealeasegroupid",
     "groupAlbumIds": ["123", "456"]
  }
}

A possible problem with groupAlbumIds is it places extra burden on the server? Maybe it is optional and groups can also be pulled by passing a param to search3?

[Tolriq]

Yes, but not all albums are in MB database, and lots of users use Discogs as their source of truth. And some users may want to group it themselves for artists/albums that are not represented in any online DB

This is irrelevant to the connection between both. Not all albums have mbids, but all albums that have the mbreleasegroupId are part of same group and expected to be.

So a group can and should possibly have an mbreleasegroupid when present so that the users can use it to query the MB database for more details.

I still think it is simpler if we just expose an opaque release group ID from the server, and the client can simply group albums from a given artist by this ID

You are still thinking pre multiple artists and track artists and everything new your BFR ;) If you expose just an opaqueID you need an API call to be able to get the other albums from that ID. As you could be browsing an album, or an artist that is just contributor to some albums, and many other cases.

A possible problem with groupAlbumIds is it places extra burden on the server? Maybe it is optional and groups can also be pulled by passing a param to search3?

It was discussed a lot, but adding new params to search3 does not really makes sense, there's tons of needs there including sorts and everything and it's complicated to manage that as extensions.

So if the data is not returned with the album there's needs for another endpoint to query them. Not all clients will query the whole album list to rebuild the data when needed.

[deluan]

If you expose just an opaqueID you need an API call to be able to get the other albums from that ID

Anyway, having an opaque ID allows the client to simply do a for loop to grab all albums from a release group:

groupID = album.releaseGroupID // Opaque ID
group = [album]
foreach (otherAlbum from album.artist.albums) {
   if otherAlbum.releaseGroupID == groupID {
      group.append(otherAlbum)
   }
}

Simple enough no? No extra API call needed.

EDIT: I see this only works for when the client has all data for an artist in its local db. If not a new call will be required for sure, but we already have that: getArtist

A possible problem with groupAlbumIds is it places extra burden on the server? Maybe it is optional and groups can also be pulled by passing a param to search3?

Yes it does add extra burden on the server and I don't feel implementing it this way, if that's approved.

---

Offtopic:

You are still thinking pre multiple artists and track artists and everything new your BFR ;)

I don't see how this relates to the discussion, and I find it unnecessary and uncalled-for. Not cool. And if we are going this direction, I rather not participate in this discussion anymore.

EDIT2: Sorry, I had to reread a few times your message and context to understand what you meant. Maybe it is an issue with two non-native trying to discuss in English? :) Yes, I was still thinking with a "single-artist" mentality. Anyway, sorry for my "uncalled-for" reaction.

[dweymouth]

If you expose just an opaqueID you need an API call to be able to get the other albums from that ID.

This is true regardless (or else a new param on getAlbumList2) because otherwise online-first clients have no way of efficiently querying for albums within a release group whether the ID is opaque or not

[mgdigital]

If the version label was back at the top album level then the proposed releaseGroup object could be a separate (and longer) discussion. That was the original intent of this PR.

That way releaseGroup would only contain IDs of the group and possibly the albums in the group. It is independent of the version label which is anyway irrelevant for grouping and obtaining other albums in the group. It's simply a label to display in the UI.

[Tolriq]

I don't see how this relates to the discussion, and I find it unnecessary and uncalled-for. Not cool. And if we are going this direction, I rather not participate in this discussion anymore.

This is tied because albums now have multiple artists and multiple track artists. You already edited your simple solution to see that it does not work, and it will be the same for artists that are part of multiple albums but may not be part of all the releases from the group ....

So no a simple call to getArtist does not work either with multiple artists per albums ...

This is just a fact don't see anything else, and your edit with a non working solution just confirm. There's no shame or anything, just that there's different paradigm to take in account now with multiple artists and artist roles.

Edit: And again I personnaly don't care because I do have all in the local DB and I perfectly know how to rebuild the data ;), but I think globally for the actual other clients who don't and are the vast majority.

[Tolriq]

This is true regardless (or else a new param on getAlbumList2) because otherwise online-first clients have no way of efficiently querying for albums within a release group whether the ID is opaque or not

This was more versus returning the other album IDs from the group, but Deluan already said he does not want that so well :)

Anyway as always simple things are complicated to be discussed simply without some taking things personnally.

Let's start simple and vote on this post.

Add a thumb up to this post if you are 100% OK to fully differentiate versions from album groups.
And thumb down if not.

If all agrees then let's go back to first proposal and delay release groups ad vitam aeternam as every complicated decisions.

[mgdigital]

Done!