MBS-12364, MBS-12367: Make use of AC MBIDs in website and webservice #2755
Conversation
Since MBS-11456, artist credits can have multiple MBIDs
(through GID redirects) but can still not have relationships.
Update entities.mjs then regenerate entities.json with:
./script/generate_entities_json.mjs > entities.json
Setting `mbid` makes:
- annotation to support `[artist_credit:<mbid>]` link notation;
- `/mbid/<mbid>` page to redirect to `/artist-credit/<mbid>` too;
- statistics to include artist credits when counting MBIDs
(even though artist credit count isn’t displayed, for now).
Setting `mbid:multiple` will make:
- `Data::Role::Load` able to load GID redirects,
once `Data::ArtistCredit` stopped overriding `_load` method;
- `Data::Role::GIDRedirect` applicable to `Data::ArtistCredit`.
Notes:
- Setting `mbid:relatable` (to `false`) doesn’t make anything
but is generally set when `mbid` exists;
- `MusicBrainz::Script::EntityDump` doesn’t refer to multiple MBIDs
when dumping artist credits, for now.
Amend commit 8a841d6 which defined the method `load_gid_redirects` without importing `object_to_ids`. This bug was inactive because this method was (is) dead code (for now).
Split the method `get_by_ids` of `Data::ArtistCredit` (which overwrites the same method of `Data::Entity` it extends), so that the second part can be used for getting ACs by GID too. The new subroutine for this second part has the distinctive name `_get_hash_by_keys` because it doesn’t return an array like `_get_by_keys` of `Data::Entity` does but it is the same otherwise. Update the POD accordingly.
In `Data::ArtistCredit`, the method `_get_by_keys` was inherited from `Data::Entity` but wasn’t used so far and wouldn’t work as-is. This patch overrides this method so that it works as it is required by the role `GetByGID` added to `Data::ArtistCredit` in commit 8a841d6 without making use of it. Update the POD accordingly.
Redirect both row ID and old GID to the canonical/current GID/MBID for accessing artist credit page. This patch also changes artist credit cache to use the GID for keys. This cache is short-lived anyway: 1h on mirrors, 1 day on main server. It also loads GID and GID redirects in ArtistCredit Moose entity model. By loading GID it breaks JSON WS/2 which is fixed in separate commits. Roles are ordered by (even weak) dependency, then alphabetically. Update tests to differentiate invalid ID into non-existent ID (for example a too large integer) and incorrectly formatted ID (which is neither an UUID nor a positive integer). In the latter case, returning 400 (Bad Request) is actually the right thing to do. Further tests are added in separate commits.
Test that the user is redirected to the artist credit’s page with the current/canonical MBID when using either its row ID or an old MBID. Update the basic test to use the canonical MBID instead of the row ID.
Since `'artist-credit'` is a JSON array of artist credit names, adding artist credit’s MBID under it would require to turn it into an object which would be a breaking change for everything that uses the WS/2 API. Instead this patch serialize artist credit’s MBID and add it under the new key `'artist-credit-id'` at the same level as `artist-credit`. Tests are updated in a separate commit.
Add an `id` attribute to the `artist-credit` XML elements in WS/2 lookup/browse results, and set it to the artist credit’s MBID. Tests are updated in a separate commit.
Are you adding any JSON-LD to them? If not, probably not? But let's see what @mwiencek thinks.
I honestly didn't even remember we have
Aren't they already dumped? The |
There was a problem hiding this comment.
There's also a typo "seralization" in the commit message for 26aba5b
Thanks for working on this - it looks sensible to me so far (tested manually based on your suggestions too).
| with 'MusicBrainz::Server::Data::Role::EntityModelClass'; | ||
| with 'MusicBrainz::Server::Data::Role::GetByGID'; | ||
| with 'MusicBrainz::Server::Data::Role::MainTable'; | ||
| # Follows GetByGID and MainTable as it requires 'get_by_gid' and '_main_table'; |
There was a problem hiding this comment.
Have you tried just consuming them all in one with call instead? I did that for all of Data:: in #2751 and it didn't seem to cause issues (while avoiding the need to order things in weird and specific ways) :)
There was a problem hiding this comment.
No I just followed the current coding style for now even though it isn’t perfect. Changing it isn’t straightforward either. For example “the need to order things” is still there when two roles like GIDEntityCache and GIDRedirect goes around the same method get_by_gid.
Since the commit 33f470b the class `MusicBrainz::Server::WebService::Serializer::JSON::2::ArtistCredit` is unusable as the method `serialize_id` from `Utils` cannot set `id` key into `artist-credit` array. That’s why artist credit serialization has been replaced in commit 4491cf3. However this class could still be useful if `artist-credit` gets added to API resources in which case it wouldn’t be a breaking change.
yvanzo
force-pushed
the
show-ac-by-mbid
branch
from
26aba5b to
718b539
Compare
|
Thanks for your comments I moved the third prospective question about sample data dump to my todolist, and fixed the typo in the last commit message.
I didn’t think about it. About JSON-LD, I checked that our currently produced JSON-LD doesn’t include any artist credit data and thus doesn’t need to be updated with AC MBIDs.
There are wo reasons for returning a hash:
|
Problem
MBS-11456 added GIDs to artist credits in the database and MBS back-end.
Neither the website nor the webservice made these usable so far.
Solution
MBS-12364
For the website:
It has some side changes as well:
/mbid/<MBID>to/artist-credit/<MBID>if applicable[artist-credit:<mbid>]link syntax in annotationMBS-12367
For the API lookup/browse results:
idattribute toartist-creditXML elementartist-credit-idJSON key as a sibling ofartist-creditarrayTest hints
For manually testing webpages:
/mbid/94eda81b-abcc-3365-9a3c-10633d06fc71/artist-credit/94eda81b-abcc-3365-9a3c-10633d06fc71/artist-credit/1599741/artist-credit/37f3d5f8-0bb7-48e2-8f82-99b73f005fccafter having added an arbitrary GID redirect with:
For manually testing changes in API JSON results:
/ws/2/recording/52bc6f21-2c6d-48d0-be24-6ddb06a8461c?fmt=json&inc=artists/ws/2/recording/52bc6f21-2c6d-48d0-be24-6ddb06a8461c?fmt=json&inc=artist-credits/ws/2/release/a7034f6f-d038-405a-8692-59f7530f6958?fmt=json&inc=artists/ws/2/release/a7034f6f-d038-405a-8692-59f7530f6958?fmt=json&inc=artist-credits/ws/2/release-group/7c4cab8d-dead-3870-b501-93c90fd0a580?fmt=json&inc=artists/ws/2/release-group/7c4cab8d-dead-3870-b501-93c90fd0a580?fmt=json&inc=artist-creditsFor manually testing changes in API XML results:
/ws/2/recording/52bc6f21-2c6d-48d0-be24-6ddb06a8461c?fmt=xml&inc=artists/ws/2/recording/52bc6f21-2c6d-48d0-be24-6ddb06a8461c?fmt=xml&inc=artist-credits/ws/2/release/a7034f6f-d038-405a-8692-59f7530f6958?fmt=xml&inc=artists/ws/2/release/a7034f6f-d038-405a-8692-59f7530f6958?fmt=xml&inc=artist-credits/ws/2/release-group/7c4cab8d-dead-3870-b501-93c90fd0a580?fmt=xml&inc=artists/ws/2/release-group/7c4cab8d-dead-3870-b501-93c90fd0a580?fmt=xml&inc=artist-creditsI didn’t manually test any browse request, however they use the same code as lookup requests to serialize artist credits, and CI tests have been updated for both lookup and browse requests.
Prospective questions to reviewers
Todolist for author
MB_SOLR_TAGto have anmmd-schemaversion that validates XML output in CI testsEntityDumppackage)Actions after merge