Disable validation of annotations

henadzit commented 1 month ago

Description

This should fix #1502.
Extends Field.to_python_value with the validate argument with the default value True which regulates whether validation should be run.
Disables validation for annotations.
⚠️ This is a not backwards compatible change for those who extended Field and overrode to_python_value because its signature changed

Motivation and Context

When an arithmetic expressions on a model field is used in annotations, the field's to_python_value is used to process the database value. to_python_value is running validations by default and if the result of aggregation function doesn't pass field's validation, an exception happens.

class ValidatorModel(Model):
  max_value = fields.IntField(null=True, validators=[MaxValueValidator(20.0)])

await ValidatorModel.create(max_value=4)
await ValidatorModel.create(max_value=4)
await ValidatorModel.annotate(sum=Sum(F("max_value") * F("max_value"))).values("sum")

will cause tortoise.exceptions.ValidationError: max_value: Value should be less or equal to 20.0

Other ideas

At the moment the validation is being run when the object is being loaded from the database, for instance, here and here. Does it really need to happen? The introduction of the validate argument can be used to disable validation in these cases.

How Has This Been Tested

Added tests exposing the issue
Tested the code from #1502:

from tortoise import Tortoise, fields, run_async
from tortoise.functions import Sum
from tortoise.models import Model
from tortoise.expressions import F, Q
from tortoise.validators import MinValueValidator

class Deal(Model):
    test = fields.IntField()
    broker_payed_money = fields.FloatField(default=0, validators=[MinValueValidator(0)])

    broker_commission = fields.FloatField(null=False)

    developer_payed_money = fields.FloatField(
        null=False, default=0, validators=[MinValueValidator(0)]
    )

async def main():
    # Initializing Tortoise and creating the schema

    await Tortoise.init(  # type: ignore
        db_url="sqlite://:memory:",
        modules={"models": ["__main__"]},
    )
    await Tortoise.generate_schemas()

    await Deal.create(
        test=1, broker_payed_money=10, broker_commission=0.1, developer_payed_money=20
    )

    result = await Deal.annotate(
        balance=Sum(F("developer_payed_money") * F("broker_commission") - F("broker_payed_money")),
    ).values_list("balance")
    print("res", result)

run_async(main())

Checklist:

[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[x] I have updated the documentation accordingly.
[ ] I have added the changelog accordingly.
[x] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

henadzit commented 1 month ago

Hi @abondar! This PR contains a non backwards compatible change. How should I treat the changelog? Should I introduce 0.22.1 there with the note of this change as part of this PR? Thanks!

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 11501275212

Details

30 of 30 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.04%) to 88.978%

Totals
Change from base Build 11435130298:	-0.04%
Covered Lines:	5990
Relevant Lines:	6613

💛 - Coveralls

abondar commented 1 month ago

Hi!

Why you have decided to go with approach of turning off validation, rather than just making sure that annotated field instance doesn't have validators?

Seems like if we change this line From

        if field in self.annotations:
            annotation = self.annotations[field]
            field_object = getattr(annotation, "field_object", None)
            if field_object:
                return field_object.to_python_value
            return lambda x: x

To

        if field in self.annotations:
            annotation = self.annotations[field]
            field_object = getattr(annotation, "field_object", None)
            if field_object:
                new_field_class = deepcopy(field_object)
                new_field_class.validators = []
                return new_field_class.to_python_value
            return lambda x: x

We can exterminate root problem, of validators leaking to dynamically generated fields. And that way we won't have to introduce breaking changes

henadzit commented 1 month ago

@abondar thank you for reviewing and challenging my approach!

I gave it a bit more thought and realized that validation on loading records from the database is not common. I prepared another pull request, please have a look!

https://github.com/tortoise/tortoise-orm/pull/1750

tortoise / tortoise-orm