tortoise-orm icon indicating copy to clipboard operation
tortoise-orm copied to clipboard

`pydantic_model_creator` returns the wrong model on multiple uses

Open marcoaaguiar opened this issue 4 years ago • 4 comments

Description

pydantic_model_creator fails to respect some of its arguments (eg: exclude=["id"]), this occurs in two scenarios:

  • When the underlying model is initiated and it has no backward or forward reference to any other model;
  • The model is not initiated.

The issue seems to be in the logic: https://github.com/tortoise/tortoise-orm/blob/9c5be496cc34e5684ff8af24f3f77f2c6a951068/tortoise/contrib/pydantic/creator.py#L389-L393 Which does not consider if the model was built with the default argument or not.

Motivation and Context

Creating multiple Pydantic models for the same Tortoise model (that has no reference to other models) always returns the same PydanticModel, regardless of its different arguments in each invocation. Fixes: #647, #594

How Has This Been Tested?

Included test using pytest for

  • [x] models with references to other models (is working now)
  • [x] models with backward references removed through PydanticMeta
  • [x] models with no reference to other models
  • [x] invoking twice pydantic_model_creator with the same arguments return the same model
  • [x] invoking the pydantic_model_creator with different arguments return different models

Checklist:

  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

marcoaaguiar avatar Apr 24 '21 15:04 marcoaaguiar

I believe that these changes are ready to be merged.

6 of the added tests fail with the current develop. With this merge, all tests are passing.

On a side note, I believe that pydantic_model_creator deserves a refactor. pydantic_model_creator should be broken down into a few functions so it is easier to test and maintain. With the added tests, this task would be easier!

marcoaaguiar avatar Apr 24 '21 21:04 marcoaaguiar

Would be great to merge this. Failing due to docstring linting:

image

Simple fix by adding spaces around the docstring content in both cases: """Model with no relation""" --> """ Model with no relation """

spwoodcock avatar May 14 '22 10:05 spwoodcock

Thanks Marco!

My bad. This seems to be a Codacy issue. D203 (space before class docstring) has been phased out and should be ignored by default If we add the required space it would trigger D211 (no space before class docstring).

The only two workarounds I can see:

  1. Comment out or delete the two multiline docstrings.
  2. Add a .codacy.yaml to the repo root with config to ignore D203 (likely not possible).

spwoodcock avatar May 17 '22 10:05 spwoodcock

Thanks! Please resolve conflicts first.

long2ice avatar May 17 '22 12:05 long2ice