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

Confusing handling of badly defined model_validator #9334

Open
1 task done
TimChild opened this issue Apr 27, 2024 · 3 comments
Open
1 task done

Confusing handling of badly defined model_validator #9334

TimChild opened this issue Apr 27, 2024 · 3 comments
Labels
feature request help wanted Pull Request welcome

Comments

@TimChild
Copy link

TimChild commented Apr 27, 2024

Initial Checks

  • I confirm that I'm using Pydantic V2

Description

If there is a bad model_validator that forgets to return self then SomeModel.model_validate(dict_obj) returns None while SomeModel(**dict_obj) continues to return the validated model.

This makes it seem like there is an issue with the .model_validate method, but obviously it's because of the bad validator...

This might have been the cause of #7055 (that's what I found when I first had the issue).

So, not quite a bug given that the code all works correctly if the user doesn't do something stupid... But, it would have been nice to be told I was stupid via a meaningful error or warning :)

Example Code

from pydantic import BaseModel, model_validator, ValidationError


class SomeModel(BaseModel):
    a: int

    @model_validator(mode="after")
    def validate_a(self):
        if self.a == 10:
            raise ValueError("Bad")
        # Should return `self` here... but "accidentally" ommitting it to show confusing behaviour that results from this.

if __name__ == "__main__":
    # Confusing behaviour is that model_validate returns None while direct instantiation does not
    gets_none = SomeModel.model_validate({"a": 5})
    assert gets_none is None, "This is somewhat surprising (although obviously the result of a bad validator)"

    gets_value = SomeModel(**{"a": 5})
    assert isinstance(gets_value, SomeModel), "This confuses the matter by still returning a value..."

    # Both correctly raise validation errors still
    try:
        SomeModel.model_validate({"a": 10})
        print("Does not get to here")
    except ValidationError as e:
        print("Correctly raises validation")

    try:
        SomeModel(**{"a": 10})
        print("Does not get to here")
    except ValidationError as e:
        print("Correctly raises validation")

Python, Pydantic & OS Version

pydantic version: 2.7.1
        pydantic-core version: 2.18.2
          pydantic-core build: profile=release pgo=true
                 install path: /home/tim/.cache/pypoetry/virtualenvs/backend-dvWTEZau-py3.12/lib/python3.12/site-packages/pydantic
               python version: 3.12.1 | packaged by Anaconda, Inc. | (main, Jan 19 2024, 15:51:05) [GCC 11.2.0]
                     platform: Linux-5.15.146.1-microsoft-standard-WSL2-x86_64-with-glibc2.35
             related packages: typing_extensions-4.11.0 pydantic-settings-2.2.1 fastapi-0.109.2
                       commit: unknown
@TimChild TimChild added bug V2 Bug related to Pydantic V2 pending Awaiting a response / confirmation labels Apr 27, 2024
@sydney-runkle
Copy link
Member

Agreed, a more helpful warning message would be helpful here. PR welcome with added support for this :).

@sydney-runkle sydney-runkle added feature request help wanted Pull Request welcome and removed bug V2 Bug related to Pydantic V2 pending Awaiting a response / confirmation labels Apr 28, 2024
@Viicos
Copy link
Contributor

Viicos commented Apr 29, 2024

I'll probably work on this!

@Viicos
Copy link
Contributor

Viicos commented Apr 30, 2024

Taking a look at it, we could error here on the pydantic-core side:

impl FunctionAfterValidator {
    fn _validate<'py, I: Input<'py> + ?Sized>(
        &self,
        call: impl FnOnce(&I, &mut ValidationState<'_, 'py>) -> ValResult<PyObject>,
        py: Python<'py>,
        input: &I,
        state: &mut ValidationState<'_, 'py>,
    ) -> ValResult<PyObject> {
        let v = call(input, state)?;
        let r = if self.info_arg {
            let info = ValidationInfo::new(py, state.extra(), &self.config, self.field_name.clone());
            self.func.call1(py, (v.to_object(py), info))
        } else {
            self.func.call1(py, (v.to_object(py),))
        };
        r.map_err(|e| convert_err(py, e, input))
        // Check if the value is `None`
    }
}

However, I'm unsure if the FunctionAfterValidator is also used in other places, e.g. with field validators where None would be a valid value returned by the validator?

As a side note, this can be prevented by using a type checker (and I'll implement a rule in flake8-pydantic soon), plus you will notice during development (as opposed to during runtime if adding an explicit check as suggested)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request help wanted Pull Request welcome
Projects
None yet
Development

No branches or pull requests

3 participants