fix int_validator not catching overflows#3112
Conversation
|
please review |
|
I reckon it would be better to catch the |
Sorry, first time contributing to pydantic. Does that mean just changing the |
| except (TypeError, ValueError, OverflowError): | ||
| raise errors.IntegerError() |
There was a problem hiding this comment.
| except (TypeError, ValueError, OverflowError): | |
| raise errors.IntegerError() | |
| except (TypeError, ValueError): | |
| raise errors.IntegerError() | |
| except OverflowError: | |
| raise errors.IntegerOverflowError() |
I think this is meant.
There was a problem hiding this comment.
Then, like 144
- return float(v)
+ rv = float(v)
+ if rv in (float("inf"), float("-inf")):
+ raise errors.FloatOverflowError()
+ if rv != rv:
+ raise errors.FloatNanError()
+ return rvP.S. the weird line of code from https://stackoverflow.com/a/44154660/705086
There was a problem hiding this comment.
floats are a separate case. I would say that float('inf') is a valid float, arguably so is float('nan').
samuelcolvin
left a comment
There was a problem hiding this comment.
This is looking good, just a few things to fix and more test cases to add.
| except (TypeError, ValueError, OverflowError): | ||
| raise errors.IntegerError() |
There was a problem hiding this comment.
floats are a separate case. I would say that float('inf') is a valid float, arguably so is float('nan').
| Model(a=2.2250738585072011e308) | ||
| assert exc_info.value.errors() == [ | ||
| {'loc': ('a',), 'msg': 'value is not a valid integer', 'type': 'type_error.integer'} | ||
| ] |
There was a problem hiding this comment.
can you add the case of passing float('inf') etc.
|
please update. |
@samuelcolvin i've added more test cases, do you think the error code/msg should be handled different? If so, how are new messages/codes decided? |
|
At this stage, best if you can roughly follow the code from pydantic-core to minimize the incompatibility with v2. |
|
Since the structure of the error dict is different between v1 and v2 anyway, does it make sense trying to make it v2-like? I feel making it fit in with the rest of v1 makes more sense (eg in v1, error codes seem to always be prefixed with |
|
Yes, that's what I meant by roughly follow, e.g. use |
When I change it to that, interestingly enough |
samuelcolvin
left a comment
There was a problem hiding this comment.
Okay, let's just stick with this then.
|
Good catch, thanks so much for the PR. |
Change Summary
int(value)can raise anOverflowError, for example when called with very large floats.Related issue number
#2345
Checklist
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)