-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Default values not validated #9389
Comments
Sharing one more reference from the doc: https://docs.pydantic.dev/2.7/concepts/fields/#validate-default-values |
Thanks guys, but this ought to work? from pydantic import BaseModel, ConfigDict, with_config
from pydantic.version import version_info
@with_config(ConfigDict(validate_default=True))
class Command(BaseModel):
chan: int = 0,
val: int = 0
validated = Command(val = 3)
print(validated.chan, type(validated.chan), validated.val, type(validated.val))
print(version_info()) No change from before, no warning that I've done anything wrong. from pydantic import BaseModel, ConfigDict
from pydantic.version import version_info
class Command(BaseModel):
model_config = ConfigDict(validate_default=True)
chan: int = 0,
val: int = 0
validated = Command(val = 3)
print(validated.chan, type(validated.chan), validated.val, type(validated.val))
print(version_info()) This works. from pydantic import BaseModel, ConfigDict
from pydantic.version import version_info
class MyBaseModel(BaseModel):
model_config = ConfigDict(validate_default=True)
class Command(MyBaseModel):
chan: int = 0,
val: int = 0
validated = Command(val = 3)
print(validated.chan, type(validated.chan), validated.val, type(validated.val))
print(version_info()) This also works and seems cleaner, particularly if there is more than one schema to modify. But I have to say I'm not impressed. This behavior ought to be the default, with the ability to switch it off, and the documentation should be a lot clearer. Also the validation error message is not very clear. It does not mention that it is validation of a default that has failed, or the name of the specific defaulting field. |
I'll open an issue to work on this.
I don't know if there was a specific reason to have them not validated by default, but this can't be changed now for backwards compatibility reasons.
What part of the documentation do you find unclear? It does state the current behavior.
Seems like a reasonable FR. |
Look at "A Simple Example" in the Github readme: this has defaults in the decorator and the 'name' field is actually defaulted in the validation. There is no indication that this is not validated, or how to fix it. This problem percolates right through the ecosystem in many starter tutorials, YouTube videos and documentation for other libraries and frameworks which use Pydantic. I came to it through FastAPI and only was aware because I made a typo in my code. Otherwise I had no reason to 'go down this rabbit hole'. I suggest the following: Ship and promote a NewBaseModel which takes a 'safety first' approach to all validation, setting the defaults on this basis regardless of compatibility, but continue to ship the current BaseModel so existing code is not broken. This could be done using the inheritance method I've used above, but this is not ideal since assigning model_config could unexpectedly disable validation again. Ideally this should be implemented 'under the hood' so the defaults with no model_config are changed.
I'll raise it! (Improving the validation error message) Thanks for your answers on the other aspects of this matter which seems like a good way forward. |
It honestly takes 5 minutes of reading in the Concepts docs page (which people usually read first when discovering Pydantic) to come across the section about opt-in validation of default values. Maybe a quick note can be added here, but I don't see what more can be done. I'll also note that a type checker would have immediately caught the typo: This may be why
You can open a new issue for this to catch attention from maintainers, but there's a high chance it will be rejected, as it is overly specific |
Yea, typical coder Catch-22 response! Initial response: "Can't fix that, it would break existing code". I suggest a way of doing it that does not break existing code. "Won't do that it is overly specific." 'Overly specific' = 'not invented here'! |
Well yeah introducing a Looking at the previous duplicate issues, it seems to be disabled by default because of performance reasons. I'm wondering if with the v2 performance improvements this would still be an issue. While I agree the default behavior of not validating default values is debatable, I don't think introducing a Maybe this could be considered as a breaking change in V3 and the default value should simply change to
I unfortunately don't think this will be accepted by maintainers Footnotes
|
OK, I'd not realized this would result in both configuration dictionaries being applied (presumably in base-first order), nor that any old dict would do, it did not actually have to be a ConfigDict! Seems like a very obscurantist way of doing this though, I'd have had a config method which took VarArgs, then it would be clear the settings were 'applied' not just stored for direct use as implied by making them a property. BUT this completely undermines any objection to Pydantic taking this approach! If it is only two lines for me, it is only two lines for them too! What I was trying to achieve is to make the simple beginners example work out of the box as a new user would reasonably expect without making it any more complex or breaking existing code. I am a big believer in the approach of minimizing documentation by designing more intuitive software that behaves the way you expect out of the box. If I have to read all the documentation for all the dependencies of every library I use it completely undermines the case for using libraries at all, it would be quicker just to write the bits I need myself! |
What is the API you have in mind? I'm not sure I understand a config method which took VarArgs, but I'm interested to know :)
Pydantic is doing a lot internally to handle config, and it does not only rely on a simple dict (it uses a class,
class Base(BaseModel):
model_config = {"nested_config": {"attr1": 1}}
class Sub(Base):
model_config = {"nested_config": {"attr2": 2}}
# How is it merged? Is Sub.model_config ==
# - {"nested_config": {"attr1": 1}}
# - {"nested_config": {"attr2": 2}}
# - {"nested_config": {"attr1": 1, "attr2: 2}}?
class Base(BaseModel):
model_config = {"validate_default": True}
class Sub(Base):
model_config = {"extra": "forbid"}
Sub.model_config["validate_default"]
#> True? False? KeyError?
It's not really about how much work it requires to implement it in Pydantic, my main concern is the third paragraph from my previous comment @sydney-runkle, what is your opinion on this? How much of a breaking change would it be validate defaults by default? (assuming the performance issues are now resolved). |
Yea, I didn't really think that through! If it was a method, where'd it get called? I guess maybe this is the best way, though as you say, it does need to be better documented, and a diagnostic function to interrogate the effective setting would also be useful.
I see the danger of proliferation, but I was thinking about it as a one-off policy change: It would be made clear that the new base might implement additional checks in future so you'd better not try to exploit any loopholes. But if you need stability or maximum performance continue to use BaseModel and we promise that anything new will be off by default. |
Yeah, I think this would be a significant breaking change, definitely something we couldn't consider until at least V3. We thought through this feature pretty carefully for V2, see https://docs.pydantic.dev/latest/migration/#changes-to-validators. Thanks @Viicos for that new PR for the |
Initial Checks
Description
I am not sure if this is a bug, a feature request or even a security vulnerability. I made a tiny syntax mistake in specifying a default value in a schema and Pydantic silently placed a tuple in a field designated in the schema as int! This seems to be an architectural issue as it indicates that default values generally are not validated. It seems to me that they should be.
Example Code
Python, Pydantic & OS Version
The text was updated successfully, but these errors were encountered: