netzkolchose / django-computedfields

Provides autogenerated autoupdated database fields for model methods.
MIT License
94 stars 14 forks source link

Dependency graph calculation fails with Foreign Keys referenced by string #136

Closed BonaFideIT0 closed 9 months ago

BonaFideIT0 commented 1 year ago

Hi there,

when using a String reference to a model from another app like this on a ComputedFieldModel:

fieldname = models.ForeignKey("appname.ModelName")

we get this error:

Traceback (most recent call last):
  File "/censored/./sites/censored/manage.py", line 21, in <module>
    main()
  File "/censored/./sites/censored/manage.py", line 17, in main
    execute_from_command_line(sys.argv)
  File "/censored/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line
    utility.execute()
  File "/censored/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 395, in execute
    django.setup()
  File "/censored/venv/lib/python3.11/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/censored/venv/lib/python3.11/site-packages/django/apps/registry.py", line 122, in populate
    app_config.ready()
  File "/censored/venv/lib/python3.11/site-packages/computedfields/apps.py", line 29, in ready
    BOOT_RESOLVER.initialize()
  File "/censored/venv/lib/python3.11/site-packages/computedfields/resolver.py", line 214, in initialize
    self.load_maps()
  File "/censored/venv/lib/python3.11/site-packages/computedfields/resolver.py", line 230, in load_maps
    self._graph = ComputedModelsGraph(self.computed_models)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/censored/venv/lib/python3.11/site-packages/computedfields/graph.py", line 458, in __init__
    self.resolved: IResolvedDeps = self.resolve_dependencies(computed_models)
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/censored/venv/lib/python3.11/site-packages/computedfields/graph.py", line 582, in resolve_dependencies
    self._right_constrain(cls, target_field)
  File "/censored/venv/lib/python3.11/site-packages/computedfields/graph.py", line 472, in _right_constrain
    f = model._meta.get_field(fieldname)
        ^^^^^^^^^^^
AttributeError: 'str' object has no attribute '_meta'

Upon further investigation we get that "model" (in the above snippet) contains the string reference to the model like this:

"appname.ModelName"

This might be happening due to usage of PEP-563 (https://peps.python.org/pep-0563/) style type hints, but I suspect (havent checked yet) this is due to missing model name resolution at this point.

After trying a few techniques for resolving the model that early before registry initialization I concede that fixing this might be beyond my current understanding of the problem.

Would you kindly look into it? It would help massively with a circular dependency problem we are having right now. ;)

Sincere thanks for your great work!

jerch commented 1 year ago

Can you give a minimal repro?

Could be, that the graph construction needs another explicit modelname to model type resolve step, but I am not sure yet, where the twist is - it seems, that the models worked on in the loop here still contains the str-notion: https://github.com/netzkolchose/django-computedfields/blob/e9985a6d0458753c0ac9aa1a90d97b8234a5e7da/computedfields/graph.py#L496

On a second glance - technically this should not have happened, as the resolver/graph always works on model types, never on the str-versions (pulled from model init signals). So yeah a repro would be good to check, where it breaks with that assumption.

Edit: I think I've found the culprit here: https://github.com/netzkolchose/django-computedfields/blob/e9985a6d0458753c0ac9aa1a90d97b8234a5e7da/computedfields/graph.py#L580

This indeed pulls the to value of the fk field without further check, which still contains the str version. Not sure yet, if thats easy fixable, the code at that stage kinda relies on proper model resolution already done by django.

jerch commented 1 year ago

Cannot repro with the minimal example (tested with python 3.10, django 4.2):

class FkIssue(ComputedFieldsModel):
    fieldname = models.ForeignKey('FkOther', on_delete=models.CASCADE)

    @computed(models.IntegerField(), depends=[('fieldname', ['aa'])])
    def pulled_aa(self):
        return self.fieldname.aa

class FkOther(models.Model):
    aa = models.IntegerField()

The models are already loaded properly and related_model returns a model type, not a str for fieldname (from debug log):

model: <class 'test_full.models.FkIssue'>
symbol: fieldname cls: <class 'test_full.models.FkOther'>

Plz provide the following info:

BonaFideIT0 commented 1 year ago

Hi Jerch,

thanks for your effort so far, I will have time to create a minimal repo the next few days and will get back to you. :)

cheers