Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MBS-12781: Avoid admin accidentally breaking options trees #2776

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

@reosarevok reosarevok commented Dec 9, 2022

Fix MBS-12781

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.

@reosarevok reosarevok added the Bug Bugs that should be checked/fixed soonish label Dec 9, 2022
@mwiencek
Copy link
Member

A recursive query isn't much harder, so might be worth doing to prevent future mistakes? (I didn't even know you could set a type as its own parent at present, but here we are. :))

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 = 123
   AND 456 = any(children)
 LIMIT 1;

I guess Data::LinkType doesn't consume Data::Role::OptionsTree for some reason, but we could just duplicate the query in Data::LinkType for now.

@reosarevok
Copy link
Member Author

Ok then! I updated the PR to use the recursive query - it was breaking for LinkAttributeType because that has a weird _table, but I just hardcoded that one.

Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to remove the link type duplication in the future (the form validation can probably already be a role), but this works for now. :)

lib/MusicBrainz/Server/Data/Role/OptionsTree.pm Outdated Show resolved Hide resolved
@reosarevok
Copy link
Member Author

Updated this to use %ENTITIES. We could move the validation to a role indeed - the only reason not to might be that LinkAttributeType makes extra checks for instruments, but I guess that could be either a second after validate or a check for model inside the role. Which of the two sounds better?

@reosarevok
Copy link
Member Author

@brainzbot, retest this please

@yvanzo
Copy link
Contributor

yvanzo commented May 16, 2024

Didn’t review yet but this can be rebased after the pull request #2777 that you updated today.

We had these for some seemingly at random (I assume because
we happened to be using them somewhere).
I needed a bunch more for my next feature, but I thought I would
add all of themnow  to avoid having to add more batches in the future.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs that should be checked/fixed soonish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants