IntField: add MinMaxValidator #1868
Conversation
CodSpeed Performance ReportMerging #1868 will not alter performanceComparing Summary
|
Pull Request Test Coverage Report for Build 13369845719Details
💛 - Coveralls |
tortoise/fields/data.py
Outdated
| def __init__( | ||
| self, | ||
| primary_key: Optional[bool] = None, | ||
| ge: Optional[int] = None, |
There was a problem hiding this comment.
Extending the field configuration with ge and le introduces the second way of setting up validation. I think we should have a single way of doing it to keep things simple - through validators. If some one needs specific validation, they can add a validator. So I suggest to remove these options from the constructor.
There was a problem hiding this comment.
ge and le are currently baked in in the constraints property. Instead of that I wanted them to be customizable, a bit like with max_char in CharField where this is reflected during validation and also by the constraints property.
There was a problem hiding this comment.
ge and le are currently baked in in the constraints property. Instead of that I wanted them to be customizable, a bit like with max_char
The int field constraints are related to the nature of the data type. I think it is different from business logic validation and we already have a way of doing it. We shouldn't have two ways of doing the same thing.
|
@markus-96 I push another solution: #1872 |
Sorry if I ask, but where is this different from mine? I put much effort into optimizing my solution, could you tell what is wrong about that? I think we all have an interest in tortoise-orm being the fastest orm for python, so something that crucial like an int field has to be as fast as possible as it can be, so I think hard-coding default validators is a completly valid solution. |
tortoise/fields/data.py
Outdated
| def to_db_value(self, value: Any, instance: "Union[Type[Model], Model]") -> Any: | ||
| if value is not None: | ||
| if not isinstance(value, self.field_type): | ||
| value = self.field_type(value) # pylint: disable=E1102 |
There was a problem hiding this comment.
In which cases is it required?
|
I think we should be able to merge it if we remove the ability to customize constraints. |
@markus-96 Your issue #1853 request a feature to check constraints for int fields before saving to db. There are several ways to do that. We can compare the two PRs and choose one or merge both. Code speed is important, and compatibility also counts. I use Django for a long time before migrating to tortoise-orm, and do not want it to be too much different from Django. |
CHANGELOG.rst
Outdated
| @@ -17,6 +17,8 @@ Fixed | |||
| Changed | |||
| ^^^^^^^ | |||
| - add benchmarks for `get_for_dialect` (#1862) | |||
| - optimize `get_for_dialect` (#1863) | |||
| - add validator to `IntField` (#1866) | |||
There was a problem hiding this comment.
Can you please extend the note so it is easier to understand for people reading the changelog? I think something like "Added default range validation to IntField, BigIntField, SmallIntField" would be better.
Could you please rebase on develop? I cleaned up the changelog to remove internal notes from the changelog.
There was a problem hiding this comment.
i am so bad at rebasing, i create a new pr...
markus-96
force-pushed
the
intfield-validator
branch
from
8ebc81f to
960b1c1
Compare
Description
Before sending anything to the DB, IntField values are checked beforehand now. As far as I could test, the added overhead is not much, lets see what Codspeed says.
Will add documentation later.
Motivation and Context
Using sqlite, you could send values greater than defined in
IntField.constraints.get('le')to the DB and nothing was raised. If you for example want to migrate to another DBMS, you have a problem.Also, have a look at the Issue I raised: #1853
How Has This Been Tested?
Existent tests and new tests
Checklist: