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

Computed ForeignKey fields are broken #155

Open
a-p-f opened this issue Nov 20, 2024 · 3 comments
Open

Computed ForeignKey fields are broken #155

a-p-f opened this issue Nov 20, 2024 · 3 comments

Comments

@a-p-f
Copy link

a-p-f commented Nov 20, 2024

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
Copy link
Author

a-p-f commented Nov 20, 2024

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
Copy link
Collaborator

jerch commented Nov 21, 2024

@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
Copy link

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.

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

No branches or pull requests

3 participants