Improve introspection of validators #533
Conversation
ngnpope
force-pushed
the
validator-fixes
branch
from
2a884f2 to
d8632ae
Compare
Codecov Report
@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 98.66% 98.71% +0.05%
==========================================
Files 56 57 +1
Lines 6122 6292 +170
==========================================
+ Hits 6040 6211 +171
+ Misses 82 81 -1
Continue to review full report at Codecov.
|
ngnpope
force-pushed
the
validator-fixes
branch
2 times, most recently
from
b9029ec to
6d8c650
Compare
|
Hi @tfranzel. I think this one should be done now. Let me know if there is anything else that needs addressing. |
There was a problem hiding this comment.
@ngnpope thank you for this top-notch PR! outstanding work! really happy about this.
just have a few minors.
you basically refactored the last remnants of the original upstream implementation, which were a pending todo for quite some time now. that's also the reason why the method names were not really aligned with the rest of the code.
Use `FloatField` as `maximum` is only valid with `numeric` type. As we can't use `DecimalField` because its `max_digits` and `decimal_places` attributes take precedence over those of the `DecimalValidator`, we just need another closely compatible field type.
Ensure that only validation keywords and values that are suitable for the detected type are used, e.g. `maximum` isn't valid for `type` of `string`. See the following links for details: - https://datatracker.ietf.org/doc/html/draft-wright-json-schema-validation-00#section-4 - https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6 Closes tfranzel#338.
For example, if `IntegerField` has a `min_value` of `10`, we don't want a `MinValueValidator` with a value of `5` to override what has already been assigned to `minimum`.
If we are have max digits set to 4 and decimal places set to 1, we expect the maximum value to be 999.9, but we are setting it to be 1000. In this case we expect the maximum bound to be exclusive, not inclusive as is the default. Obviously, if we encounter another non-DecimalValidator that would override, we'd need to remove the exclusive flag. Note: This will need tweaking for OpenAPI 3.1 which uses a newer version of the JSON Schema specification. In that case `exclusiveMaximum` and `exclusiveMinimum` are no longer boolean values and should replace the `maximum` and `minimum` properties instead.
ngnpope
force-pushed
the
validator-fixes
branch
from
6d8c650 to
470878b
Compare
You're welcome 🙂 I've made the requested changes. |
This resolves the bug in #388 which I have also bumped into.
There weren't many tests covering validator introspection, so I've added a lot and it should be quite robust now.
I also made use of
exclusiveMaximumandexclusiveMinimumwhen usingmax_digitsto constructminimumandmaximumfor decimals as they are not going to be inclusive, i.e.max_digits = 3anddecimal_places = 2→###.##, which is1000 < x < 1000and not1000 ≤ x ≤ 1000.Other than that, introspection of validators for child fields was also fixed, some missing validators were added, missing
minPropertiesandmaxPropertiesare now handled and tests for various subclasses of validators too.