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

Enable pydantic_model_creator to use Forward references using ReverseRelation in different files

Open eyllanesc-JE opened this issue 1 year ago • 7 comments

Description

Motivation and Context

Fixed #1841

How Has This Been Tested?

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.
  • [ ] I have added the changelog accordingly.
  • [ ] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

eyllanesc-JE avatar Jan 06 '25 12:01 eyllanesc-JE

CodSpeed Performance Report

Merging #1842 will not alter performance

Comparing eyllanesc-JE:develop (06c10a3) with develop (8b5ac14)

Summary

āœ… 8 untouched benchmarks

codspeed-hq[bot] avatar Jan 06 '25 12:01 codspeed-hq[bot]

Pull Request Test Coverage Report for Build 12632106944

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 90.147%

Totals Coverage Status
Change from base Build 12617761088: 0.002%
Covered Lines: 6462
Relevant Lines: 7050

šŸ’› - Coveralls

coveralls avatar Jan 06 '25 12:01 coveralls

@eyllanesc-JE, could you please explain why setting localns won't cause the same issue as in https://github.com/tortoise/tortoise-orm/issues/1552 when globalns was set?

henadzit avatar Jan 07 '25 10:01 henadzit

@markus-96 any chance you can weight in here since you contributed to the pydantic integration recently?

henadzit avatar Jan 07 '25 10:01 henadzit

@henadzit maybe next week, too busy right now... But what I can say is that I did not encounter this bug yet, although I am working on a project right now that makes heavy use of defining models in different packages. Also, please take the comment into account I made in the corresponding issue: https://github.com/tortoise/tortoise-orm/issues/1841#issuecomment-2575217383

markus-96 avatar Jan 07 '25 13:01 markus-96

@henadzit

@eyllanesc-JE, could you please explain why setting localns won't cause the same issue as in #1552 when globalns was set?

By default if the global namespace is not set then the mro is used where the elements such as the fields indicated in the bug are defined.

See the docs:

  • ...
  • If obj is a class C, the function returns a dictionary that merges annotations from C’s base classes with those on C directly. This is done by traversing C.__mro__ and iteratively combining __annotations__ dictionaries. Annotations on classes appearing earlier in the method resolution order always take precedence over annotations on classes appearing later in the method resolution order.
  • ...

By overriding global namespace then get_type_hints does not have access to properties of the Model namespace. But only to the new models themselves.

In my solution using locals namespace I am adding elements that typing.get_type_hints does not know about, such as models in other files.

Another solution could be that pydantic_model_creator can be passed the locals and globals namespace giving the programmer more freedom to see which elements get_type_hints needs.

eyllanesc-JE avatar Jan 07 '25 22:01 eyllanesc-JE

ok, since get_annotions is not even used for relational fields (only data fields and computed fields), I think it is OK to advise people to use the following pattern for importing related models to prevent circular imports:

from typing import TYPE_CHECKING
from typing_extensions import TypeVar

from tortoise import fields
from tortoise import Model

if TYPE_CHECKING:
    from models.foo import Foo
else:
    Foo = TypeVar('Foo', bound=Model)

class Bar(Model):
    foo: fields.ForeignKeyRelation['Foo'] = fields.ForeignKeyField("models.Foo")

markus-96 avatar Jan 14 '25 11:01 markus-96