tortoise / tortoise-orm

Familiar asyncio ORM for python, built with relations in mind
https://tortoise.github.io
Apache License 2.0
4.39k stars 359 forks source link

Fix pydantic v2 pydantic_model_creator nullable field not optional #1465

Closed waketzheng closed 8 months ago

waketzheng commented 10 months ago

Fix pydantic v2 pydantic_model_creator nullable field not optional(#1454)

How Has This Been Tested?

  1. Tests for examples/fastapi passed
  2. Tests for examples/blacksheep not work, because it does not support pydantic V2

Checklist:

coveralls commented 10 months ago

Pull Request Test Coverage Report for Build 6497193654


Changes Missing Coverage Covered Lines Changed/Added Lines %
tortoise/contrib/pydantic/creator.py 4 5 80.0%
<!-- Total: 4 5 80.0% -->
Totals Coverage Status
Change from base Build 6479987928: 88.7%
Covered Lines: 5756
Relevant Lines: 6408

💛 - Coveralls
plusiv commented 10 months ago

This PR solve a lot of things... I think it should be taken as hotfix @long2ice .

long2ice commented 10 months ago

Thanks! Since 0.20.0 was released, so changelog should be in new section 0.20.1

plusiv commented 10 months ago

I was reviewing @waketzheng 's code. And, I humbly submit that in order to set as nullable ForeignKeyFieldInstance and OneToOneFieldInstance, the following portion of the code:

https://github.com/tortoise/tortoise-orm/blob/ba593c3888741122b832e19a45d4504a951fa7ae/tortoise/contrib/pydantic/creator.py#L352-L365

Should be set to:

        if (
            field_type is relational.ForeignKeyFieldInstance
            or field_type is relational.OneToOneFieldInstance
            or field_type is relational.BackwardOneToOneRelation
        ):
            is_to_one_relation = True
            model = get_submodel(fdesc["python_type"])
            if model:
                if fdesc.get("nullable"):
                    json_schema_extra["nullable"] = True
                    # set default_factory to None in the field config
                    fconfig["default_factory"] = lambda: None
                if fdesc.get("nullable") or field_default is not None:
                    model = Optional[model]  # type: ignore

                # create property with that config
                properties[fname] = (model, Field(**fconfig))
waketzheng commented 10 months ago

@plusiv After set default_factory to None in the field config, unittest failed at tests/contrib/test_pydantic.py

plusiv commented 10 months ago
properties[fname] = (model, Field(**fconfig))

Hi again @waketzheng !

I've cloned your changes and tested with the changes that I suggested before, and figure out that the reason is because the hardcoded schema used in the test assertion has so many errors since the relational fields are not set in the proper way to make nullable relationships fields/objects in the schema.

To explain my point, let's take a look to the models Event and Reporter

https://github.com/tortoise/tortoise-orm/blob/c030c9cebd7aba2697832d9c4769428805238f73/tests/testmodels.py#L74-L113

Since the field alias of the model Event is set to be null, the schema looks like: https://github.com/tortoise/tortoise-orm/blob/c030c9cebd7aba2697832d9c4769428805238f73/tests/contrib/test_pydantic.py#L191-L198

But, with the nullable reporter field of the same Event model doesn't happen the same in its schema:

https://github.com/tortoise/tortoise-orm/blob/c030c9cebd7aba2697832d9c4769428805238f73/tests/contrib/test_pydantic.py#L84-L99

Notice that the element {"type": "null"}, in the anyOf property list and the property "nullable": True, are missing in the schema definition.

I think that re-generating the hardcoded schema with the changes that I suggested before and replacing the old ones with the new ones, should make the test run without problems. I could help with this if you like.

plusiv commented 10 months ago

Hello again @waketzheng !

I was reviewing the function pydantic_model_creator, and I'm wondering why did you just added IntField and TextField in:

https://github.com/waketzheng/tortoise-orm/blob/ba593c3888741122b832e19a45d4504a951fa7ae/tortoise/contrib/pydantic/creator.py#L415-L424

This is causing setting as required IntFields and TextFields, even if marked as nullable.

Note: I'm just trying to understand the project logic in order to collaborate in a future.

waketzheng commented 10 months ago
properties[fname] = (model, Field(**fconfig))

Hi again @waketzheng !

I've cloned your changes and tested with the changes that I suggested before, and figure out that the reason is because the hardcoded schema used in the test assertion has so many errors since the relational fields are not set in the proper way to make nullable relationships fields/objects in the schema.

To explain my point, let's take a look to the models Event and Reporter

https://github.com/tortoise/tortoise-orm/blob/c030c9cebd7aba2697832d9c4769428805238f73/tests/testmodels.py#L74-L113

Since the field alias of the model Event is set to be null, the schema looks like:

https://github.com/tortoise/tortoise-orm/blob/c030c9cebd7aba2697832d9c4769428805238f73/tests/contrib/test_pydantic.py#L191-L198

But, with the nullable reporter field of the same Event model doesn't happen the same in its schema:

https://github.com/tortoise/tortoise-orm/blob/c030c9cebd7aba2697832d9c4769428805238f73/tests/contrib/test_pydantic.py#L84-L99

Notice that the element {"type": "null"}, in the anyOf property list and the property "nullable": True, are missing in the schema definition.

I think that re-generating the hardcoded schema with the changes that I suggested before and replacing the old ones with the new ones, should make the test run without problems. I could help with this if you like.

@plusiv You are right! Unittest should be updated.

waketzheng commented 10 months ago

Hello again @waketzheng !

I was reviewing the function pydantic_model_creator, and I'm wondering why did you just added IntField and TextField in:

https://github.com/waketzheng/tortoise-orm/blob/ba593c3888741122b832e19a45d4504a951fa7ae/tortoise/contrib/pydantic/creator.py#L415-L424

This is causing setting as required IntFields and TextFields, even if marked as nullable.

Note: I'm just trying to understand the project logic in order to collaborate in a future.

This line is to respect to unittest and compare with old version logic, after tests code updated, if can be removed.

plusiv commented 10 months ago

This line is to respect to unittest and compare with old version logic, after tests code updated, if can be removed.

Good! Seems razonable now!

@plusiv You are right! Unittest should be updated.

Got it, I think it can be updated on this PR, or you think a new one is necessary @long2ice ??

I am willing to assist if you agree.

plusiv commented 10 months ago

Hey everyone, Did this PR got stuck? I think those are very importan changes.

long2ice commented 8 months ago

Thanks! Just need resolve conflicts.

waketzheng commented 8 months ago

Thanks! Just need resolve conflicts.

conflicts fixed, but need you to rerun ci action manually.

long2ice commented 8 months ago

Thanks!