MBS-12781: Avoid admin accidentally breaking options trees

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'm also blocking linking a type as a child of its own child, which also causes similar breaking loop messes. To do that properly we need a recursive query, but the tables are relatively small and these edits happen fairly rarely, so it's probably not an issue.
metabrainz · Dec 14, 2023 · 11f73b1 · 11f73b1
1 parent 78eca85
commit 11f73b1
Show file tree

Hide file tree

Showing 6 changed files with 142 additions and 7 deletions.
diff --git a/lib/MusicBrainz/Server/Controller/Relationship/LinkType.pm b/lib/MusicBrainz/Server/Controller/Relationship/LinkType.pm
@@ -171,7 +171,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 ),

diff --git a/lib/MusicBrainz/Server/Data/LinkType.pm b/lib/MusicBrainz/Server/Data/LinkType.pm
@@ -278,6 +278,34 @@ sub load_root_ids {
     }
 }
 
+=method is_child
+
+Checks whether a specific attribute is a child of the given one in the tree.
+Used to block creating loops where a parent is set as its own child.
+
+=cut
+
+sub is_child {
+    my ($self, $own_id, $possible_child_id) = @_;
+
+    return $self->sql->select_single_value(<<~'SQL', $own_id, $possible_child_id);
+        WITH RECURSIVE hierarchy(parent, children) AS (
+                SELECT parent, ARRAY[id] AS children
+                  FROM link_type
+            UNION ALL
+                SELECT link_type.parent, hierarchy.children || link_type.id
+                  FROM link_type
+                  JOIN hierarchy ON link_type.id = hierarchy.parent
+                 WHERE link_type.parent != any(hierarchy.children)
+        )
+        SELECT 1
+          FROM hierarchy
+         WHERE parent = ?
+           AND ? = any(children)
+         LIMIT 1
+        SQL
+}
+
 sub insert
 {
     my ($self, $values) = @_;

diff --git a/lib/MusicBrainz/Server/Data/Role/OptionsTree.pm b/lib/MusicBrainz/Server/Data/Role/OptionsTree.pm
@@ -2,6 +2,8 @@ package MusicBrainz::Server::Data::Role::OptionsTree;
 use MooseX::Role::Parameterized;
 use namespace::autoclean;
 
+use MusicBrainz::Server::Constants qw( %ENTITIES );
+
 # This is not used by the role directly,
 # but passed to Role::SelectAll below
 parameter 'order_by' => (
@@ -44,8 +46,37 @@ role {
 
         return $root;
     }
+
+    sub is_child {
+        my ($self, $own_id, $possible_child_id) = @_;
+
+        my $table = $ENTITIES{$self->_type}{table};
+
+        return $self->sql->select_single_value(<<~"SQL", $own_id, $possible_child_id);
+            WITH RECURSIVE hierarchy(parent, children) AS (
+                    SELECT parent, ARRAY[id] AS children
+                      FROM $table
+                UNION ALL
+                    SELECT $table.parent, hierarchy.children || $table.id
+                      FROM $table
+                      JOIN hierarchy ON $table.id = hierarchy.parent
+                     WHERE $table.parent != any(hierarchy.children)
+            )
+            SELECT 1
+              FROM hierarchy
+             WHERE parent = ?
+               AND ? = any(children)
+             LIMIT 1
+            SQL
+    }
 };
 
+=method is_child
+
+Checks whether a specific row id is a child of the given one in the tree. 
+
+=cut
+
 1;
 
 =head1 COPYRIGHT AND LICENSE

diff --git a/lib/MusicBrainz/Server/Form/Admin/Attributes.pm b/lib/MusicBrainz/Server/Form/Admin/Attributes.pm
@@ -60,6 +60,32 @@ sub options_item_entity_type {
     return map { $_ => $_ } sort { $a cmp $b } entities_with($entity_type);
 }
 
+after validate => sub {
+    my ($self) = @_;
+
+    my $model = $self->ctx->model($self->ctx->stash->{model});
+    my $parent = $self->field('parent_id')->value
+        ? $model->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(
+                'An attribute cannot be its own parent.',
+            );
+        } else {
+            my $is_own_child = $model->is_child($own_id, $parent->id);
+            if ($is_own_child) {
+                $self->field('parent_id')->add_error(
+                    'An attribute cannot be a child of its own child.',
+                );
+            }
+        }
+    }
+};
+
 1;
 
 =head1 COPYRIGHT AND LICENSE

diff --git a/lib/MusicBrainz/Server/Form/Admin/LinkAttributeType.pm b/lib/MusicBrainz/Server/Form/Admin/LinkAttributeType.pm
@@ -49,13 +49,35 @@ sub options_parent_id
 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 $model = $self->ctx->model('LinkAttributeType');
+    my $parent = $self->field('parent_id')->value
+        ? $model->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(
+                'An attribute cannot be its own parent.',
+            );
+        } else {
+            my $is_own_child = $model->is_child($own_id, $parent->id);
+            if ($is_own_child) {
+                $self->field('parent_id')->add_error(
+                    '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('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(
+            'Cannot add or edit instruments here; use the instrument editing forms instead.',
+        );
     }
 };
 

diff --git a/lib/MusicBrainz/Server/Form/Admin/LinkType.pm b/lib/MusicBrainz/Server/Form/Admin/LinkType.pm
@@ -126,6 +126,34 @@ sub options_parent_id
     return select_options_tree($self->ctx, $self->root, accessor => 'name');
 }
 
+after validate => sub {
+    my ($self) = @_;
+
+    my $model = $self->ctx->model('LinkType');
+    my $parent = $self->field('parent_id')->value
+        ? $model->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(
+                'A relationship type cannot be its own parent.',
+            );
+        } else {
+            my $is_own_child = $model->is_child($own_id, $parent->id);
+            if ($is_own_child) {
+                $self->field('parent_id')->add_error(
+                    'A relationship type cannot be a child of its own child.',
+                );
+            }
+        }
+    }
+};
+
 1;
 
 =head1 COPYRIGHT AND LICENSE