Opened 10 years ago

Closed 9 years ago

#3688 closed (fixed)

[patch] Improve support for mutually-referential models

Reported by: Ben Slavin <benjamin.slavin@…> Owned by: Adrian Holovaty
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

ForeignKey and other related model fields allow recursive relationships; however, there can be problems when dealing with mutually referential models.

For example, consider a user class and a series of articles:

class User(models.Model):
    saved_articles = models.ManyToManyField('Article')
    # ...

class Article(models.Model):
    author = models.ForeignKey(User)
    # ...

It doesn't make much sense for Users and Articles to be stored in the same
models.py file.

By using dotted-notation (app_name.model_name), it should be
possible to reference across models.py files when recursive import
statements would otherwise cause problems.

my_project/accounts/models.py

class User(models.Model):
    saved_articles = models.ManyToManyField('articles.Article')
    # ...

my_project/articles/models.py

class Article(models.Model):
    author = models.ForeignKey('accounts.User')
    # ...

Attached patch adds this functionality and provides documentation.

Attachments (2)

recursive-related-fields-4692.diff (6.3 KB) - added by Ben Slavin <benjamin.slavin@…> 10 years ago.
Patch described above.
recursive-related-fields-4692v2.diff (6.5 KB) - added by Ben Slavin <benjamin.slavin@…> 10 years ago.
Modification based on Ramiro's suggestion. Also fixed a slight typo in the docs (use of : instead of ::)

Download all attachments as: .zip

Change History (12)

Changed 10 years ago by Ben Slavin <benjamin.slavin@…>

Patch described above.

comment:1 Changed 10 years ago by Ben Slavin <benjamin.slavin@…>

Summary: Improve support for mutually-referential models[patch] Improve support for mutually-referential models

Modification of title to indicate patch is included.

comment:2 Changed 10 years ago by Ramiro Morales <rm0 _at_ gmx.net>

Ben,

I'm not a Django guru so I can't comment on the code changes your patch implements. But I 'd like to point out that IMHO the changes to model-api.txt eliminate the paragraphs and examples that explicitely explain ManyToManyField and OneToOneField relationship field can be on 'self' but the new Recursive relationships section it adds doesn't make it clear with the same emphasis (it just talks about the ForeignKey case).

So, from the point of view an user implementing such a common use case (that obviously doesn't involves relationships to external models at all) the documentation is slightly less helpful than before.

Just my 2 cents.

Changed 10 years ago by Ben Slavin <benjamin.slavin@…>

Modification based on Ramiro's suggestion. Also fixed a slight typo in the docs (use of : instead of ::)

comment:3 Changed 10 years ago by Ben Slavin <benjamin.slavin@…>

Ramiro,

You're right about removing some of the clarity. I've uploaded a new version of the patch to address your concern... what do you think?

It's tough to articulate complex relationships clearly... to terse and it's confusing, too verbose and it's intimidating. If you have any suggestions to make it more understandable, please let me know.

In terms of OneToOneField, I don't think that self-referential OneToOneFields actually work. Because of the OneToOneField internals, they must share the same ID (last I checked)... so it's literally a reference to 'self' and therefore somewhat silly. I may be mistaken here, though.

  • Ben

comment:4 Changed 10 years ago by Russell Keith-Magee

Resolution: wontfix
Status: newclosed

The current behaviour is by design. Text-style references are permitted within a single application, but not across applications. If you have two models in different apps that are self referential, then you don't really have two apps - you have one app (or, two apps that are so closely related that they probably should be one app).


comment:5 Changed 10 years ago by joaoma@…

What if I have an amazingly large number of models, and I want to split the models across different files, and there are mutually-referential models in different files? This is a problem I've run into once, and I had to put all the models back in one (giant) models.py file.

comment:6 Changed 10 years ago by Ben Slavin <benjamin.slavin@…>

joaoma:

You might find this helpful: CookBookSplitModelsToFiles

Then, using the already-existing 'modelname' reference style (string) will work since they're still in the same application.

comment:7 in reply to:  4 Changed 10 years ago by Osso

Resolution: wontfix
Status: closedreopened

Replying to russellm:

The current behaviour is by design. Text-style references are permitted within a single application, but not across applications. If you have two models in different apps that are self referential, then you don't really have two apps - you have one app (or, two apps that are so closely related that they probably should be one app).


This means you have to put the articles and the users in the same app which doesn't make sense.

comment:8 Changed 10 years ago by Ben Slavin <benjamin.slavin@…>

Resolution: wontfix
Status: reopenedclosed

Replying to Osso:

This means you have to put the articles and the users in the same app which doesn't make sense.

I assure you that Russell understood the implications you describe when he marked the ticket as wontfix. This is a design decision on the dev team's part... I don't completely agree with the choice, but I respect it.

The actual number of users impacted is very small (this doesn't tend to pop-up with the built-in user model). If you need the functionality this ticket describes, just apply the attached patch... it works. :-)

If you can come up with an argument that goes beyond what's already been discussed, please bring it up... but don't reopen the ticket without adding anything new to the discussion.

comment:9 Changed 9 years ago by Ben Slavin

Resolution: wontfix
Status: closedreopened

(Reopening to change resolution. Sorry if there's a way to do it in one step -- I don't see it.)

comment:10 Changed 9 years ago by Ben Slavin

Resolution: fixed
Status: reopenedclosed

Fixed in [7158].

Note: See TracTickets for help on using tickets.
Back to Top