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/MaxValueValidator is no longer transferred from a model's DecimalField #3774

Merged
merged 4 commits into from Jan 4, 2016

Conversation

kevin-brown
Copy link
Member

@kevin-brown kevin-brown commented Dec 24, 2015

After upgrading from DRF 3.2.5 to 3.3.2, a bunch of our tests started failing because the min_value and max_value are no longer being transferred over to the automatically generated fields. This is a regression that does not appear to be documented anywhere, and there is still code that is supposed to be setting these arguments.

  • Create regression test demonstrating the issue
  • Fix the issue

This adds tests for a regression where the `min_value` and `max_value`
arguments are not being set for a DRF `DecimalField` even though the
corresponding `MinValueValidator` and `MaxValueValidator` is being set
on the model fields.

Note that this only appears to be a regression for Django < 1.9, as
these regression tests pass on newer versions of Django.
kevin-brown added 2 commits Dec 24, 2015
Previously, all validators set on a DecimalField in Django would be
stripped when converted to a Django REST framework field. This was
because any validator that was an instance of `DecimalValidator` would
be removed, and when `DecimalValidator` wasn't supported (so it was
`None`), all validators would be removed.

This fixes the issue by only removing the `DecimalValidator` instances
if the `DecimalValidator` is supported.
This test was incorrectly checking that there were no validators set in
older versions of Django, even though it should have been checking for
the two validators that were set up on the model field level.

The originally regression test that this fixes was added in
7d79cf3
when fixing an issue with the `DecimalValidator`.
@xordoquy xordoquy added the Bug label Dec 25, 2015
@xordoquy xordoquy added this to the 3.3.3 Release milestone Dec 25, 2015
@kevin-brown
Copy link
Member Author

kevin-brown commented Dec 27, 2015

@jpadilla Do you think it would be reasonable to merge the two regression tests you added in 7d79cf3 now that we have discovered that there actually are not two separate cases?

@jpadilla
Copy link
Member

jpadilla commented Dec 28, 2015

@kevin-brown yep def reasonable, good work hunting this down.

@tomchristie
Copy link
Member

tomchristie commented Jan 4, 2016

Nice work 👍

@@ -130,7 +130,7 @@ def get_field_kwargs(field_name, model_field):

# Our decimal validation is handled in the field code, not validator code.
# (In Django 1.9+ this differs from previous style)
if isinstance(model_field, models.DecimalField):
if isinstance(model_field, models.DecimalField) and DecimalValidator:
validator_kwarg = [
validator for validator in validator_kwarg
if DecimalValidator and not isinstance(validator, DecimalValidator)
Copy link
Collaborator

@xordoquy xordoquy Jan 4, 2016

Choose a reason for hiding this comment

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

We could probably remove the check against DecimalValidator at line 136 since it's redondant with the one line 133.

Edit: "the check against DecimalValidator" meaning "if DecimalValidator and not isinstance(validator, DecimalValidator)" -> if not isinstance(validator, DecimalValidator)

These two tests were previously added in
7d79cf3
but we have now discovered that there are not actually two separate
cases, there was just a  bug in the code that made it look that way.

This also removes a redundant check to see if `DecimalValidator` was
defined.
@kevin-brown kevin-brown force-pushed the decimalfield_validators branch from 294a183 to a772326 Compare Jan 4, 2016
xordoquy added a commit that referenced this pull request Jan 4, 2016
Min/MaxValueValidator is no longer transferred from a model's DecimalField
@xordoquy xordoquy merged commit dceb686 into master Jan 4, 2016
3 checks passed
@xordoquy
Copy link
Collaborator

xordoquy commented Jan 4, 2016

@kevin-brown \o/ thanks a lot !

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

Successfully merging this pull request may close these issues.

None yet

4 participants