Skip to content

Refuse to merge a forwarding node in addChild instead of dropping its redirect#164

Open
WouterGritter wants to merge 1 commit into
Mojang:masterfrom
WouterGritter:fix-addchild-silently-drops-redirect-on-merge
Open

Refuse to merge a forwarding node in addChild instead of dropping its redirect#164
WouterGritter wants to merge 1 commit into
Mojang:masterfrom
WouterGritter:fix-addchild-silently-drops-redirect-on-merge

Conversation

@WouterGritter

Copy link
Copy Markdown

CommandNode#addChild merges onto an existing same-named child by carrying over only the command and grandchildren:

final CommandNode<S> child = children.get(node.getName());
if (child != null) {
  // We've found something to merge onto
  if (node.getCommand() != null) {
      child.command = node.getCommand();
  }
  for (final CommandNode<S> grandchild : node.getChildren()) {
      child.addChild(grandchild);
  }
}

The incoming node's redirect, modifier and forks are final, so they're not merged, they're silently dropped. Registering a redirecting/forking literal under a name that's already present therefore yields a node that quietly loses its forwarding behavior (or, the reverse, grandchildren get merged onto an existing redirect leaf which the builder itself forbids via then()).

A genuine field-level merge isn't viable: requirement defaults to a fresh s -> true lambda on every builder and can't be compared, so an equality-based conflict check would also break the legitimate grandchild-merge case (testAddChildMergesGrandchildren). redirect/modifier/forks are different; they default to null/null/false and are only set deliberately through forward(), and the builder already treats forwarding and children as mutually exclusive (then() and forward() both throw).

So addChild now fails fast when a merge would involve a forwarding node on either side, mirroring those existing builder guards, instead of silently discarding the config:

if (isForwarding(node) || isForwarding(child)) {
  throw new IllegalArgumentException("Cannot merge a node '" + node.getName()
      + "' that redirects or forks, as its redirect/modifier/fork would be lost");
}

private boolean isForwarding(final CommandNode<S> node) {
  return node.getRedirect() != null || node.getRedirectModifier() != null || node.isFork();
}

Adding a redirect/fork as a brand-new child (the normal case) is unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant