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

min_value/max_value support in DurationField #5643

Merged
merged 3 commits into from Apr 24, 2018

Conversation

noamkush
Copy link
Contributor

@noamkush noamkush commented Dec 1, 2017

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
Copy link
Collaborator

xordoquy commented Dec 1, 2017

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

@noamkush
Copy link
Contributor Author

noamkush commented Dec 1, 2017

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

Copy link
Collaborator

@xordoquy xordoquy left a comment

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)])

'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,
Copy link
Collaborator

@xordoquy xordoquy Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

# Number types...

class IntegerField(Field):
class _MaxMinValueField(Field):
Copy link
Collaborator

@xordoquy xordoquy Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@noamkush
Copy link
Contributor Author

noamkush commented Dec 1, 2017

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

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 1, 2017

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
Copy link
Contributor Author

noamkush commented Dec 1, 2017

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
rpkilby approved these changes Dec 4, 2017
Copy link
Member

@rpkilby rpkilby left a comment

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

Copy link
Collaborator

@carltongibson carltongibson left a comment

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?

# 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.)

Copy link
Collaborator

@carltongibson carltongibson left a comment

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
Copy link
Contributor Author

noamkush commented Feb 22, 2018

Looks great to me

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

# Number types...

class IntegerField(Field):
class MaxMinMixin(object):
Copy link
Member

@tomchristie tomchristie Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's not push behavior into mixin classes.

@tomchristie
Copy link
Member

tomchristie commented Feb 22, 2018

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

@carltongibson
Copy link
Collaborator

carltongibson commented Feb 22, 2018

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

@carltongibson carltongibson added this to the 3.8.3 Release milestone Apr 24, 2018
@carltongibson
Copy link
Collaborator

carltongibson commented Apr 24, 2018

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

@carltongibson carltongibson merged commit 7268643 into encode:master Apr 24, 2018
1 check passed
@rpkilby rpkilby modified the milestones: 3.8.3 Release, 3.9 Release Aug 29, 2018
@noamkush noamkush deleted the duration-minmax branch Mar 18, 2019
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Added min_value/max_value field arguments to DurationField.
* Made field mapping use mix/max kwargs for DurationField validators.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants