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

toJSON not verifiying ints are actualy ints #433

Closed
yoavmil opened this issue Dec 7, 2021 · 4 comments · Fixed by #444
Closed

toJSON not verifiying ints are actualy ints #433

yoavmil opened this issue Dec 7, 2021 · 4 comments · Fixed by #444
Labels

Comments

@yoavmil
Copy link

yoavmil commented Dec 7, 2021

If a proto has an int/uint, at the ts side it is number, therefor can get any floating point number. when trying to create a JSON from it, there is no error, or rounding to nearest int. and later on, in my case, when parsing it at a C++ server with the protof utils, it fails.

It will be best to make sure, that the data fits the data type. For me, a workaround was to encode & decode before writing the JSON.

Cheers.

@stephenh
Copy link
Owner

stephenh commented Dec 7, 2021

Ah yeah, that's a good point. Probably both fromJSON and fromPartial should do some runtime checking / conversion of number; we could probably have a int32Check(...)-type method(s) (like one per primitive type) added to Utils:

https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L279

So that they are conditionally included in the output (i.e. only if you're actually using that data type), around these line:

https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L996

https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L1225

If you'd like to submit a PR, that'd be great!

@yoavmil
Copy link
Author

yoavmil commented Dec 8, 2021

see here PR.
However, I cannot compile and check. just take a look.

note1: I also think there is need to change only the toJSON function, because the purpose is to make sure the output is valid for other services to read. Javascript is more forgiving, so it is ins't a must to validate the integers when reading from json or proto.

note2: no need to check the numbers when using encode, because the writer does the validation for you, I think.

note3: it might be smart to validate all the types, ex make sure no overflow for small types, signed/unsigned etc...

@stephenh
Copy link
Owner

@yoavmil I picked up your initial PR as #444 and merged that. FWIW I'm kinda wondering, in retrospect, if blowing up inside of toJSON would be better than silently dropping data. I get what you're saying, that for you having the C++/JSON parsing blow up wasn't helpful, and you can avoid that by rounding away numbers when putting them on the wire, put seems like a safer approach would be to have toJSON itself throw an error, and let the upstream codebase know that they have precision issues that need to be fixed by Math.rounds at appropriate points in the application codebase.

I still merged the Math.round approach for now, but at some point might move to throwing errors, just FYI/FWIW.

@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 1.94.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

2 participants