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

Random.Decimal with min = 0, max = decimal.MaxValue overflows #319

Closed
Tvel opened this issue Aug 14, 2020 · 6 comments
Closed

Random.Decimal with min = 0, max = decimal.MaxValue overflows #319

Tvel opened this issue Aug 14, 2020 · 6 comments
Labels

Comments

@Tvel
Copy link

Tvel commented Aug 14, 2020

Version Information

Software Version(s)
Bogus NuGet Package 30.0.3
.NET Core? 3.1
.NET Full Framework?
Windows OS? 10
Linux OS?
Visual Studio?

What locale are you using with Bogus?

en-us

What is the expected behavior?

before v30 faker.Random.Decimal(0m, decimal.MaxValue) returned a value.

What is the actual behavior?

System.OverflowException
Value was either too large or too small for a Decimal.
at System.Number.ThrowOverflowException(TypeCode type)
at System.Decimal.DecCalc.ScaleResult(Buf24* bufRes, UInt32 hiRes, Int32 scale)
at System.Decimal.DecCalc.VarDecMul(DecCalc& d1, DecCalc& d2)
at System.Decimal.op_Multiply(Decimal d1, Decimal d2)
at Bogus.Randomizer.Decimal(Decimal min, Decimal max)

Can you identify the location in Bogus' source code where the problem exists?

https://github.com/bchavez/Bogus/blob/master/Source/Bogus/Randomizer.cs#L195
As I understand there was a change in the randomiser to make double values more precise.

If the bug is confirmed, would you be willing to submit a PR?

Yes

@bchavez
Copy link
Owner

bchavez commented Aug 14, 2020

Hi Tosil,

I think you might be correct. The recent Randomizer changes for decimal should be associated with PR #300.

If you could help with a new PR that fixes the OverflowException that would be wonderful, if not I will fix the issue tonight. A unit test that covers this OverflowException and maybe other edge cases would be great too.

Also, @logiclrd if you have any suggestions or comments on the issue, please let us know.

Thanks for bringing this to our attention.

Brian

@bchavez bchavez added the bug label Aug 14, 2020
@Tvel
Copy link
Author

Tvel commented Aug 14, 2020

Hi Brian

I am not sure what is the best way to fix the overflow and I will not be able to work on this until at least monday, so if you or someone else want to fix it go ahead :). I am happy to help with unit tests or testing later.

Thanks!

@logiclrd
Copy link
Contributor

I see what the problem is. Still thinking about how to correctly solve it, but the problem is that it's possible for the product of the initial result and (max - min) to exceed decimal.MaxValue.

@logiclrd
Copy link
Contributor

(And yes, I wrote that code, so I ought to fix it :-D My apologies)

logiclrd added a commit to logiclrd/Bogus that referenced this issue Aug 14, 2020
…l_with_very_large_range_succeeds to Bogus.Tests.

Updated Randomizer.Decimal to make the test pass.
@bchavez
Copy link
Owner

bchavez commented Aug 15, 2020

@Tvel , @logiclrd

For the time being, I released Bogus v30.0.4 which Random.Decimal is reverted to a previous v29 implementation. So, you should be unblocked @Tvel while we work on a proper fix in PR #320.

The previous Random.Decimal code that throws the OverflowException was moved to the Bogus.Extensions namespace called Random.Decimal2() in case you still need the implementation that generates greater precision; however Decimal2() extension method will still throw OverflowException in the edge case you've discovered until we've fully worked out the details in #320.

Hope this helps.

Thanks,
Brian

@bchavez bchavez closed this as completed Aug 15, 2020
@Tvel
Copy link
Author

Tvel commented Aug 16, 2020

Thanks for the quick work from both of you!

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

No branches or pull requests

3 participants