Handle DRF MoneyField validation in to_internal_value

[flaeppe]

Fixes #601
Fixes #637

Move any form of Money validation to MoneyField.to_internal_value. A quick peek at serializers' validation implementation shows that errors bubbling up from .to_internal_value will be handled.

Although, since the DRF MoneyField is a composite field per default, I'm slightly uncertain of how to react when finding an invalid currency value. At the moment, this will mark up an invalid_currency error for the declared MoneyField, which might feel a little off, especially when working with explicitly declared amount and currency fields (e.g. money_currency declared as a ChoiceField), since that'll then always result in 2 field errors.

Since MoneyField.get_value is declared the way it is, even if there's no %_currency-field or default currency declared for a serializer, any data passed matching that key will be silently/implicitly captured.. Consider the following test case that currently fails:

    def test_does_not_capture_unknown_field_data(self):
        class Serializer(serializers.Serializer):
            """
            No default_currency or explicit currency field declared for 'money'
            """
            money = MoneyField(max_digits=9, decimal_places=2)

        # 'money_currency' have no declared place on Serializer.
        serializer = Serializer(data={"money": "0.01", "money_currency": "USD"})
        assert serializer.is_valid()
        assert serializer.validated_data["money"] == Decimal("0.01")  # Money("0.01", "USD") != Decimal("0.01")

[codecov-commenter]

Codecov Report

Merging #650 (ded2183) into main (13e7a63) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
+ Coverage   97.95%   97.96%   +0.01%     
==========================================
  Files          29       29              
  Lines         976      985       +9     
  Branches      192      193       +1     
==========================================
+ Hits          956      965       +9     
  Misses         13       13              
  Partials        7        7              

Impacted Files	Coverage Δ
djmoney/contrib/django_rest_framework/fields.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13e7a63...ded2183. Read the comment docs.

[Stranger6667]

What is the primary purpose of this class and why Money won't work instead?

[Stranger6667]

Oh, I think that it is here to avoid validation inside Money, right? so it is deferred to to_internal_value

[flaeppe]

Exactly, creation of a Money instance and validation of its data will now only happen in to_internal_value. PrimitiveMoney will only "transport" the data to to_internal_value without doing any validation

[Stranger6667]

Cool! I'd suggest replacing namedtuple with a handwritten class with __slots__ (as it is a bit faster and potentially harder to misuse) and renaming it to _PrimitiveMoney to denote, its private use. What do you think?

[Stranger6667]

Also, a short comment describing the purpose of this class would be helpful

[flaeppe]

Cool! I'd suggest replacing namedtuple with a handwritten class with __slots__ (as it is a bit faster and potentially harder to misuse)

The only (small) drawback I see with only __slots__ is immutability, which I kind of like in cases like this. Though I won't say I have a very strong opinion about it here. It seems like it's possible to combine namedtuple and __slots__ to get an immutable object. What do you think, should we do that?

renaming it to _PrimitiveMoney to denote, its private use

Sounds good!

Also, a short comment describing the purpose of this class would be helpful

Definitely, I'll add in a comment

[flaeppe]

@Stranger6667 I bumped to use the namedtuple + __slots__ combination just to get a feel of what that would look like. Let me know if you want me to change it.

[Stranger6667]

Maybe something like this to avoid using namedtuple at all?

class _PrimitiveMoney:
    __slots__ = ("amount", "currency")

    def __init__(self, amount, currency):
        self.amount = amount
        self.currency = currency

[flaeppe]

Alright, I bumped to your suggestion.