netzkolchose / django-computedfields

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

Computed ForeignKey fields are broken #155

Open a-p-f opened 1 day ago

a-p-f commented 1 day ago

Hi there. I have a project that's been using django-computedfields for a while, but I've run into an error when upgrading Django and django-computedfields. It appears you can no longer have a computed ForeignKey field (which my project has been happily doing with django-computedfields==0.1.5).


models.py (Bar has a computed ForeignKey to Foo):

from computedfields.models import ComputedFieldsModel, computed
from django.db import models

class Foo(models.Model):
    pass

class Bar(ComputedFieldsModel):
    foo1 = models.ForeignKey(Foo, models.CASCADE, related_name="+")

    @computed(
        models.ForeignKey(Foo, models.CASCADE, related_name="+"),
        depends=[("self", ["foo1"])],
    )
    def foo2(self):
        return self.foo1

test code (just trying to save a Bar instance):

from computedfields_bug.models import Bar, Foo

f = Foo()
f.save()

b = Bar(foo1=f)
b.save()

The test code runs fine with: Django==3.2.25 django-computedfields==0.1.5

But with: Django==5.1.3 django-computedfields==0.2.6

the b.save() call generates:

Traceback (most recent call last):
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/fields/__init__.py", line 2123, in get_prep_value
    return int(value)
           ^^^^^^^^^^
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'Foo'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/alex/temp/computedfields_test/test.py", line 7, in <module>
    b.save()
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/computedfields/models.py", line 54, in save
    return super(ComputedFieldsModel, self).save(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/base.py", line 891, in save
    self.save_base(
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/base.py", line 997, in save_base
    updated = self._save_table(
              ^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/base.py", line 1160, in _save_table
    results = self._do_insert(
              ^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/base.py", line 1201, in _do_insert
    return manager._insert(
           ^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/query.py", line 1847, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/sql/compiler.py", line 1835, in execute_sql
    for sql, params in self.as_sql():
                       ^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/sql/compiler.py", line 1760, in as_sql
    self.prepare_value(field, self.pre_save_val(field, obj))
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/sql/compiler.py", line 1699, in prepare_value
    return field.get_db_prep_save(value, connection=self.connection)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/fields/related.py", line 1142, in get_db_prep_save
    return self.target_field.get_db_prep_save(value, connection=connection)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/fields/__init__.py", line 1008, in get_db_prep_save
    return self.get_db_prep_value(value, connection=connection, prepared=False)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/fields/__init__.py", line 2815, in get_db_prep_value
    value = self.get_prep_value(value)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/temp/computedfields_test/venv/lib/python3.12/site-packages/django/db/models/fields/__init__.py", line 2125, in get_prep_value
    raise e.__class__(
TypeError: Field 'id' expected a number but got <Foo: Foo object (5)>.

PS: I had a quick look at examples.test_full.models, and it looks like you don't have any tests with a computed ForeignKey.

a-p-f commented 1 day ago

Update:

My code seems to work as expected if I update the field-computing-method to return the primary key of the related object, instead of directly returning the related object. Ie:


    @computed(
        models.ForeignKey(Foo, models.CASCADE, related_name="+"),
        depends=[("self", ["foo1"])],
    )
    def foo2(self):
        return self.foo1.pk

This approach will work for me in current project, but I think my initial approach is the more natural/expected way. Thanks,

Alex

jerch commented 18 hours ago

@a-p-f Thx for reporting. To be honest, ForeignKey fields as CFs were never officially supported (and thus not tested), as they would screw up the dependency tree and make the resolvers life much harder. Yes they are reported to work for the first CF field, guess thats what you are doing. Furthermore fk fields are treated differently at different stages by the ORM (at some point as DB-native value, at others as model instance), seems you just run into that difference here. There was a similar report in the past, imho the change happened around django ~4.x.

I agree, that returning the model type is more straight forward to comprehend, will see if I can find a workaround for that.