[mb-commits] branch, mbs-5696, created. MBS-5696: Assert uniqueness for labels also
MusicBrainz Git Server <[email protected]> Fri, 25 Jan 2013 16:19:21 +0000
| Newsgroups | gmane.comp.audio.musicbrainz.cvs |
|---|---|
| Message-ID | <E1Tylzt-0001Yi-4q@wiley> |
The branch, mbs-5696 has been created
at e7d9990c74834bb387b39eb8e3e952700f04e1a3 (commit)
- Log -----------------------------------------------------------------
commit e7d9990c74834bb387b39eb8e3e952700f04e1a3
Author: Oliver Charles <[email protected]>
Date: Fri Jan 25 16:13:28 2013 +0000
MBS-5696: Assert uniqueness for labels also
diff --git a/lib/MusicBrainz/Server/Data/Artist.pm b/lib/MusicBrainz/Server/Data/Artist.pm
index f64df11..fcc61c8 100644
--- a/lib/MusicBrainz/Server/Data/Artist.pm
+++ b/lib/MusicBrainz/Server/Data/Artist.pm
@@ -7,7 +7,6 @@ use List::MoreUtils qw( uniq );
use MusicBrainz::Server::Constants qw( $VARTIST_ID $DARTIST_ID $STATUS_OPEN );
use MusicBrainz::Server::Entity::Artist;
use MusicBrainz::Server::Entity::PartialDate;
-use MusicBrainz::Server::Exceptions;
use MusicBrainz::Server::Data::ArtistCredit;
use MusicBrainz::Server::Data::Edit;
use MusicBrainz::Server::Data::Utils qw(
@@ -20,9 +19,9 @@ use MusicBrainz::Server::Data::Utils qw(
merge_table_attributes
merge_partial_date
placeholders
- query_to_list
query_to_list_limited
);
+use MusicBrainz::Server::Data::Utils::Uniqueness qw( assert_uniqueness_conserved );
extends 'MusicBrainz::Server::Data::CoreEntity';
with 'MusicBrainz::Server::Data::Role::Annotation' => { type => 'artist' };
@@ -225,44 +224,7 @@ sub update
my %names = $self->find_or_insert_names($update->{name}, $update->{sort_name});
my $row = $self->_hash_to_row($update, \%names);
- # Check if this could violate uniqueness constraints
- if (exists $update->{comment} || exists $update->{name}) {
- my ($new_name, $new_comment, @params);
-
- if (exists $update->{name}) {
- $new_name = '?::text';
- push @params, $update->{name};
- }
- else {
- $new_name = '(SELECT name.name FROM artist
- JOIN artist_name name ON artist.name = name.id
- WHERE artist.id = ?)';
- push @params, $artist_id;
- }
-
- if (exists $update->{comment}) {
- $new_comment = '?::text';
- push @params, $update->{comment};
- }
- else {
- $new_comment = '(SELECT comment FROM artist WHERE id = ?)';
- push @params, $artist_id;
- }
-
- my $query =
- "SELECT " . $self->_columns . ' FROM ' . $self->_table .
- " WHERE (name.name, comment) IN (SELECT $new_name, $new_comment)";
-
- my ($conflict) = query_to_list(
- $self->sql, sub { $self->_new_from_row(shift) }, $query, @params
- );
-
- if ($conflict) {
- MusicBrainz::Server::Exceptions::DuplicateViolation->throw({
- conflict => $conflict
- })
- }
- }
+ assert_uniqueness_conserved($self, artist => $artist_id, $update);
$self->sql->update_row('artist', $row, { id => $artist_id }) if %$row;
}
diff --git a/lib/MusicBrainz/Server/Data/Label.pm b/lib/MusicBrainz/Server/Data/Label.pm
index f72d5c4..95536f1 100644
--- a/lib/MusicBrainz/Server/Data/Label.pm
+++ b/lib/MusicBrainz/Server/Data/Label.pm
@@ -18,6 +18,7 @@ use MusicBrainz::Server::Data::Utils qw(
query_to_list
query_to_list_limited
);
+use MusicBrainz::Server::Data::Utils::Uniqueness qw( assert_uniqueness_conserved );
extends 'MusicBrainz::Server::Data::CoreEntity';
with 'MusicBrainz::Server::Data::Role::Annotation' => { type => 'label' };
@@ -179,6 +180,9 @@ sub update
my %names = $self->find_or_insert_names($update->{name}, $update->{sort_name});
my $row = $self->_hash_to_row($update, \%names);
+
+ assert_uniqueness_conserved($self, label => $label_id, $update);
+
$self->sql->update_row('label', $row, { id => $label_id }) if %$row;
return 1;
diff --git a/lib/MusicBrainz/Server/Data/Utils/Uniqueness.pm b/lib/MusicBrainz/Server/Data/Utils/Uniqueness.pm
new file mode 100644
index 0000000..a9ef795
--- /dev/null
+++ b/lib/MusicBrainz/Server/Data/Utils/Uniqueness.pm
@@ -0,0 +1,56 @@
+package MusicBrainz::Server::Data::Utils::Uniqueness;
+use Moose;
+
+use MusicBrainz::Server::Data::Utils qw(
+ query_to_list
+);
+use MusicBrainz::Server::Exceptions;
+
+use Sub::Exporter -setup => {
+ exports => [qw( assert_uniqueness_conserved )]
+};
+
+sub assert_uniqueness_conserved {
+ my ($model, $table, $id, $update) = @_;
+
+ # Check if this could violate uniqueness constraints
+ if (exists $update->{comment} || exists $update->{name}) {
+ my ($new_name, $new_comment, @params);
+
+ if (exists $update->{name}) {
+ $new_name = '?::text';
+ push @params, $update->{name};
+ }
+ else {
+ $new_name = "(SELECT name.name FROM $table
+ JOIN ${table}_name name ON $table.name = name.id
+ WHERE $table.id = ?)";
+ push @params, $id;
+ }
+
+ if (exists $update->{comment}) {
+ $new_comment = '?::text';
+ push @params, $update->{comment};
+ }
+ else {
+ $new_comment = "(SELECT comment FROM $table WHERE id = ?)";
+ push @params, $id;
+ }
+
+ my $query =
+ "SELECT " . $model->_columns . ' FROM ' . $model->_table .
+ " WHERE (name.name, comment) IN (SELECT $new_name, $new_comment)";
+
+ my ($conflict) = query_to_list(
+ $model->sql, sub { $model->_new_from_row(shift) }, $query, @params
+ );
+
+ if ($conflict) {
+ MusicBrainz::Server::Exceptions::DuplicateViolation->throw({
+ conflict => $conflict
+ })
+ }
+ }
+}
+
+1;
diff --git a/t/lib/t/MusicBrainz/Server/Data/Label.pm b/t/lib/t/MusicBrainz/Server/Data/Label.pm
index 939b211..5e757ce 100644
--- a/t/lib/t/MusicBrainz/Server/Data/Label.pm
+++ b/t/lib/t/MusicBrainz/Server/Data/Label.pm
@@ -139,4 +139,33 @@ test 'Deny delete "Deleted Label" trigger' => sub {
}, qr/ERROR:\s*Attempted to delete a special purpose row/;
};
+test 'Cannot edit an label into something that would violate uniqueness' => sub {
+ my $c = shift->c;
+ $c->sql->do(<<'EOSQL');
+INSERT INTO label_name (id, name) VALUES (1, 'A'), (2, 'B');
+INSERT INTO label (id, gid, name, sort_name, comment) VALUES
+ (3, '745c079d-374e-4436-9448-da92dedef3ce', 1, 1, ''),
+ (4, '7848d7ce-d650-40c4-b98f-62fc037a678b', 2, 1, 'Comment');
+EOSQL
+
+ my $conflicts_exception_ok = sub {
+ my ($e, $target) = @_;
+
+ isa_ok $e, 'MusicBrainz::Server::Exceptions::DuplicateViolation';
+ is $e->conflict->id, $target;
+ };
+
+ ok !exception { $c->model('Label')->update(4, { comment => '' }) };
+ $conflicts_exception_ok->(
+ exception { $c->model('Label')->update(3, { name => 'B' }) },
+ 4
+ );
+
+ ok !exception { $c->model('Label')->update(3, { name => 'B', comment => 'Unique' }) };
+ $conflicts_exception_ok->(
+ exception { $c->model('Label')->update(3, { comment => '' }) },
+ 4
+ );
+};
+
1;
commit a0eea5a8c85498ab49c17980534259bb33ed27cb
Author: Oliver Charles <[email protected]>
Date: Fri Jan 25 15:59:25 2013 +0000
MBS-5696: Catch DuplicateViolation errors in Generic::Edit and echo them via ModBot
diff --git a/lib/MusicBrainz/Server/Edit/Generic/Edit.pm b/lib/MusicBrainz/Server/Edit/Generic/Edit.pm
index 97b9374..8bbbdd5 100644
--- a/lib/MusicBrainz/Server/Edit/Generic/Edit.pm
+++ b/lib/MusicBrainz/Server/Edit/Generic/Edit.pm
@@ -66,7 +66,22 @@ override 'accept' => sub
}
my $data = $self->_edit_hash(clone($self->data->{new}));
- $self->c->model( $self->_edit_model )->update($self->entity_id, $data);
+ try {
+ $self->c->model( $self->_edit_model )->update($self->entity_id, $data);
+ }
+ catch {
+ if (blessed($_) && $_->isa('MusicBrainz::Server::Exceptions::DuplicateViolation')) {
+ my $conflict = $_->conflict;
+ MusicBrainz::Server::Edit::Exceptions::GeneralError->throw(
+ sprintf(
+ 'The changes in this edit cause it to conflict with another artist. ' .
+ 'You may need to merge this artist with "' . $conflict->name . '" ' .
+ '(http://%s/artist/%s/).',
+ DBDefs->WEB_SERVER, $conflict->gid
+ )
+ );
+ }
+ };
};
sub _edit_hash
diff --git a/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm b/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm
index 4d801df..e35c819 100644
--- a/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm
+++ b/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm
@@ -227,6 +227,39 @@ test 'Check IPI changes' => sub {
[ '11111111111', '55555555555', '66666666666', '77777777777' ]);
};
+test 'Editing two artists into a conflict fails gracefully' => sub {
+ my $test = shift;
+ my $c = $test->c;
+
+ MusicBrainz::Server::Test->prepare_test_database($c, '+edit_artist_merge');
+
+ my $edit_1 = $c->model('Edit')->create(
+ edit_type => $EDIT_ARTIST_EDIT,
+ editor_id => 1,
+ to_edit => $c->model('Artist')->get_by_id(3),
+ name => 'Conflicting name',
+ comment => 'Conflicting comment',
+ ipi_codes => []
+ );
+
+ my $edit_2 = $c->model('Edit')->create(
+ edit_type => $EDIT_ARTIST_EDIT,
+ editor_id => 1,
+ to_edit => $c->model('Artist')->get_by_id(4),
+ name => 'Conflicting name',
+ comment => 'Conflicting comment',
+ ipi_codes => []
+ );
+
+ ok !exception { $edit_1->accept }, 'First edit can be applied';
+
+ my $exception = exception { $edit_1->accept };
+ isa_ok $exception, 'MusicBrainz::Server::Edit::Exceptions::GeneralError';
+ like $exception->message, qr/da34a170-7f7f-11de-8a39-0800200c9a66/,
+ 'Error message contains the MBID of the conflict';
+ diag $exception->message;
+};
+
sub _create_full_edit {
my ($c, $artist) = @_;
return $c->model('Edit')->create(
commit 549655c402d1ca3074b8f36e50f1e2a048958a5a
Author: Oliver Charles <[email protected]>
Date: Fri Jan 25 15:31:22 2013 +0000
MBS-5696: Throw duplicate violation exceptions when editing artists
If an edit would cause an artist to conflict with another existing
artist, we throw a structured exception to indicate this, with
metadata about the artist that the edit would conflict with.
diff --git a/lib/MusicBrainz/Server/Data/Artist.pm b/lib/MusicBrainz/Server/Data/Artist.pm
index 6fe95b5..f64df11 100644
--- a/lib/MusicBrainz/Server/Data/Artist.pm
+++ b/lib/MusicBrainz/Server/Data/Artist.pm
@@ -7,6 +7,7 @@ use List::MoreUtils qw( uniq );
use MusicBrainz::Server::Constants qw( $VARTIST_ID $DARTIST_ID $STATUS_OPEN );
use MusicBrainz::Server::Entity::Artist;
use MusicBrainz::Server::Entity::PartialDate;
+use MusicBrainz::Server::Exceptions;
use MusicBrainz::Server::Data::ArtistCredit;
use MusicBrainz::Server::Data::Edit;
use MusicBrainz::Server::Data::Utils qw(
@@ -19,6 +20,7 @@ use MusicBrainz::Server::Data::Utils qw(
merge_table_attributes
merge_partial_date
placeholders
+ query_to_list
query_to_list_limited
);
@@ -222,6 +224,46 @@ sub update
croak '$artist_id must be present and > 0' unless $artist_id > 0;
my %names = $self->find_or_insert_names($update->{name}, $update->{sort_name});
my $row = $self->_hash_to_row($update, \%names);
+
+ # Check if this could violate uniqueness constraints
+ if (exists $update->{comment} || exists $update->{name}) {
+ my ($new_name, $new_comment, @params);
+
+ if (exists $update->{name}) {
+ $new_name = '?::text';
+ push @params, $update->{name};
+ }
+ else {
+ $new_name = '(SELECT name.name FROM artist
+ JOIN artist_name name ON artist.name = name.id
+ WHERE artist.id = ?)';
+ push @params, $artist_id;
+ }
+
+ if (exists $update->{comment}) {
+ $new_comment = '?::text';
+ push @params, $update->{comment};
+ }
+ else {
+ $new_comment = '(SELECT comment FROM artist WHERE id = ?)';
+ push @params, $artist_id;
+ }
+
+ my $query =
+ "SELECT " . $self->_columns . ' FROM ' . $self->_table .
+ " WHERE (name.name, comment) IN (SELECT $new_name, $new_comment)";
+
+ my ($conflict) = query_to_list(
+ $self->sql, sub { $self->_new_from_row(shift) }, $query, @params
+ );
+
+ if ($conflict) {
+ MusicBrainz::Server::Exceptions::DuplicateViolation->throw({
+ conflict => $conflict
+ })
+ }
+ }
+
$self->sql->update_row('artist', $row, { id => $artist_id }) if %$row;
}
diff --git a/lib/MusicBrainz/Server/Edit/Exceptions.pm b/lib/MusicBrainz/Server/Edit/Exceptions.pm
index 3a6e3a1..1f500cd 100644
--- a/lib/MusicBrainz/Server/Edit/Exceptions.pm
+++ b/lib/MusicBrainz/Server/Edit/Exceptions.pm
@@ -5,7 +5,10 @@ use Exception::Class (
'MusicBrainz::Server::Edit::Exceptions::FailedDependency',
'MusicBrainz::Server::Edit::Exceptions::HistoricDataCorrupt',
'MusicBrainz::Server::Edit::Exceptions::MustApply',
- 'MusicBrainz::Server::Edit::Exceptions::GeneralError'
+ 'MusicBrainz::Server::Edit::Exceptions::GeneralError',
+ 'MusicBrainz::Server::Edit::Exceptions::DuplicateViolation' => {
+ fields => [qw( conflict )]
+ }
);
1;
diff --git a/lib/MusicBrainz/Server/Exceptions.pm b/lib/MusicBrainz/Server/Exceptions.pm
index 5bac83c..97189c4 100644
--- a/lib/MusicBrainz/Server/Exceptions.pm
+++ b/lib/MusicBrainz/Server/Exceptions.pm
@@ -16,4 +16,10 @@ package MusicBrainz::Server::Exceptions::InvalidSearchParameters;
use Moose;
extends 'Throwable::Error';
+package MusicBrainz::Server::Exceptions::DuplicateViolation;
+use Moose;
+with 'Throwable';
+
+has 'conflict' => ( is => 'ro', required => 1 );
+
1;
diff --git a/t/lib/t/MusicBrainz/Server/Data/Artist.pm b/t/lib/t/MusicBrainz/Server/Data/Artist.pm
index b5addd4..aa9595b 100644
--- a/t/lib/t/MusicBrainz/Server/Data/Artist.pm
+++ b/t/lib/t/MusicBrainz/Server/Data/Artist.pm
@@ -394,4 +394,33 @@ EOSQL
is($artist->end_date->day, 12);
};
+test 'Cannot edit an artist into something that would violate uniqueness' => sub {
+ my $c = shift->c;
+ $c->sql->do(<<'EOSQL');
+INSERT INTO artist_name (id, name) VALUES (1, 'A'), (2, 'B');
+INSERT INTO artist (id, gid, name, sort_name, comment) VALUES
+ (3, '745c079d-374e-4436-9448-da92dedef3ce', 1, 1, ''),
+ (4, '7848d7ce-d650-40c4-b98f-62fc037a678b', 2, 1, 'Comment');
+EOSQL
+
+ my $conflicts_exception_ok = sub {
+ my ($e, $target) = @_;
+
+ isa_ok $e, 'MusicBrainz::Server::Exceptions::DuplicateViolation';
+ is $e->conflict->id, $target;
+ };
+
+ ok !exception { $c->model('Artist')->update(4, { comment => '' }) };
+ $conflicts_exception_ok->(
+ exception { $c->model('Artist')->update(3, { name => 'B' }) },
+ 4
+ );
+
+ ok !exception { $c->model('Artist')->update(3, { name => 'B', comment => 'Unique' }) };
+ $conflicts_exception_ok->(
+ exception { $c->model('Artist')->update(3, { comment => '' }) },
+ 4
+ );
+};
+
1;
-----------------------------------------------------------------------
hooks/post-receive
--
mb_server