-
- Notifications
You must be signed in to change notification settings - Fork 233
fix(core): Improve CustomInt and CustomInt64 UnmarshalJSON robustness #2431
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: 666Goofy666 <153852747+666Goofy666@users.noreply.github.com>
Summary of ChangesHello @666Goofy666, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the resilience of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request improves the robustness of UnmarshalJSON for CustomInt and CustomInt64 by adding a fallback to parse scientific notation and floating-point numbers. This is a good improvement to handle large values from the Proxmox API.
I've added a couple of suggestions:
- For
CustomInt64, I've recommended usingmath/big.Floatto avoid potential precision loss when parsing very large numbers that are represented in scientific notation. This is important for maintaining data integrity for values like datastore sizes. - For
CustomInt, I've suggested a small change to make the integer parsing more portable across different platform architectures (32-bit vs 64-bit).
The added unit tests are comprehensive and cover many edge cases, which is great. The acceptance test for the datastores data source is also a good addition. Overall, this is a solid contribution.
Using math/big.Float for parsing prevent precision loss, and use a bit size of 0, which will make ParseInt use the platform's native integer size. Signed-off-by: 666Goofy666 <153852747+666Goofy666@users.noreply.github.com>
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.
Code Review
This pull request improves the robustness of CustomInt and CustomInt64 UnmarshalJSON methods by adding support for scientific notation, which is a great improvement. The implementation using math/big is solid, and the new unit tests provide good coverage. I've added one suggestion to make the tests for CustomInt architecture-independent, ensuring they run correctly on both 32-bit and 64-bit systems.
| Thank you for the summary! I'm now proceeding with a detailed review of the changes and will provide my feedback shortly. |
Signed-off-by: 666Goofy666 <153852747+666Goofy666@users.noreply.github.com>
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.
Code Review
This pull request addresses an issue with unmarshaling large integer values from the Proxmox API that are represented in scientific notation. The changes in CustomInt and CustomInt64 UnmarshalJSON methods are robust, correctly handling this by falling back to big.Float for parsing. The addition of comprehensive unit tests is excellent and ensures the correctness of the fix across various numeric formats and edge cases. I have one minor suggestion to improve a comment for better clarity.
| | ||
| *r = CustomInt64(i) | ||
| // If that fails, numbers may be provided in scientific notation or as floats. | ||
| // Parse as float64 and convert to int64 (truncate towards zero). |
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.
The comment mentions parsing as float64, but the implementation correctly uses big.Float for arbitrary-precision parsing. Using float64 would lead to precision loss for large integers, which this change is meant to handle. To avoid confusion for future maintainers, it would be clearer to update the comment to reflect that a high-precision float is used.
| // Parse as float64 and convert to int64 (truncate towards zero). | |
| // Parse as a high-precision float and convert to int64 (truncating towards zero). |
Contributor's Note
/docsfor any user-facing features or additions./fwprovider/testsfor any new or updated resources / data sources.make exampleto verify that the change works as expected.Note
Feel free to change the code, especially the tests. QA and tests are always stronger when written by someone other than the original author. I just needed something to validate my functions, so improvements are welcome!
The Proxmox API returns very large datastore sizes (petabytes) in scientific notation, which was breaking unmarshaling into
CustomInt64. This PR adds support for those scientific notation values.Proof of Work
Make a fake big datastore :
In GUI :
Datacenter → Storage → Add → Directory / Directory : /mnt/fake_big_xfsBefore patch I reproduce issue : #2298
After the patch, parsing large storage values looks correct: (
make example)For comparison :
Community Note
Closes #0000 | Relates #0000