MBS-8838: Add gids to all _type tables #259

gentlecat · 2016-03-16T22:16:43Z

http://tickets.musicbrainz.org/browse/MBS-8838

mwiencek · 2016-03-16T22:22:13Z

admin/sql/CreateTables.sql

@@ -21,6 +21,7 @@ CREATE TABLE application

 CREATE TABLE area_type ( -- replicate
    id                  SERIAL, -- PK
+    gid                 uuid NOT NULL,


I'm not sure this would practically cause us any problems, but since ALTER TABLE in the upgrade script will add the column to the end, I wonder if we should do the same here for consistency.

Well, actually, I guess it'd break data dump imports if we didn't.

Yes, unfortunately Postgres (even in 9.5) still doesn’t allow freely ordering columns. :-/

Ah, unfortunate. I pushed an update.

mwiencek · 2016-03-16T22:35:01Z

Some missing pieces I see. :)

Could do with some unique indexes in admin/sql/CreateIndexes.sql and admin/sql/caa/CreateIndexes.sql like

musicbrainz-server/admin/sql/CreateIndexes.sql

Line 321 in ad6f5aa

CREATE UNIQUE INDEX link_type_idx_gid ON link_type (gid);

upgrade.json in the root of the repository should be updated with a key for schema version "23" (then you just need to add "20160516-mbs-8838.sql" to slave and standalone sub-keys, meaning it'll run on both types of servers when someone runs upgrade.sh).

mwiencek · 2016-03-16T22:36:25Z

After adding indexes, run ./admin/GenerateSQLScripts.pl admin/sql/ to automatically update all the DropIndexes.sql files too.

gentlecat · 2016-03-17T04:50:28Z

OK, indexes added and upgrade.json is updated.

Is WS update the only thing left there, assuming I got the rest right?

mwiencek · 2016-03-17T05:06:32Z

Is WS update the only thing left there, assuming I got the rest right?

Yeah, I think so. 💯 So now you've got the annoying part for last.

mwiencek · 2016-03-17T05:09:06Z

But I don't see any change to upgrade.json here. :P

gentlecat · 2016-03-17T05:10:13Z

Forgot to push. 😱

gentlecat · 2016-03-19T21:57:11Z

http://chatlogs.metabrainz.org/brainzbot/metabrainz/2016-03-18/?msg=3539231&page=1

gentlecat · 2016-03-21T09:54:31Z

I updated XMLSerializer and most things seem to be working. However, I couldn't figure out how to integrate gid columns from the following tables:

cover_art_archive.art_type
editor_collection_type
series_ordering_type
work_attribute_type_allowed_value

mwiencek · 2016-03-21T18:11:32Z

Looks good so far. For the tables you mentioned:

cover_art_archive.art_type - I guess we don't currently output detailed type information for cover art in the web service, just "front"/"back", which seems odd in retrospect. So let's leave that alone for now.
editor_collection_type - Check https://github.com/metabrainz/musicbrainz-server/blob/ff74a51/lib/MusicBrainz/Server/WebService/XMLSerializer.pm#L219-L219
series_ordering_type - That's not outputted in the web service either, but IIRC it was intentional since it's mostly an internal thing. So let's ignore that too.
work_attribute_type_allowed_value - Annoying since it's just text data, like "E minor" in http://beta.musicbrainz.org/ws/2/work/aec2489b-051c-4892-8df5-6c601877e5f6 Let's just add a value-id to the attribute node. (Only where $work_attribute_type->free_text is false, since free-text values don't have any gid.)

mwiencek · 2016-03-21T19:15:03Z

lib/MusicBrainz/Server/WebService/XMLSerializer.pm

            nsort_by { $fallback_type_order{$_} }
                grep { exists $fallback_type_order{$_} }
                    map { $_->name }
                        $release_group->all_secondary_types;
+        my ($fallback_gid) =
+            nsort_by { $fallback_type_order{$_} }
+                grep { exists $fallback_type_order{$_} }


%fallback_type_order doesn't have gids for keys, so nothing will exist there. You can try removing the map { $_->name } above, use $_->name as the key into %fallback_type_order, and then do $fallback->name and $fallback->gid below.

So something like this?

my $fallback = nsort_by { $fallback_type_order{$_} } grep { exists $fallback_type_order{$_} } $release_group->all_secondary_types; if ($fallback) { $attr{type} = $fallback->name; $attr{"type-id"} = $fallback->gid; } else { $attr{type} = $release_group->primary_type->name; $attr{"type-id"} = $release_group->primary_type->gid; }

Though I'm having trouble understanding when $fallback is actually used.

Yeah, except in nsort_by/grep you should do $fallback_type_order{$_->name} since it's keyed by name.

The fallback stuff is used when the primary type is Album. If there's a more specific secondary type, it'll output that instead of Album. I'm guessing this is because we used to only have a flat list of release types, so you would just have "Compilation" instead of "Album (primary), Compilation (secondary)", so ouputting "Compilation" is more backwards-compatible.

That's only for the type attribute. We still output the primary/secondary types separately below, which people should ideally be using instead.

Thanks. Fixed.

… WS.

gentlecat · 2016-03-25T10:58:43Z

Tests should be passing now, but I still have some problems with WikiDoc ones. Check please?
https://gist.github.com/anonymous/fd7261a2a390b3408d30

mwiencek · 2016-03-25T15:15:16Z

t/sql/initial.sql

+INSERT INTO work_alias_type VALUES (1, 'Work name', NULL, 0, NULL, 'a18cab3f-0ae2-3978-8f75-dd9c09702b25');
+INSERT INTO work_alias_type VALUES (2, 'Search hint', NULL, 0, NULL, '02238bc1-dfd8-39a8-bbf8-c697747291ec');
+
+INSERT INTO work_type VALUES (29, 'Musical', NULL, 2, 'Musical theatre is a form of theatrical performance that combines songs, spoken dialogue, acting, and dance.', '9ca5e067-acf7-3cd6-baa4-92bf1975bf24');


Can we keep each group sorted so it's easy to see what was actually added? (F5 in sublime, but you prob. knew that.)

Ignore that, F5 doesn't sort numerically. Forgot I have a plugin to do that. So I'll just use it here.

mwiencek · 2016-03-25T15:24:58Z

We added a dep. on Text::Diff3 recently, so it looks like your tests are failing because of that. cpanm Text::Diff3.

mwiencek · 2016-03-25T20:02:44Z

I added the necessary changes to the JSON web service and fixed the browse tests. Back to you. :)

gentlecat · 2016-03-26T21:19:43Z

t/lib/t/MusicBrainz/Server/Controller/Work/Edit.pm

-  (1, 'Attribute', false),
-  (2, 'Free attribute', true);
+  (1, '325c079d-374e-4436-9448-da92dedef3ce', 'Attribute', false),
+  (2, '425c079d-374e-4436-9448-da92dedef3ce', 'Free attribute', true);
 INSERT INTO work_attribute_type_allowed_value (id, work_attribute_type, value)


Forgot to add gid there.

gentlecat · 2016-03-26T22:26:50Z

I'm currently a bit stuck with running tests. See http://chatlogs.metabrainz.org/brainzbot/metabrainz/2016-03-26/?msg=3549941&page=4.

mwiencek · 2016-03-29T16:40:00Z

Fixed some more tests, still a bunch of failures left. But I think it would be easier for us if we merge this and let Jenkins run things (especially since you're having trouble running them on your machine).

gentlecat · 2016-03-29T16:41:07Z

Sure. Is there anything else apart from tests that needs fixing?

mwiencek · 2016-03-29T16:46:45Z

Not that I can think of right now. We might find out something when more tests start failing. :)

gentlecat · 2016-03-29T16:47:11Z

Alright. :)

gentlecat added 2 commits March 16, 2016 19:20

Add gids to all *_type* tables.

61e51f1

Add schema update for MBS-8838.

cb520d0

gentlecat assigned mwiencek Mar 16, 2016

mwiencek reviewed Mar 16, 2016
View reviewed changes

Fix order of columns in CreateTables.sql scripts.

8c2a045

gentlecat force-pushed the mbs-8838 branch from 5b79001 to 8c2a045 Compare March 17, 2016 03:41

gentlecat added 3 commits March 17, 2016 05:44

Add indexes.

d41bbb9

Run ./admin/GenerateSQLScripts.pl on /admin/sql and /admin/sql/caa.

4efc595

Add "20160516-mbs-8838.sql" into upgrade.son

87606d6

gentlecat force-pushed the mbs-8838 branch 2 times, most recently from cc99323 to 7beda3e Compare March 21, 2016 06:08

gentlecat added 3 commits March 21, 2016 10:50

Update classes in Server::Data.

519c1b9

Add 'gid' attribute into modified Entity classes.

80f15db

Update XMLSerializer.

6c7aba7

gentlecat force-pushed the mbs-8838 branch from 7beda3e to 6c7aba7 Compare March 21, 2016 09:51

mwiencek reviewed Mar 21, 2016
View reviewed changes

Output "gid" column from "work_attribute_type_allowed_value" table in…

3c448b1

… WS.

gentlecat force-pushed the mbs-8838 branch from 2c2b73c to 3c448b1 Compare March 22, 2016 04:43

gentlecat added 2 commits March 22, 2016 06:23

Fix the way fallback type is determined for release group in WS output.

d1be709

Update old SQL scripts.

8461f8e

gentlecat force-pushed the mbs-8838 branch from f816ab2 to 8461f8e Compare March 25, 2016 10:42

gentlecat changed the title ~~[WIP] MBS-8838: Add gids to all *_type* tables~~ MBS-8838: Add gids to all *_type* tables Mar 25, 2016

mwiencek reviewed Mar 25, 2016
View reviewed changes

gentlecat and others added 6 commits March 25, 2016 17:03

Update tests.

143bb22

Sort inserts in t/sql/initial.sql

f37e235

Move types from webservice.sql into initial.sql

a1dd080

Output packaging ids in XML

e1f39fd

JSON serialization for MBS-8838

3e4b386

Fix WS browse tests for MBS-8838

735e399

gentlecat force-pushed the mbs-8838 branch from 7ccef66 to 3d43eb3 Compare March 26, 2016 21:16

gentlecat reviewed Mar 26, 2016
View reviewed changes

More updates for test SQL.

f630a14

gentlecat force-pushed the mbs-8838 branch from 3d43eb3 to f630a14 Compare March 26, 2016 21:20

Fix more Controller::WS tests

60f82b9

mwiencek merged commit aaf788d into schema-change-2016-q2 Mar 29, 2016

mwiencek deleted the mbs-8838 branch March 29, 2016 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBS-8838: Add gids to all _type tables #259

MBS-8838: Add gids to all _type tables #259

gentlecat commented Mar 16, 2016

mwiencek Mar 16, 2016

mwiencek Mar 16, 2016

chirlu Mar 17, 2016

gentlecat Mar 17, 2016

mwiencek commented Mar 16, 2016

mwiencek commented Mar 16, 2016

gentlecat commented Mar 17, 2016

mwiencek commented Mar 17, 2016

mwiencek commented Mar 17, 2016

gentlecat commented Mar 17, 2016

gentlecat commented Mar 19, 2016

gentlecat commented Mar 21, 2016

mwiencek commented Mar 21, 2016

mwiencek Mar 21, 2016

gentlecat Mar 22, 2016

mwiencek Mar 22, 2016

gentlecat Mar 22, 2016

gentlecat commented Mar 25, 2016

mwiencek Mar 25, 2016

mwiencek Mar 25, 2016

mwiencek commented Mar 25, 2016

mwiencek commented Mar 25, 2016

gentlecat Mar 26, 2016

gentlecat Mar 26, 2016

gentlecat commented Mar 26, 2016

mwiencek commented Mar 29, 2016

gentlecat commented Mar 29, 2016

mwiencek commented Mar 29, 2016

gentlecat commented Mar 29, 2016

MBS-8838: Add gids to all *_type* tables #259

MBS-8838: Add gids to all *_type* tables #259

Conversation

gentlecat commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwiencek commented Mar 16, 2016

mwiencek commented Mar 16, 2016

gentlecat commented Mar 17, 2016

mwiencek commented Mar 17, 2016

mwiencek commented Mar 17, 2016

gentlecat commented Mar 17, 2016

gentlecat commented Mar 19, 2016

gentlecat commented Mar 21, 2016

mwiencek commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlecat commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwiencek commented Mar 25, 2016

mwiencek commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlecat commented Mar 26, 2016

mwiencek commented Mar 29, 2016

gentlecat commented Mar 29, 2016

mwiencek commented Mar 29, 2016

gentlecat commented Mar 29, 2016

MBS-8838: Add gids to all _type tables #259

MBS-8838: Add gids to all _type tables #259