Computed ForeignKey fields are broken

[a-p-f]

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]

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]

@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.

[darkpixel]

Hmm...that must be what I'm running into too.

I have a field called 'reported_user', and two computed fields 'user1' and 'user2'. user1 depends on self.reported_user user2 depends on self.reported_user and user1

All kinds of crashiness.

To work around it, I'm just going to handle my logic by overriding the 'save' method.

[nnseva]

Oops, I met exactly the same problem. I was happy with v.1.x + early Django versions, and trying to create a compatible code which should work everywhere.

[jerch]

@nnseva Sure, feel free to look into that issue 👍

[nnseva]

@jerch I've prepared a PR actually changing only one line of the runtime code. I'm not sure about all use cases to be tested, but this change makes my own applications work as it was available with v.1.x

[jerch]

@nnseva Oh wow - the change from field.attname to field.name already fixed things here? I cannot really remember, why I went with attname here in the first place, but originally there was a reason. Since the tests still run unaffected by that change I am pretty sure it will not create a bigger disaster for standard usages.

Yeah the testbase with fk-fields as CFs is lousy to non-existant, so I am not sure, if it will handle all instance vs instance_id issues yet, esp. arounding cascade deleting and stuff.

Prolly will have to do a few more tests with fk CFs before I gonna merge it.

[nnseva]

@jerch feel free to add test cases for this PR. I will try to check and fix if something goes wrong, I'm very interested in this fix.

[jerch]

@nnseva I've added your PR, thx alot, it solves a long standing issue with computed fk fields.

I also was able to find the reason for the attname usage from a FIXME in an older work branch - initially I planned to fully reject fk fields as computed fields to keep things comprehensible and as a safety measure, as the implication on the resolver logic was not quite clear to me. Well, the latter problem still exists and comes from the fact, that the resolver spans its update logic across relations (thus taking fk fields into account). But with computed fk fields these relations can morph from the resolver state itself, which twists the update logic, if another CF depends on that field.

I am pretty sure, that things will work correctly with computed fk fields, as long as they are used as finals, meaning no further CF updates depend on them.

So to anyone - plz feel free to look into that, if you have such a convoluted use case.

[JestemStefan]

FYI This change is now causing issue in my setup after upgrade from 0.2.8 to 0.2.9

We are getting an error that

/usr/local/lib/python3.12/site-packages/django/db/models/query.py:658: in create
    obj.save(force_insert=True, using=self.db)
../../common/model_mixins.py:14: in save
    super().save(force_insert=force_insert, update_fields=update_fields, **kwargs)
../../transactions/models.py:1671: in save
    super().save(
/usr/local/lib/python3.12/site-packages/computedfields/models.py:54: in save
    return super(ComputedFieldsModel, self).save(
/usr/local/lib/python3.12/site-packages/django/db/models/base.py:814: in save
    self.save_base(
/usr/local/lib/python3.12/site-packages/django/db/models/base.py:892: in save_base
    post_save.send(
/usr/local/lib/python3.12/site-packages/django/dispatch/dispatcher.py:177: in send
    (receiver, receiver(signal=self, sender=sender, **named))
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/usr/local/lib/python3.12/site-packages/computedfields/handlers.py:69: in postsave_handler
    active_resolver.update_dependent(
/usr/local/lib/python3.12/site-packages/computedfields/resolver.py:482: in update_dependent
    _pks = self.bulk_updater(queryset, fields, return_pks=True, querysize=querysize)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/usr/local/lib/python3.12/site-packages/computedfields/resolver.py:562: in bulk_updater
    setattr(elem, comp_field, new_value)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <django.db.models.fields.related_descriptors.ForwardManyToOneDescriptor object at 0xffff8a394830>
instance = <[REDACTED] object (3)>, value = 3
    def __set__(self, instance, value):
        """
        Set the related instance through the forward relation.
    
        With the example above, when setting ``child.parent = parent``:
    
        - ``self`` is the descriptor managing the ``parent`` attribute
        - ``instance`` is the ``child`` instance
        - ``value`` is the ``parent`` instance on the right of the equal sign
        """
        # An object must be an instance of the related class.
        if value is not None and not isinstance(
            value, self.field.remote_field.model._meta.concrete_model
        ):
>           raise ValueError(
                'Cannot assign "%r": "%s.%s" must be a "%s" instance.'
                % (
                    value,
                    instance._meta.object_name,
                    self.field.name,
                    self.field.remote_field.model._meta.object_name,
                )

ValueError: Cannot assign "3": "[REDACTED]" must be a "[REDACTED]" instance.

We are passing related instance id similar to code below.

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

Weird that breaking change was introduced in minor release.

[jerch]

@JestemStefan

You are right - in retrospection this change would be better on the 0.3.0 release. By the time the fix was applied, it looked more like a bug getting finally fixed. I totally overlooked the fact, that it basically means to return model instances instead of a pk in the custom compute functions, sorry for that.

So the remaining question is - does it work for you with 0.2.10+ if you change the return value to the model instance?

[JestemStefan]

It works if I change the setup to the one that @a-p-f was using.

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

but I need to change it which is issue itself. We are using computedfield in many placed in our project and our versioning was set to upgrade to versions "0.2.*" as they should not introduce any breaking changes...

Currently I'm locking our version to 0.2.9 and we will make changes later to handle version 0.2.9+ later.

I'm just annoyed that breaking change was introduced as patch

Edit: Version 0.2.9 is working for me. 0.2.10 does not