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

ensure numbers where we expected booleans parse correctly. #1108

Merged
merged 3 commits into from
Dec 8, 2024

Conversation

456dev
Copy link
Contributor

@456dev 456dev commented Sep 9, 2024

Boolean.parseBoolean expects the literal "true", which doesn't match the number 1.

The fix is to parse "1" as truthy, and all other values as falsy, like Boolean.parseBoolean() does.

fixes PaperMC/Velocity#1286

I'm assuming its meant to accept "1" as valid, seeing as it was specifically peeking for Numbers.
In velocity at least, "1" in json is emitted by the nbt component converter, from bytes

Boolean.parseBoolean expects the literal "true", which doesn't match the number 1.

The fix is to parse "1" as truthy, and all other values as falsy, like Boolean.parseBoolean() does.

fixes PaperMC/Velocity#1286
@456dev
Copy link
Contributor Author

456dev commented Sep 9, 2024

Oh, looks like it might be intentional, if there is a test for it.

does that just make this velocities problem?

@zml2008
Copy link
Member

zml2008 commented Sep 9, 2024

I think this is a format change from since component serialization started using Codecs? so it's a case of adventure's tests being outdated against the newly changed spec.

@456dev
Copy link
Contributor Author

456dev commented Sep 9, 2024

I'm still not 100% sure if this should change, as looking at the codecs etc, the vanilla jsonOps only parses / emits standard json bools, so this is more permissive than how minecraft behaves with json.
(only nbt ops parses bools using bytes i think)

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

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

Assuming this is too minor a change to bother with a serialization option here, LGTM.

@zml2008 zml2008 self-assigned this Dec 8, 2024
@zml2008 zml2008 added this pull request to the merge queue Dec 8, 2024
Merged via the queue into KyoriPowered:main/4 with commit 97b1abe Dec 8, 2024
3 checks passed
@456dev 456dev deleted the fix/gson-decode-number branch December 9, 2024 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to parse Component from Json (packet) correctly
3 participants