min_value/max_value support in DurationField #5643

noamkush · 2017-12-01T15:13:44Z

This adds min_value/max_value to DurationField, by splitting the code for min_value/max_value from Integer/Float/Decimal fields into a shared base class and having DurationField also inherit it.
I think it's useful for cases such as accepting only positive/negative durations or accepting at least a minute, at least an hour etc.

xordoquy · 2017-12-01T15:18:24Z

Can't we rather use validators ?
I'm pretty confident this can already be done with Django's MaxValueValidator and MinValueValidator.

noamkush · 2017-12-01T15:20:29Z

That's what the existing code in integer/float/decimal already used. I just moved it to a base class.

xordoquy

Getting the validators generation is a nice move.
OTOH I'm not sure I prefer

DurationField(min_value=datetime.timedelta(days=1), max_value=datetime.timedelta(days=4))

over

DurationField(validators=[MinValueValidator(datetime.timedelta(days=1), message=message), MaxValueValidator(datetime.timedelta(days=4), message=message)])

xordoquy · 2017-12-01T15:26:11Z

rest_framework/fields.py

        'max_digits': _('Ensure that there are no more than {max_digits} digits in total.'),
        'max_decimal_places': _('Ensure that there are no more than {max_decimal_places} decimal places.'),
        'max_whole_digits': _('Ensure that there are no more than {max_whole_digits} digits before the decimal point.'),
        'max_string_length': _('String value too large.')
    }
    MAX_STRING_LENGTH = 1000  # Guard against malicious string inputs.

-    def __init__(self, max_digits, decimal_places, coerce_to_string=None, max_value=None, min_value=None,
+    def __init__(self, max_digits, decimal_places, coerce_to_string=None,


That is an API change. I'd rather keep the current signature as it is.

xordoquy · 2017-12-01T15:26:51Z

rest_framework/fields.py

@@ -903,20 +903,16 @@ def to_internal_value(self, data):

 # Number types...

-class IntegerField(Field):
+class _MaxMinValueField(Field):


I'd rather it be a MaxMinMixin instead of a base class.

noamkush · 2017-12-01T16:13:38Z

IMHO, you can say the same about FloatField, I don't see the difference.

xordoquy · 2017-12-01T16:25:11Z

IMHO, you can say the same about FloatField, I don't see the difference.

agreed, which is why I haven't closed it already :)

noamkush · 2017-12-01T23:38:20Z

Let me put it this way:
DurationField is probably more rare than most fields, but when you use it you would probably want to make sure the values you get are sane, and the most common way to do that is with max/min, so you won't get negative values if you need positive or maybe you don't want to accept a 1,000 years but 10 days is okay. This is the most (and maybe only) reasonable validation for the DurationField, so why not make it standard instead of having to write it for every such field?

rpkilby

I'm +1 on adding min/max handling to DurationField. Also a +1 on factoring out the relevant min/max validation handling.

carltongibson

There's an extra part to this. In rest_framework.utils.field_mapping.get_field_kwargs, do we want to be ensuring max_value and min_value are passed as kwargs rather than validators, as we do for other fields?

django-rest-framework/rest_framework/utils/field_mapping.py

Lines 128 to 152 in daba5e9

    
           # Ensure that max_value is passed explicitly as a keyword arg, 
        
           # rather than as a validator. 
        
           max_value = next(( 
        
               validator.limit_value for validator in validator_kwarg 
        
               if isinstance(validator, validators.MaxValueValidator) 
        
           ), None) 
        
           if max_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES): 
        
               kwargs['max_value'] = max_value 
        
               validator_kwarg = [ 
        
                   validator for validator in validator_kwarg 
        
                   if not isinstance(validator, validators.MaxValueValidator) 
        
               ] 
        
           # Ensure that min_value is passed explicitly as a keyword arg, 
        
           # rather than as a validator. 
        
           min_value = next(( 
        
               validator.limit_value for validator in validator_kwarg 
        
               if isinstance(validator, validators.MinValueValidator) 
        
           ), None) 
        
           if min_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES): 
        
               kwargs['min_value'] = min_value 
        
               validator_kwarg = [ 
        
                   validator for validator in validator_kwarg 
        
                   if not isinstance(validator, validators.MinValueValidator) 
        
               ]

(Not yet sure what I think about the change in general: if we go for it the proposed changes seem OK.)

carltongibson

Hi @noamkush. This looks great.

IMHO, you can say the same about FloatField, I don't see the difference.

I think this is what swings it. 🙂

I've squashed your commits and added DurationField to have the min/max kwargs set from any validators in field_mapping. (This is the same as we apply to the other numeric fields.)

Please take a look to make sure it looks as expected.

Thanks for the effort!

noamkush · 2018-02-22T14:47:10Z

Looks great to me

tomchristie · 2018-02-22T15:03:46Z

rest_framework/fields.py

@@ -903,20 +903,16 @@ def to_internal_value(self, data):

 # Number types...

-class IntegerField(Field):
+class MaxMinMixin(object):


Please let's not push behavior into mixin classes.

tomchristie · 2018-02-22T15:04:48Z

I'm okay with us adding this behavior in, but I'd rather we replicated code than used mixin classes.

carltongibson · 2018-02-22T15:41:32Z

Hi @noamkush — fancy pulling out the mixin and just applying the change to DurationField? Ta!

…hem to DurationField. PR fixes. Python 2 fix.

carltongibson · 2018-04-24T07:23:24Z

I rebased and pulled out the mixin. We'll take this now. Thanks for the input @noamkush!

* Added min_value/max_value field arguments to DurationField. * Made field mapping use mix/max kwargs for DurationField validators.

xordoquy reviewed Dec 1, 2017

View reviewed changes

rpkilby approved these changes Dec 4, 2017

View reviewed changes

carltongibson reviewed Dec 4, 2017

View reviewed changes

carltongibson force-pushed the duration-minmax branch from be1c751 to e32736d Compare February 22, 2018 14:24

carltongibson approved these changes Feb 22, 2018

View reviewed changes

carltongibson force-pushed the duration-minmax branch from e32736d to 77200f6 Compare February 22, 2018 14:34

tomchristie reviewed Feb 22, 2018

View reviewed changes

noamkush and others added 3 commits April 24, 2018 09:01

Split min_value/max_value field arguments to a base class and added t…

a4c9e4c

…hem to DurationField. PR fixes. Python 2 fix.

Made field mapping use mix/max kwargs for DurationField validators.

d1822ec

Remove mixin

4537444

carltongibson force-pushed the duration-minmax branch from 77200f6 to 4537444 Compare April 24, 2018 07:12

carltongibson added this to the 3.8.3 Release milestone Apr 24, 2018

carltongibson merged commit 7268643 into encode:master Apr 24, 2018

rpkilby modified the milestones: 3.8.3 Release, 3.9 Release Aug 29, 2018

noamkush deleted the duration-minmax branch March 18, 2019 13:01

	# Ensure that max_value is passed explicitly as a keyword arg,
	# rather than as a validator.
	max_value = next((
	validator.limit_value for validator in validator_kwarg
	if isinstance(validator, validators.MaxValueValidator)
	), None)
	if max_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES):
	kwargs['max_value'] = max_value
	validator_kwarg = [
	validator for validator in validator_kwarg
	if not isinstance(validator, validators.MaxValueValidator)
	]

	# Ensure that min_value is passed explicitly as a keyword arg,
	# rather than as a validator.
	min_value = next((
	validator.limit_value for validator in validator_kwarg
	if isinstance(validator, validators.MinValueValidator)
	), None)
	if min_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES):
	kwargs['min_value'] = min_value
	validator_kwarg = [
	validator for validator in validator_kwarg
	if not isinstance(validator, validators.MinValueValidator)
	]

Uh oh!

min_value/max_value support in DurationField #5643

min_value/max_value support in DurationField #5643

Uh oh!

Conversation

noamkush commented Dec 1, 2017

Uh oh!

xordoquy commented Dec 1, 2017

Uh oh!

noamkush commented Dec 1, 2017

Uh oh!

xordoquy left a comment

Choose a reason for hiding this comment

Uh oh!

xordoquy Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

xordoquy Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

noamkush commented Dec 1, 2017

Uh oh!

xordoquy commented Dec 1, 2017

Uh oh!

noamkush commented Dec 1, 2017

Uh oh!

rpkilby left a comment

Choose a reason for hiding this comment

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

noamkush commented Feb 22, 2018

Uh oh!

tomchristie Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

tomchristie commented Feb 22, 2018

Uh oh!

carltongibson commented Feb 22, 2018

Uh oh!

carltongibson commented Apr 24, 2018

Uh oh!

Uh oh!