Skip to content

Commit

Permalink
MBS-12781: Avoid admin accidentally breaking options trees
Browse files Browse the repository at this point in the history
When editing attributes, relationship types and relationship attributes,
you can change their parent type.
There's currently nothing stopping an admin from accidentally selecting
the entity itself from the dropdown, which makes MB so confused
it doesn't know where to show the entity - so it doesn't. Fun times.

I also tried blocking linking a type as a child of its own child,
which also causes similar breaking loop messes.
I don't think it's possible to do that completely
with the tools we have now, though.
This code blocks a root from being set under any of its children
and a direct parent from being set under its direct children,
which should cover most possible cases;
in a very deep tree a->b->c->d it's probably still possible
to set b as a child of d but I don't think it's worth adding
a new recursive query to get all child IDs just to avoid that scenario.
  • Loading branch information
reosarevok committed Dec 9, 2022
1 parent 7f437e5 commit 755360e
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 4 deletions.
2 changes: 1 addition & 1 deletion lib/MusicBrainz/Server/Controller/Relationship/LinkType.pm
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ sub edit : Chained('load') RequireAuth(relationship_editor)
init_object => {
attributes => $attribs,
map { $_ => $link_type->$_ }
qw( parent_id child_order name link_phrase reverse_link_phrase
qw( id parent_id child_order name link_phrase reverse_link_phrase
long_link_phrase description priority documentation
examples is_deprecated has_dates entity0_cardinality entity1_cardinality
orderable_direction )
Expand Down
26 changes: 26 additions & 0 deletions lib/MusicBrainz/Server/Form/Admin/Attributes.pm
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use warnings;
use HTML::FormHandler::Moose;
use MusicBrainz::Server::Form::Utils qw( select_options_tree );
use MusicBrainz::Server::Constants qw( entities_with );
use MusicBrainz::Server::Translation qw( l );

extends 'MusicBrainz::Server::Form';
with 'MusicBrainz::Server::Form::Role::Edit';
Expand Down Expand Up @@ -60,6 +61,31 @@ sub options_item_entity_type {
return map { $_ => $_ } sort { $a cmp $b } entities_with($entity_type);
}

after validate => sub {
my ($self) = @_;

my $parent = $self->field('parent_id')->value ?
$self->ctx->model('LinkAttributeType')->get_by_id($self->field('parent_id')->value) :
undef;
my $own_id = defined $self->init_object ? $self->init_object->{id} : undef;

if (defined $parent && defined $own_id) {
my $is_self_parent = $parent->id == $own_id;
if ($is_self_parent) {
$self->field('parent_id')->add_error(
l('An attribute cannot be its own parent.'),
);
} else {
my $is_parent_parent = $parent->parent_id == $own_id;
if ($is_parent_parent) {
$self->field('parent_id')->add_error(
l('An attribute cannot be a child of its own child.'),
);
}
}
}
};

1;

=head1 COPYRIGHT AND LICENSE
Expand Down
28 changes: 25 additions & 3 deletions lib/MusicBrainz/Server/Form/Admin/LinkAttributeType.pm
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,32 @@ after validate => sub {
my $parent = $self->field('parent_id')->value ?
$self->ctx->model('LinkAttributeType')->get_by_id($self->field('parent_id')->value) :
undef;
my $own_id = defined $self->init_object ? $self->init_object->id : undef;

if (defined $parent && defined $own_id) {
my $is_self_parent = $parent->id == $own_id;
if ($is_self_parent) {
$self->field('parent_id')->add_error(
l('An attribute cannot be its own parent.'),
);
} else {
my $is_parent_root = $parent->root_id == $own_id;
my $is_parent_parent = $parent->parent_id == $own_id;
if ($is_parent_root || $is_parent_parent) {
$self->field('parent_id')->add_error(
l('An attribute cannot be a child of its own child.'),
);
}
}
}

my $root = defined $parent ? ($parent->root_id // $parent->id) : 0;
if ($root == $INSTRUMENT_ROOT_ID) {
$self->field('parent_id')->add_error(l('Cannot add or edit instruments here; use the instrument editing forms instead.'));
my $is_root_instrument = defined $parent && (
$parent->root_id == $INSTRUMENT_ROOT_ID ||
$parent->id == $INSTRUMENT_ROOT_ID);
if ($is_root_instrument) {
$self->field('parent_id')->add_error(
l('Cannot add or edit instruments here; use the instrument editing forms instead.'),
);
}
};

Expand Down
31 changes: 31 additions & 0 deletions lib/MusicBrainz/Server/Form/Admin/LinkType.pm
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use warnings;

use HTML::FormHandler::Moose;
use MusicBrainz::Server::Form::Utils qw( select_options_tree );
use MusicBrainz::Server::Translation qw( l );

extends 'MusicBrainz::Server::Form';
with 'MusicBrainz::Server::Form::Role::Edit';
Expand Down Expand Up @@ -126,6 +127,36 @@ sub options_parent_id
return select_options_tree($self->ctx, $self->root, accessor => 'name');
}

after validate => sub {
my ($self) = @_;

my $parent = $self->field('parent_id')->value ?
$self->ctx->model('LinkType')->get_by_id($self->field('parent_id')->value) :
undef;
my $own_id = defined $self->init_object
? $self->init_object->{id}
: undef;

if (defined $parent && defined $own_id) {
my $is_self_parent = $parent->id == $own_id;
if ($is_self_parent) {
$self->field('parent_id')->add_error(
l('A relationship type cannot be its own parent.'),
);
} else {
$self->ctx->model('LinkType')->load_root_ids($parent);

my $is_parent_root = $parent->root_id == $own_id;
my $is_parent_parent = $parent->parent_id == $own_id;
if ($is_parent_root || $is_parent_parent) {
$self->field('parent_id')->add_error(
l('A relationship type cannot be a child of its own child.'),
);
}
}
}
};

1;

=head1 COPYRIGHT AND LICENSE
Expand Down

0 comments on commit 755360e

Please sign in to comment.