Code

Opened 7 years ago

Closed 6 years ago

#3688 closed (fixed)

[patch] Improve support for mutually-referential models

Reported by: Ben Slavin <benjamin.slavin@…> Owned by: adrian
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@…> 7 years ago.
Patch described above.
recursive-related-fields-4692v2.diff (6.5 KB) - added by Ben Slavin <benjamin.slavin@…> 7 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 7 years ago by Ben Slavin <benjamin.slavin@…>

Patch described above.

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Improve support for mutually-referential models to [patch] Improve support for mutually-referential models

Modification of title to indicate patch is included.

comment:2 Changed 7 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 7 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 7 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 follow-up: Changed 7 years ago by russellm

  • Resolution set to wontfix
  • Status changed from new to closed

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 7 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 7 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 7 years ago by Osso

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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 7 years ago by Ben Slavin <benjamin.slavin@…>

  • Resolution set to wontfix
  • Status changed from reopened to closed

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 6 years ago by __hawkeye__

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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

comment:10 Changed 6 years ago by __hawkeye__

  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed in [7158].

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.