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

Disable validation of annotations #1747

Closed

Conversation

henadzit
Copy link
Contributor

@henadzit henadzit commented Oct 23, 2024

Description

  • This should fix Problem with Sum with F and Field validator MinValue #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

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:

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

@henadzit
Copy link
Contributor Author

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

coveralls commented Oct 23, 2024

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 Coverage Status
Change from base Build 11435130298: -0.04%
Covered Lines: 5990
Relevant Lines: 6613

💛 - Coveralls

@henadzit henadzit force-pushed the fix/validation-of-annotations branch from 542670f to c0c16b0 Compare October 24, 2024 14:20
@abondar
Copy link
Member

abondar commented Oct 26, 2024

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

@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!

#1750

@henadzit henadzit closed this Oct 27, 2024
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.

Problem with Sum with F and Field validator MinValue
3 participants