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

DecimalField fails when max_digits=None #4372

Closed
terramars opened this issue Aug 9, 2016 · 8 comments
Closed

DecimalField fails when max_digits=None #4372

terramars opened this issue Aug 9, 2016 · 8 comments

Comments

@terramars
Copy link

terramars commented Aug 9, 2016

Steps to reproduce

  1. Create a DecimalField inside a serializer with max_value, min_value, and decimal_places set, but set max_digits = None. EG: serializers.DecimalField(max_value=100, min_value=0, decimal_places=10, max_digits=None)
  2. call serializer.is_valid()
  3. observe traceback similar to the following :

Traceback (most recent call last):
File "/api/views.py", line 263, in monitor
serializer.is_valid()
File "
/env/local/lib/python2.7/site-packages/rest_framework/serializers.py", line 213, in is_valid
self._validated_data = self.run_validation(self.initial_data)
File "/env/local/lib/python2.7/site-packages/rest_framework/serializers.py", line 407, in run_validation
value = self.to_internal_value(data)
File "
/env/local/lib/python2.7/site-packages/rest_framework/serializers.py", line 437, in to_internal_value
validated_value = field.run_validation(primitive_value)
File "/env/local/lib/python2.7/site-packages/rest_framework/fields.py", line 488, in run_validation
value = self.to_internal_value(data)
File "
/env/local/lib/python2.7/site-packages/rest_framework/fields.py", line 959, in to_internal_value
return self.quantize(self.validate_precision(value))
File "~/env/local/lib/python2.7/site-packages/rest_framework/fields.py", line 1022, in quantize
context=context
File "/usr/lib/python2.7/decimal.py", line 2455, in quantize
if not (context.Etiny() <= exp._exp <= context.Emax):
File "/usr/lib/python2.7/decimal.py", line 3898, in Etiny
return int(self.Emin - self.prec + 1)
TypeError: unsupported operand type(s) for -: 'int' and 'NoneType'

Expected behavior

is_valid returns true if the value is between min and max, without concerning itself with the number of digits / precision of the number, OR do not accept None as a valid value for max_digits.

Actual behavior

an exception is raised, which is difficult to track down and counter intuitive.

@xordoquy
Copy link
Collaborator

xordoquy commented Aug 10, 2016

is_valid returns true if the value is between min and max, without concerning itself with the number of digits / precision of the number, OR do not accept None as a valid value for max_digits.

Nop. max_digits is a required argument. It shouldn't be None or ignored. I'd assume you'll get a faster traceback if you simply ignore it.

What I'd be willing to do here would be:

  • to add an explicit mention in the doc about the required fields.
  • to add an assert about max_digits not being None.

Closing this issue meanwhile as we're not making max_digit optional.

@terramars
Copy link
Author

terramars commented Aug 10, 2016

That makes sense, yeah assert when max_digits is None would be great.
Definitely easier for people to debug.

On Aug 9, 2016 11:53 PM, "Xavier Ordoquy" notifications@github.com wrote:

Closed #4372
#4372.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4372 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AE4PcV0S1msjrxb6c-2dJxbv8Gx6aFplks5qeXVtgaJpZM4Jgk4N
.

@tomchristie
Copy link
Member

tomchristie commented Aug 10, 2016

Taking a look through the code it's not clear if max_digits is intended to be optional or not. We do switch on if self.max_digits is None, and a DecimalField without digit restrictions is a reasonable enough use case. Worth putting on to the queue to have a second think over.

@xordoquy
Copy link
Collaborator

xordoquy commented Aug 10, 2016

@tomchristie max_digits is not a keyword argument in __init__ therefore it's supposed to be required. Same applies to decimal_field. This is likely inherited from Django's own DecimalField.
If it becomes an optional argument, we'll need to cover the case where it's over the Django's Model boundaries.

@tomchristie
Copy link
Member

tomchristie commented Aug 10, 2016

Passing the argument is required, but it may be None. None will have worked up until #4339, so figure it's worth resolving.

@tomchristie tomchristie changed the title DecimalField throws "TypeError: unsupported operand type(s) for -: 'int' and 'NoneType'" when max_digits is set to none (or unspecified) DecimalField fails when max_digits=None Aug 10, 2016
@xordoquy
Copy link
Collaborator

xordoquy commented Aug 10, 2016

I don't think it worked before as there are other calls to quantize anyway.

@tomchristie
Copy link
Member

tomchristie commented Aug 10, 2016

Comes across as a regression to me, tho yes slightly borderline behavior, but not massivly. Test looks happy now.

@bravmi
Copy link

bravmi commented Dec 24, 2020

From my tests and the source, it seems decimal_places is also allowed to be None but it is not mentioned in the docs.

Also, it'd be cool if the docs stated exactly what happens when either of them is None.

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

No branches or pull requests

4 participants