fixing issue with django rest framework when using MinMoneyValidator/MaxMoneyValidator

[hosamhamdy258]

issue : MinMoneyValidator or MaxMoneyValidator with ModelSerializer
error : Decimal object has no attribute amount
solution : getting default currency from source field on django model
expiation : default_currency is not passed to MoneyField Serializer

[benjaoming]

LGTM! To elaborate: This is fine to me, even without a test. It lives in the .contrib space so I wouldn't require a test unless @hosamhamdy258 you want to add one? :)

[Stranger6667]

I'd ask for a test that demonstrates the issue, so we can be sure we have all the use cases in our test suite so we don't break it accidentally in the future.

[benjaoming]

@hosamhamdy258 would you be able to provide a test case for this?

[errietta]

Hello @benjaoming , we are also hitting this issue. What do we need in order to get this PR merged? Do you want me to write a test case since it's been a few months?

[hosamhamdy258]

sorry for being late i would like to get your help to make this fix and test cases
@benjaoming @errietta and anyone who's facing same issue too

[hosamhamdy258]

Hello @benjaoming , we are also hitting this issue. What do we need in order to get this PR merged? Do you want me to write a test case since it's been a few months?

go ahead would be so appreciated

[benjaoming]

@errietta the test case is most welcome and will make this PR merged and very likely also go into a quick little patch release 👍 🙏

[errietta]

@benjaoming Can you help here? I'm trying to run the tests even against main with make test and I get the following

Traceback (most recent call last):
  File "/home/erry/django-money/setup.py", line 58, in <module>
    setup(
  File "/home/erry/django-money/venv/lib/python3.12/site-packages/setuptools/__init__.py", line 104, in setup
    return distutils.core.setup(**attrs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/erry/django-money/venv/lib/python3.12/site-packages/setuptools/_distutils/core.py", line 184, in setup
    return run_commands(dist)
           ^^^^^^^^^^^^^^^^^^
  File "/home/erry/django-money/venv/lib/python3.12/site-packages/setuptools/_distutils/core.py", line 200, in run_commands
    dist.run_commands()
  File "/home/erry/django-money/venv/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 969, in run_commands
    self.run_command(cmd)
  File "/home/erry/django-money/venv/lib/python3.12/site-packages/setuptools/dist.py", line 967, in run_command
    super().run_command(command)
  File "/home/erry/django-money/venv/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
    cmd_obj.run()
  File "/home/erry/django-money/venv/lib/python3.12/site-packages/setuptools/command/test.py", line 223, in run
    self.run_tests()
  File "/home/erry/django-money/setup.py", line 25, in run_tests
    errno = pytest.main(self.pytest_args)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/erry/django-money/.eggs/pytest-8.2.0-py3.12.egg/_pytest/config/__init__.py", line 159, in main
    config = _prepareconfig(args, plugins)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/erry/django-money/.eggs/pytest-8.2.0-py3.12.egg/_pytest/config/__init__.py", line 335, in _prepareconfig
    raise TypeError(msg.format(args, type(args)))
TypeError: `args` parameter expected to be a list of strings, got: '--cov=./djmoney tests' (type: <class 'str'>)
make: *** [Makefile:31: test] Error 1

[benjaoming]

@errietta hmm, that could be a missing pytest plugin. Try to run pip install -e '.[test]' and see if that helps.

[benjaoming]

I can see that we have some issues in the stack. We need to upgrade pytest to work with the latest pytest-django. And that somehow triggers the above error.

[errietta]

@benjaoming I'm not sure where to push but here is a test that fails in main.

class TestMinValueSerializer:
    def test_serializer_validators(self):
        from djmoney.contrib.django_rest_framework import MoneyField

        class MinValueSerializer(serializers.Serializer):
            field = MoneyField(
                decimal_places=2,
                max_digits=10,
                min_value=0,
            )


        serializer = MinValueSerializer(data={"field": -10})
        assert not serializer.is_valid()
        print(serializer.errors)
        assert serializer.errors["field"][0] == "Ensure this value is more or equal to 0."

You can also do it this way and it's the same issue

class TestMinValueSerializer:
    def test_serializer_validators(self):
        from django.core.validators import MinValueValidator
        from djmoney.contrib.django_rest_framework import MoneyField

        class MinValueSerializer(serializers.Serializer):
            field = MoneyField(
                decimal_places=2,
                max_digits=10,
                validators=[MinValueValidator(Money(0, 'Eur'))]
            )


        serializer = MinValueSerializer(data={"field": -10})
        assert not serializer.is_valid()
        print(serializer.errors)
        assert serializer.errors["field"][0] == "Ensure this value is more or equal to 0."

.. I wonder if this issue only manifests because the minimum is 0?

I got the tests to run with tox against main. But for some reason they are not passing on this branch either. THat's weird, because I literally copy-pasted this code from this branch and got it to wrok without errors on my app.
Because I had issues running the tests, can you please try this test case with this branch and see if it works?

[errietta]

The failure:

self = <djmoney.models.validators.MinMoneyValidator object at 0xf4e5bc50>
cleaned = Decimal('-10.00')

    def get_limit_value(self, cleaned):
        if isinstance(self.limit_value, Money):
            if cleaned.currency.code != self.limit_value.currency.code:
                return
            return self.limit_value
        elif isinstance(self.limit_value, (int, Decimal)):
            return self.limit_value
        try:
>           return Money(self.limit_value[cleaned.currency.code], cleaned.currency.code)
E           AttributeError: 'decimal.Decimal' object has no attribute 'currency'

[errietta]

@benjaoming Digging further, the other thing I'm missing here is default_currency. If I give it explicitly in the serializer, this test passes even against main. However, that should not be required. Then, I unfortunately don't know how I can make a default_currency django setting in the test case like I have in my real app. So I'm not sure how to get this test to pass on this branch without explicit default_currency, but theoretically it would pass if the default_currency django setting was there.

[benjaoming]

@errietta you should be able to do that with the @override_settings(DEFAULT_CURRENCY=...) decorator applied to the test case.

[benjaoming]

@errietta you can try to merge in the latest main, there are some fixes to tests and pytest got upgraded.

[errietta]

@benjaoming here is a test that fails on main, but works on this branch

class TestMinValueSerializer:
    def test_serializer_validators(self):
        from djmoney.contrib.django_rest_framework import MoneyField

        class MinValueSerializer(serializers.Serializer):
            money = MoneyField(decimal_places=2, max_digits=10, min_value=0)

            class Meta:
                model = BaseModel

        serializer = MinValueSerializer(data={"money": Decimal(-10.00)})
        assert not serializer.is_valid()
        print(serializer.errors)
        assert serializer.errors["money"][0] == "Ensure this value is greater than or equal to 0."

again sorry for just pasting here but I don't have commit permissions neither in the original repo nor the fork
The only caveat is for this to work there needs to be a Model attached and the model needs to have a default currency...

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 90.60%. Comparing base (2370575) to head (16b2742).
Report is 10 commits behind head on main.

Files	Patch %	Lines
djmoney/contrib/django_rest_framework/fields.py	0.00%	6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #722      +/-   ##
==========================================
- Coverage   95.85%   90.60%   -5.25%     
==========================================
  Files          28       28              
  Lines         964      969       +5     
  Branches      166      160       -6     
==========================================
- Hits          924      878      -46     
- Misses         29       78      +49     
- Partials       11       13       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[errietta]

@benjaoming I see you did not add the latest version sorry :'0

[benjaoming]

yes it's your latest test case, I just updated a few things

[errietta]

@benjaoming the thing is that @hosamhamdy258 's fix works if there's a currency in the model you're including, which BaseModel has but the vanilla thing doesn't 😁
Maybe there'sa a better way to just take the currency from django settings 🤔

[benjaoming]

@errietta it seems to work if there's a DEFAULT_CURRENCY defined... but yeah, it's generally important to define currencies in these validations. I don't really mind that it breaks in cases where the currency has become implicit.

[errietta]

@benjaoming if you want to enforce that behaviour maybe it would be good to have an actual error message rather than just let the code die with a cryptic message.. just IMO

[benjaoming]

if you want to enforce that behaviour maybe it would be good to have an actual error message rather than just let the code die with a cryptic message.. just IMO

I'm just trying to release this feature, I don't use django-rest-framework. My assessment is that "yes there might be ways to make it correctly ignore a missing currency value, but this is largely not desirable, so that case can be ignored".

[phillipuniverse]

Caused a regression at #757

[phillipuniverse]

I got some additional slightly weird behavior but tough to tell if it's an actual regression or not.

I recall now that this PR is just another version of some research I did a couple of years ago on #665 (comment). I have been running an overridden version of MinMoneyValidator and MaxMoneyValidator for a couple of years now in production as a result.

The root is that there are cases where the deserialized values from DRF are sometimes a Money and sometimes a Decimal. The behavior before is that if there was a <<field_name>>_currency field passed, the deserialized value became a Money, otherwise it stayed a dictionary. The exception was then caused by some other assumptions in the library that assumed the deserialized value was a Money, hence the exception in this PR and the other one.

I had some tests fail on my end and I realize that the problem with this current fix is there is no way to differentiate between currency being explicitly passed in/set as a default on the field and currency being read from the system default.

From request JSON data that looks like this:

{'category': 'OTHER', 'unit_amount': 10, 'unit_name': 'Safety Fee', 'unit_quantity': 4}

where the Django unit_amount field looks like this:

unit_amount = MoneyField(
    max_digits=19, decimal_places=4, null=False, default_currency=constants.DEFAULT_CURRENCY
)

(by the way - this is another weird thing, this field is being automatically created by DRF but the default_currency set on the Django model field doesn't copy over when the DRF field gets created. So if it was, my custom behavior to check whether or not something was explicitly set in the payload would already be broken in 3.4)

Old behavior in 3.4:

New behavior in 3.5:

---

Anyway, at the end of the day it's probably better to be consistent and always return Money no matter what. Hopefully this helps somebody that notices the change.

[phillipuniverse]

The more I think about it the more I think what has been done here is probably the right thing to do.

For my particular case to differentiate between using the system default currency and an explicitly provided currency for the field I can use self.initial_data in my serializer. This will contain the mostly-raw request body. 👍