Opened 10 months ago

Closed 9 months ago

Last modified 6 months ago

#35269 closed New feature (invalid)

GeneratedFields can't be defined on RelatedFields

Reported by: Perrine L. Owned by: nobody
Component: Database layer (models, ORM) Version: 5.0
Severity: Normal Keywords:
Cc: Perrine L. Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It is not possible to define a GeneratedField with a ForeignKey or a ManyToManyField as an output_field.
When those RelatedFields are defined as output_field, an error is raised

AttributeError: 'ForeignKey' object has no attribute 'opts' line 190, in _check_relation_model_exists or line 494, in related_query_name (cf. https://github.com/django/django/blob/3d4fe39bac082b835a2d82b717b6ae88ea70ea15/django/db/models/fields/related.py)

At least with Postgres as a backend, there doesn't seem to be any technical impossibility to make a GeneratedField on a ForeignKey or ManyToMany fields : https://www.postgresql.org/docs/current/ddl-generated-columns.html

Change History (9)

comment:1 by Mariusz Felisiak, 10 months ago

Resolution: needsinfo
Status: newclosed

Related fields were never intended to be used as output_fields. Moreover, as far as I'm aware, GeneratedFields cannot refer fields from external models, so I'm not exactly sure what kind of column you want to add. Please add a sample project that reproduces your issue, and provide a valid column DDL definition that you believe is not currently supported.

comment:2 by fero, 9 months ago

Hello, I think I have developed something that would do for this bug. With RelatedField on a GeneratedField it is possible to use all wonderful Django ORM lookup features.

https://github.com/feroda/play-django-generatedfk

Now my implementation requires a separate field (I have called NoopForeignKey https://github.com/feroda/play-django-generatedfk/blob/main/noopfk/fields.py), in addition to the GeneratedField, but I think it is not too much work to integrate the feature in a dedicated field to be used as output_field (maybe: models.GeneratedForeignKey?) and I am almost confident that it could be a benefit also for referencing F() foreign key fields that should not provide only the value of the foreign key, but also the RelatedManager too.

What do you think about it?

comment:3 by fero, 9 months ago

Resolution: needsinfo
Status: closednew

comment:4 by Sarah Boyce, 9 months ago

Resolution: invalid
Status: newclosed

Hi Perrine, this ticket should only be re-opened if you do as Mariusz requested and provide a valid column DDL definition that you believe is not currently supported.
I believe this will not be possible as the linked documentation states and Mariusz mentioned, GeneratedFields cannot reference anything other than the current row in any way and being a ForeignKey is a reference to an external model.

Thank you for the sample project and I can see what you're trying to do. As what you're suggesting would be a new feature to Django, the recommended path forward is to first propose and discuss the idea with the community and gain consensus that this would be a desirable addition to Django. To do that, please consider starting a new conversation on the Django Forum, where you'll reach a wider audience and likely get extra feedback.

I'll close this ticket, but if there is a community agreement for the feature request, you are welcome to create a ticket for the new feature and point to the forum topic. For more details, please see the documented guidelines for requesting features.

comment:5 by Natalia Bidart, 9 months ago

Type: BugNew feature

in reply to:  4 comment:6 by fero, 9 months ago

Hello Sarah,
thanks for the answer

Replying to Sarah Boyce:

Hi Perrine,

Perrine?!? Sorry...I think you are answering me, and not Perrine. I hope to be right.

this ticket should only be re-opened if you do as Mariusz requested and provide a valid column DDL definition that you believe is not currently supported.

I have opened the issue without a DDL because the matter is just a way on how Django give an accessor to ForeignKey attributes, mainly very useful lookup. And this can be done (at least in PostgreSQL) even without constraints. So no DDL really required for this feature.

I believe this will not be possible as the linked documentation states and Mariusz mentioned, GeneratedFields cannot reference anything other than the current row in any way and being a ForeignKey is a reference to an external model.

Yes, the GeneratedField can reference only info of its own row. I have had the problem when I wanted to created a generated field with a CASE WHEN and apply a Now(), aka CURRENT_TIMESTAMP as a default value. This cannot be done.

But in this case the ForeignKey is just and ID and it is computed from data of its own row. It is really the same as a BigIntegerField (or a CharField if you use id like ssn), but it gives you a quicker way to access to the related model.

So, I really appreciate your answer, but my humble opinion disagrees with the position that implementing this feature would imply that GeneratedField relies in external data.

Thank you for the sample project and I can see what you're trying to do. As what you're suggesting would be a new feature to Django, the recommended path forward is to first propose and discuss the idea with the community and gain consensus that this would be a desirable addition to Django. To do that, please consider starting a new conversation on the Django Forum, where you'll reach a wider audience and likely get extra feedback.

I'll close this ticket, but if there is a community agreement for the feature request, you are welcome to create a ticket for the new feature and point to the forum topic. For more details, please see the documented guidelines for requesting features.

I leave the ticket closed, but I have answered here to return you with a feedback, and to link the ticket in the forum where I will go.

Thank you very much!

comment:7 by Simon Charette, 9 months ago

You don't need any GeneratedField subclass to achieve what you're after, simply use ForeignObject which is the accessor base of ForeignKey.

from django.db.models.fields.related_fields import ForeignObject

class Event(models.Model):
    last_updated_by_id = models.GeneratedField(
        expression=Greatest("created_by", "confirmed_by", "canceled_by"),
        output_field=models.BigIntegerField(),
        db_index=True,
        db_persist=True,
    )
    last_updated_by = ForeignObject(
        User,
        models.SET_NULL,
        from_fields=["last_updated_by_id"],
        to_fields=["id"],
        related_name="last_updated_events_set",
        null=True,
    )

I also believe there is no reason to add this feature in core as with these two primitives it should be trivial to implement a ForeignGeneratedField(ForeignObject) that does exactly what you're after in a third-party application for the rare use case this is useful.

in reply to:  7 comment:8 by fero, 9 months ago

Replying to Simon Charette:

You don't need any GeneratedField subclass to achieve what you're after, simply use ForeignObject which is the accessor base of ForeignKey.

Thank you very much for the snippet Simon, I am going to try for sure!

I also believe there is no reason to add this feature in core as with these two primitives it should be trivial to implement a ForeignGeneratedField(ForeignObject) that does exactly what you're after in a third-party application for the rare use case this is useful.

I am not sure this is a rare case, but you could be right.
If not, it would be useful to have it imho because needs internal knowledge.

Let's try and see if I am able to build a 3rd party app. Thanks

comment:9 by Marco Glauser, 6 months ago

We ran into the same issue. We tried to switch the responsible user of a task depending on some simple business logic that could be evaluated by the GeneratedField.

The ForeignObject approach is working for us.

For anyone stumbling on this discussion later on, we observed a caching issue with ForeignObject. The cache of the field doesn't get refreshed when refresh_from_db gets called.
We had to explicitly override refresh_from_db on the Task model

    def refresh_from_db(self, using=None, fields=None):
        super(Task, self).refresh_from_db(using=using, fields=fields)
        responsible_user_field = self._meta.get_field("responsible_user")
        if responsible_user_field.is_cached(self):
           responsible_user_field.delete_cached_value(self)
Note: See TracTickets for help on using tickets.
Back to Top