Allow for passing keyword arguments to from_orm

[dosisod]

Change Summary

You can already use Field(alias='whatever') on the model to allow for
converting from ORM models that might be named differently, but this
means that the model knows about the existence of a database (or at
least it's columns), which seems like a leaky abstraction.

With this modification though, the database will be the one to handle
the naming conflict, which keeps the model more succinct.

Related issue number

None that I can find

Checklist

• Unit tests for the changes exist
• Tests pass on CI and coverage remains at 100%
• Documentation reflects the changes where applicable
• changes/<pull request or issue id>-<github username>.md file added describing change
  (see changes/README.md for details)
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

[dosisod]

please review

[jordi-reinsma]

This feature is VERY much appreciated. If my ORM model has A, B, C..., W and X fields but I want the resulting PydanticModel to have an extra Y and Z fields, I would've needed to:

• Do some Config.getter_dict trickeries if available
• PydanticModel(A=orm.A, B=orm.B, ..., X=orm.X, Y=y, Z=z) instead of PydanticModel.from_orm(orm, Y=y, Z=z)
• root_validator trickery if Y and Z fields are based on any of the other fields

And if you need a async function result the only headacheless solution is the second one. But just this one feature and all of these are solved.

Have to point out though that the kwargs overrides the original ORM model fields, e.g.:

class UserORM:
  name: str
class User(BaseModel):
  name: str
  class Config:
    orm_mode=True
print(User.from_orm(UserORM("Daniel"), name="Cooler Daniel"))
# prints User(name="Cooler Daniel")

[samuelcolvin]

LGTM, please can you update the docs including an example.

Please update.

[dosisod]

@samuelcolvin Docs have been updated

[dosisod]

ping @samuelcolvin

[dosisod]

@samuelcolvin is there anything else needed for this PR to be merged?

[jordi-reinsma]

pinging for relevancy

[axgb]

Also pinging for relevancy

[samuelcolvin]

I'll try to merge for v1.10 soon.

[hauh]

There is an issue with this approach when an ORM object has descriptors (@property decorated methods).
When you unpack an object {**obj, **kwargs}, each attribute gets accessed, even if it is not defined in pydantic model, therefore all property methods are called, which is not always appropriate and probably can lead to slower performance.
Example:

class Model(BaseModel):

    class Config:
        orm_mode = True

    a: int


class Test:

    a = 1

    @property
    def test(self):
        print('called')


Model.from_orm(Test(), extra=42)

[samuelcolvin]

@hauh is correct, if we're to support this, we would need to modify _decompose_class to try kwargs on get before getattr.

Please update.

[samuelcolvin]

please update.

[samuelcolvin]

Looking good, just a few small things to fix. Please update.

Please remember to use the manage phrase described in the PR template to assign us once this is ready to review, otherwise it'll be missed.

[samuelcolvin]

Not sure this needs to be in the try: block?

Also this code isn't covered at all which is why tests are failing.

[dosisod]

If there are kwargs, but the key isn't contained, we have to fallback on the getattr() call. I updated it to the following:

            if self._kwargs and key in self._kwargs:
                return self._kwargs[key]
            return getattr(self._obj, key)

The try block was one way, this is another way. Is there a better way you think?

[dosisod]

please review.

I don't know why the doc building is failing now, worked last commit, and works on my local machine.

[samuelcolvin]

The docs build is failing because #4339 was merged.

It'll be failing locally and the error message explains how to fix it, so should be easy to fix.

Please update.

[Bobronium]

@dosisod, this should fix docs build:

Suggested change

	```py
	{!.tmp_examples/models_orm_mode_kwargs.py!}
	```
	_(This script is complete, it should run "as is")_
	{!.tmp_examples/models_orm_mode_kwargs.md!}

You can test this locally as well, but you will need to pull changes from remote.

[Bobronium]

@samuelcolvin, please beware that this PR introduces a breaking change

This was previously a valid usecase, but with these changes it will raise an error because second positional argument is now always passed to getter_dict:

def test_custom_getter_dict():
    class TestCls:
        x = 1
        y = 2

    def custom_getter_dict(obj):
        assert isinstance(obj, TestCls)
        return {'x': 42, 'y': 24}

    class Model(BaseModel):
        x: int
        y: int
        class Config:
            orm_mode = True
            getter_dict = custom_getter_dict
    model = Model.from_orm(TestCls())
    assert model.dict() == {'x': 42, 'y': 24}

[Bobronium]

Seems like, kwargs will be silently ignored if cls.__custom_root_type__ is true. I don't think it should be the case.

[Bobronium]

This is a breaking change and should be reflected in docs and history.

[Bobronium]

IMO, this should be out of try/except AttrubuteError block.

And perhaps we should ask for forgiveness, not permission (like you've done it in get method)?

[Bobronium]

IMO, The way of getting key out of kwargs should match one in __getitem__

[samuelcolvin]

Thanks so much @Bobronium for looking into this.

I wonder if we should delay this feature to V2?

@Bobronium @dosisod @PrettyWood @hramezani what do you think?

[Bobronium]

I wonder if we should delay this feature to V2?

It's one option.

Another is checking if getter_dict has kwargs argument when validating a config:

if "kwargs" not in inspect.signature(config.getter_dict).parameters:
    orig_getter_dict = config.getter_dict
    @functools.wraps(orig_getter_dict)
    def updated_getter_dict(obj, kwargs):
        return orig_getter_dict(obj)
    
    config.getter_dict = updated_getter_dict

Or perhaps, not changing the GetterDict in the first place and always just wrapp it to account for kwargs?
So we'll have an obj behind two proxy layers: kwargs_wrapper -> getter_dict -> obj.

All this seems not quite right though, but I don't have a perfect solution in mind.

[samuelcolvin]

The best option long term is pydantic/pydantic-core#223, I think that there's quite an elegant solution in rust and we don't have to worry about breaking changes.

My worry is that any solution for this in V1.10 is either going to be hacky or a break changing in some edge case, or both.

I'm leaning towards closing this and waiting for V2, although I feel pretty guilty for @dosisod who has worked on this a lot and responded to my feedback, they would understandably find it pretty annoying if this PR gets closed now. 🙏

[PrettyWood]

they would understandably find it pretty annoying if this PR gets closed now.

computed fields PR will probably be closed so trust me I know the feeling 😆 but this is part of the deal when working on OSS.

---

If there is a way to avoid any breaking change like @Bobronium suggested let's go for it. But if there is a risk of breaking change I would defer this PR. I would love 1.10 to be a smooth and stable release before the big bang

[dosisod]

@samuelcolvin I don't mind closing this PR, this feature seems to be introducing more friction then a feature of this size should introduce. And, if there are plans to handle this in v2, we might be better off waiting.

Thank you all for your feedback and support!

[samuelcolvin]

Thanks so much for understanding, we'll do our best to make this happen in V2.