-
Notifications
You must be signed in to change notification settings - Fork 7
Fix group merging and linking #20
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| boolean fail = true; | ||
|
|
||
| // You have a freaking hashmap, use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peak
Diet-Cola
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only thing I could think of being extra nice would maybe be a group link event but otherwise, cool stuff
| "DROP TABLE faction_id", | ||
| "DROP TABLE subgroup", // don't use old values from subgroup as it has been disabled anyway | ||
| "ALTER TABLE faction ADD COLUMN (parent_id INT)", | ||
| // TODO: ON DELETE SET NULL or ON DELETE CASCADE? If the parent group is deleted, should the child group be unlinked or deleted too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my vote would be to orphan children, less destructive if parent delete was unintended or part of a reorganization
| mergeGroup.setString(1, groupName); | ||
| mergeGroup.setString(2, toMerge); | ||
|
|
||
| public boolean mergeGroup(int group, int toMerge){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that I think was a source of issue with merge & subgroups in particular is the possibility of complex group dynamics; two main complexities immediately come to mine.
I'm assuming single parent dynamics;
a -> b means a is subgroup of b.
So, imagine this starting state:
b -> a
c -> b
d -> c
e -> d
This is a simple chain.
Now, merge e with a. (might need to play with from / to). In the right direction, this will create a cycle, and is not currently checked.
This should be relatively simple to address; can simply use your cycle check code to see if ending state will result in a cycle and abort if it will.
The other is less about cycles and more about complexity a.la user error. (hinted at in the "play with from / to"). Permissions flow from parent to subgroup, and it may not be terribly obvious the connected graph and consequences of this. Accidentally orphaning or subsuming whole subsets of groups can result.
A second example:
b -> a
c -> b
d -> c
g -> f
h -> f
i -> g
Goal is to merge f with b, such that a is still the parent, and c, g, h are all subgroups of f/b.
so, FROM must be f; TO must be b, resulting in b remaining and f being deleted. It will be quite easy to mix that up, which will dehome c, d, from a as superparent, leaving a with no children and f with all the children.
Other more complex forms of this and cycle finding exist... although I suspect these are good, trivial forms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something, but I think the problem lies with permitting redirection-chains at all. I fully acknowledge the need for redirection-ids since updating every possible entry in the database, like reinforcements, anytime a group is merged is probably quite unfeasible, prone to error, and laggy.
So instead of having the following:
a
b -> a
c -> b
d -> c
e -> d
You'd instead have:
a
b -> a
c -> a
d -> a
e -> a
And if a were to be merged into f, the the following would occur:
a -> f
b -> f
c -> f
d -> f
e -> f
f
That way, group merging is kept in-house, you're not passing the responsibility to ensure group-id integrity onto users of NameLayer like Citadel and JukeAlert. You're also reducing the magnitude of queries on the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something was lost in translation? The way it currently works, subgroups can have subgroups. So, there's no redirection chains, just "a group can have subgroups". This subgroup chaining just has natural graph theory consequences, described previously.
Now, perhaps you are suggesting that instead all attempts to establish a group-subgroup relationship should link the candidate subgroup to the "highest" parent group (in which case, there could never be loops, and group / subgroup relationships would only ever be one deep). This would indeed avoid all the problems described, as single-depth subgrouping (no children of children allowed!) is a very nicely described sub-problem that should be easy to code protections for.
I'm definitely not opposed to that, as it probably would satisfy some 99% of the use-cases and reduces the complexity of any solution by several orders of magnitude!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was just referring to merging, as in combining one group into another. My bad.
ProgrammerDan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are well structured, and I love the removal of groupname legacy. Lots of upstream changes potentially?
The big concerns to me are dealing with merges that then introduce cycles, and reducing the complexity for end users in understanding the "result" of a merge or subgroup. Those were what motivated pulling it in the past, not the multiple-ids problem, which was a big, but separate problem I'm happy to see refactored out here.
|
I definitely think the GUI could use some improvements to change how linked groups are visualised, so I can make some improvements there as well. |
|
After discussion with Orinnari, I think the best approach here is to remove merging as the implementation is simply too complex, and it can be wholly replaced by group linking. So, I will change this PR to just fix linking and remove merging. |
|
Mulling this over, am I mistaken in thinking that the subgroup/linking issue could be [largely] resolved by banning the concept of subgroup-subgroups altogether? You could impose such a restriction fairly straightforwardly and efficiently both in-code and in-database by asserting that the parent's parent is null. The only issue I can foresee, though I am fairly tired, is attempting to link a parent-group to another group, eg: I'd therefore propose that the former parent ("Ashelor-Public") become a sibling-subgroup, like so: You can then have APIs to change a group's parent, or to 'emancipate' a subgroup from its parent. |
WIP, do not merge. Other plugins will need changes for this to work.