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

Same model returned by pydantic_model_creator calls with different arguments #1741

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

henadzit
Copy link

@henadzit henadzit commented Oct 16, 2024

Description

This PR fixes the issue where calling pydantic_model_creator multiple times with different arguments without specifying a name might return the same model due to caching (_MODEL_INDEX in creator.py). The example below demonstrates the issue.

from tortoise import fields
from tortoise.contrib.pydantic import pydantic_model_creator
from tortoise.models import Model


class People(Model):
    id = fields.IntField(pk=True)
    created_at = fields.DatetimeField(auto_now_add=True)
    modified_at = fields.DatetimeField(auto_now=True)

    first_name = fields.CharField(max_length=50)
    last_name = fields.CharField(max_length=50)


Person = pydantic_model_creator(People)
PersonIn = pydantic_model_creator(People, exclude_readonly=True)

print(list(Person.model_fields))  
# [id, created_at, modified_at, first_name_last_name]
print(list(PersonIn.model_fields))  
# [id, created_at, modified_at, first_name_last_name] 
# However, it should be ['first_name', 'last_name'] because of exclude_readonly=True

print(Person is PersonIn)  
# True (!)

This issues affects the models that have no backward or forward reference to any other model.

This PR:

  • Makes pydantic_model_creator to create verbatim names for "root" models even if they do not have relations
  • Adds the exclude_readonly to the hash used for naming unnamed models so pydantic_model_creator(People) and pydantic_model_creator(People, exclude_readonly=True)

Motivation and Context

The issues that will be fixed by this PR:

There was an attempt to solve this issue previously but it was never merged. The approach used does not seem to work anymore.

How Has This Been Tested?

  • Tested the snippets from the issues above
  • Added unit tests

Checklist:

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

@henadzit
Copy link
Author

Hey @waketzheng @waketzheng, any chance you can review this PR? Please let me know if more information or more tests are required! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant