Repository: pydantic/pydantic
Pull request: Do not override `model_post_init` in subclass with private attrs
<!-- Thank you for your contribution! -->
<!-- Unless your change is trivial, please create an issue to discuss the change before creating a PR -->

## Change Summary

<!-- Please give a short summary of the changes. -->

## Related issue number

Fixes https://github.com/pydantic/pydantic/issues/7293

<!-- WARNING: please use "fix #123" style references so the issue is closed when this PR is merged. -->

## Checklist

* [x] The pull request title is a good summary of the changes - it will be used in the changelog
* [x] Unit tests for the changes exist
* [x] Tests pass on CI
* [ ] Documentation reflects the changes where applicable
* [x] My PR is ready to review, **please add a comment including the phrase "please review" to assign reviewers**


Selected Reviewer: @hramezani

Problem statement:
model_post_init not called on subclasses with a private attribute
### Initial Checks

- [X] I confirm that I'm using Pydantic V2

### Description

In the example below, I would expect the `Model1.model_post_init` to be called when instantiating `Model2` but it is not.

On the other hand, `Model1.model_post_init` is called:
- when instantiating `Model1`
- when instantiating `Model1` even if I add a private attribute
- when instantiating `Model2` if I change `_private` into `private`
- when instantiating `Model2` if I add a `model_post_init` in `Model2` which calls `Model1.model_post_init`

Is this an expected behavior?
Thanks for your help!

Note: `Model2.__pydantic_post_init__` remains set to `'model_post_init'`

### Example Code

```Python
import pydantic

class Model1(pydantic.BaseModel):
    a: int = 12

    def model_post_init(self, __context) -> None:
        print("Post init", self.__class__)

class Model2(Model1):
    _private: bool = True

Model2() # does not trigger inherited post init
```


### Python, Pydantic & OS Version

```Text
pydantic version: 2.3.0
        pydantic-core version: 2.6.3
          pydantic-core build: profile=release pgo=false
                 install path: /Users/jrapin/Applications/miniconda3/envs/test_env/lib/python3.10/site-packages/pydantic
               python version: 3.10.12 (main, Jul  5 2023, 15:02:25) [Clang 14.0.6 ]
                     platform: macOS-13.5.1-arm64-arm-64bit
     optional deps. installed: ['typing-extensions']
```


Selected Assignee: @Kludex

Hints from issue discussion:
@Viicos I think you are on the right track. Would you like to open a PR for this? If not, we can get it across the line.
> @Viicos I think you are on the right track. Would you like to open a PR for this? If not, we can get it across the line.

I'll try to submit a PR

Commit message: Use a default return value for `getattr`

Patch to review:
diff --git a/pydantic/_internal/_model_construction.py b/pydantic/_internal/_model_construction.py
index 5b22a67e4a3..cca9d449369 100644
--- a/pydantic/_internal/_model_construction.py
+++ b/pydantic/_internal/_model_construction.py
@@ -104,9 +104,9 @@ def __new__(
                 namespace, config_wrapper.ignored_types, class_vars, base_field_names
             )
             if private_attributes:
-                if 'model_post_init' in namespace:
+                original_model_post_init = get_model_post_init(namespace, bases)
+                if original_model_post_init is not None:
                     # if there are private_attributes and a model_post_init function, we handle both
-                    original_model_post_init = namespace['model_post_init']
 
                     def wrapped_model_post_init(self: BaseModel, __context: Any) -> None:
                         """We need to both initialize private attributes and call the user-defined model_post_init
@@ -266,6 +266,17 @@ def init_private_attributes(self: BaseModel, __context: Any) -> None:
     object_setattr(self, '__pydantic_private__', pydantic_private)
 
 
+def get_model_post_init(namespace: dict[str, Any], bases: tuple[type[Any], ...]) -> Callable[..., Any] | None:
+    """Get the `model_post_init` method from the namespace or the class bases, or `None` if not defined."""
+    if 'model_post_init' in namespace:
+        return namespace['model_post_init']
+
+    for base in bases:
+        model_post_init = getattr(base, 'model_post_init', None)
+        if model_post_init is not None:
+            return model_post_init
+
+
 def inspect_namespace(  # noqa C901
     namespace: dict[str, Any],
     ignored_types: tuple[type[Any], ...],
diff --git a/tests/test_main.py b/tests/test_main.py
index e6eb3e0e0c8..5fba4af8cd4 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -1979,6 +1979,27 @@ def model_post_init(self, __context: Any) -> None:
         BaseModel.model_post_init = original_base_model_post_init
 
 
+def test_model_post_init_subclass_private_attrs():
+    """https://github.com/pydantic/pydantic/issues/7293"""
+    calls = []
+
+    class A(BaseModel):
+        a: int = 1
+
+        def model_post_init(self, __context: Any) -> None:
+            calls.append(f'{self.__class__.__name__}.model_post_init')
+
+    class B(A):
+        pass
+
+    class C(B):
+        _private: bool = True
+
+    C()
+
+    assert calls == ['C.model_post_init']
+
+
 def test_deeper_recursive_model():
     class A(BaseModel):
         b: 'B'


Code review comments:
diff_hunk: @@ -266,6 +266,17 @@ def init_private_attributes(self: BaseModel, __context: Any) -> None:
     object_setattr(self, '__pydantic_private__', pydantic_private)
 
 
+def get_model_post_init(namespace: dict[str, Any], bases: tuple[type[Any], ...]) -> Callable[..., Any] | None:
+    """Get the `model_post_init` method from the namespace or the class bases, or `None` if not defined."""
+    if 'model_post_init' in namespace:
+        return namespace['model_post_init']
+
+    for base in bases:
+        model_post_init = getattr(base, 'model_post_init', None)
path: pydantic/_internal/_model_construction.py
text: @author:
This will actually always be defined for any base, as any subclass of `BaseModel` will inherit from the original empty method:

https://github.com/pydantic/pydantic/blob/c987bc6398b898d5c9e49606e3acb17b732359e8/pydantic/main.py#L417-L421

So if we have the following:

```python
from pydantic import BaseModel

class A(BaseModel):
    a: int = 1

class B(BaseModel):
    b: int = 1

    def model_post_init(self, __context):
        """Do custom things"""

class C(A, B):
    _private: bool = True
```

`get_model_post_init` will use the empty `model_post_init` available on `A`, and skip the custom one from `B`. Any ideas on how to prevent this? Maybe a custom flag could help, similar to:

https://github.com/pydantic/pydantic/blob/c987bc6398b898d5c9e49606e3acb17b732359e8/pydantic/main.py#L167-L168

?

@user1:
If your concern is to make sure attribute lookup only considers things defined on the exact class, the way we achieve that elsewhere is to look in `base.__dict__`. So, if you do `if 'model_post_init' in base.__dict__`, that tells you if it was defined specifically in `base` vs. in one of its subclasses. The best way to do it might be to loop over the `__mro__` for the class looking for items in the classes' `__dict__`s. But it's not clear to me if that is relevant here.

Thinking this through, I think if we want to auto-generate a method but not have that auto-generated version factor into inheritance, using a flag like you've described is probably the best way. _But_, since in this case the method _is_ defined on BaseModel, I think the correct behavior would actually involve calling the BaseModel version in your code above, since that is consistent with standard python behavior for method resolution:

```python
class A:
    def my_method(self):
        print('A')

class B(A):
    pass

class C:
    def my_method(self):
        print('C')

class D(B, C):
    pass

D().my_method()
#> A
```

Not sure if I'm overlooking something important here; in general I'm open to other points of view but I think it will be important that whatever we do doesn't introduce breaking changes here. More generally, I won't be surprised if the best solution ends up being for users to just be more proactive about overriding `model_post_init` in subclasses when inheriting from multiple base models and not being able to put the one with the desired `model_post_init` first in the inheritance order.

@user1:
Also, I think if all you are wanting to do is specifically ignore `BaseModel.model_post_init`, it might be best to add something like:
```python
if model_post_init is BaseModel.model_post_init:
    continue
```
It's not clear to me if that's how it should work, or if any such generated method should be ignored (this might be the case if you are adding more private attributes for example, but never defined the `model_post_init` explicitly).

Either way, if we change this, I think we should probably add a section to the docs about how this is expected to work because it's starting to get away from typical python behavior. (But I think is likely still enough of an improvement in behavior to be worth making the change.)

diff_hunk: @@ -266,6 +266,17 @@ def init_private_attributes(self: BaseModel, __context: Any) -> None:
     object_setattr(self, '__pydantic_private__', pydantic_private)
 
 
+def get_model_post_init(namespace: dict[str, Any], bases: tuple[type[Any], ...]) -> Callable[..., Any] | None:
+    """Get the `model_post_init` method from the namespace or the class bases, or `None` if not defined."""
+    if 'model_post_init' in namespace:
+        return namespace['model_post_init']
+
+    for base in bases:
+        model_post_init = getattr(base, 'model_post_init', None)
+        if model_post_init is not None:
+            return model_post_init
path: pydantic/_internal/_model_construction.py
text: @user1:
```suggestion
    from ..main import BaseModel

    model_post_init = get_attribute_from_bases(bases, 'model_post_init')
    if model_post_init is not BaseModel.model_post_init:
        return model_post_init
```
I think this is probably the "most correct" version of this, since we are already using the `get_attribute_from_bases` function which handles mro the same as python, etc.

@user1:
The above is also closely related to what we do when setting `__pydantic_post_init__` (i.e., ignoring `BaseModel.model_post_init` and follows the same logic as we do when setting `__hash__` (i.e., using the mro).

@author:
Unfortunately even using `get_attribute_from_bases`, the test is still failing (in fact `get_attribute_from_bases` is doing something quite similar, i.e. using `hasattr` and `getattr`, so we face the same issue). Maybe doing something like the following:

```python
for base in mro_for_bases(bases):
    model_post_init = base.__dict__.get('model_post_init')
    if model_post_init is not None and model_post_init is not BaseModel.model_post_init:
        return model_post_init
```

Edit: that did the trick (see `test_model_post_init_correct_mro`)

@user1:
@author this is a good catch — but actually I think this is a bug in the `get_attribute_from_bases` function — it shouldn't be using `hasattr` and `getattr` for exactly this reason. I think looking in the class `__dict__` should fix this and allow us to use that function, if it does, I would propose we make that change. Any objections? If not, I can push a commit and merge this.

@author:
Sure, do you want to do it in a separate PR? Might be easier if it breaks some tests. Then I'll rebase on `main` once merged

@user1:
Doesn't break any tests — I think my preference would be to just put it in this PR since the test you've added is actually a good test of that change.

To reduce the back-and-forth I'll just push the commit in hopes you are okay with it; I can revert that if you prefer though, and move it elsewhere.

@author:
Totally ok with it, thanks for the last push!