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

Fix incorrect command node serialization by creating new Command #11671

Merged
merged 1 commit into from
Dec 8, 2024

Conversation

Rickyboy320
Copy link
Contributor

Fixes #11649 - As noted in the issue, when CommandNodes are serialized they are used as the key in a Map. Their equals()/hashcode() should only match if they are equal nodes (name & command), but due to the erasure of the command field pre-serialization, nodes with different commands can be mapped onto the same value. This causes the client to interpret both nodes as the same, causing suggestions where they should not.

This is fixed by creating a different no-op Command for the erasure, instead of them holding the same lambda.

@Rickyboy320 Rickyboy320 requested a review from a team as a code owner November 26, 2024 19:59
Fixes PaperMC#11649 - As noted in the issue, when CommandNodes are serialized
they are used as the key in a Map. Their equals()/hashcode() should only    match if they are equal nodes (name & command), but due to the erasure of the command field pre-serialization, nodes with different commands can be mapped onto the same value. This causes the client to interpret both nodes as the same, causing suggestions where they should not.

This is fixed by creating a different no-op command for the
erasure, instead of them holding the same lambda.
@Owen1212055 Owen1212055 force-pushed the fix/incorrect-suggestions branch from 0aa738c to 1240059 Compare December 8, 2024 21:38
@Owen1212055 Owen1212055 merged commit 07ef2bb into PaperMC:master Dec 8, 2024
3 checks passed
@Machine-Maker
Copy link
Member

This fix has created an issue. It breaks the compactness serialized size of the commands. For any plugin that uses "flat" aliases (aliases that just shallow copy the root literal), it now doubles the serialized size of the command tree instead of re-using the references to the children command nodes. This is because the hashmap that tracks a serialization id for each CommandNode no longer finds existing nodes because the Command instances are not equal.

@Rickyboy320
Copy link
Contributor Author

I suggested another approach in the related issue, where we keep the original command in the node, so that the equality of the fake node matches the original node (and other copies thereof). I'm not sure what side-effects this has, however, since it seems like the command field is intentionally wiped

@lynxplay
Copy link
Contributor

Yea we are presumably going to opt towards that approach down the line to avoid the tree reduction from failing as hard as it has been 👍 The approach here worked well enough, it was only ever exposed as an issue cause once a couple of other bugs were fixed as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Brigadier: Incorrect serialization of same-name command nodes causing client to suggest invalid literals
4 participants