Code

Opened 7 years ago

Closed 4 years ago

Last modified 3 years ago

#5537 closed (fixed)

Remove Reverse Lookups on ForeignKeys/ManyToMany

Reported by: dcramer Owned by: nobody
Component: Documentation Version: master
Severity: Keywords:
Cc: jpwatts Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

There should be way to remove the addition of a property on models for doing reverse lookups.

Say I have a minor table, that holds statistical information, and it has rows per user. I will never use myuser.statistical_table.all(), so it makes sense to have a way to tell it to not append that object to the instances.

Attachments (1)

relatedname (1).diff (559 bytes) - added by subsume 4 years ago.
Docfix per russel

Download all attachments as: .zip

Change History (17)

comment:1 Changed 7 years ago by ubernostrum

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

Personally I'd be -1 on this because it would clutter the ORM syntax while only providing noticeable utility in rather extreme use cases.

comment:2 Changed 7 years ago by dcramer

This could possibly be a large performance gain as well. Maybe it should even be pushed the related_name="" be required to add it to the reverse instances. If you're selecting 100 rows (an normal query), that has 50 accessors on it (I'm sure people attach to "User" just as much as us), it must have some impact on performance to attach 500 instances of a manager to those classes.

comment:3 Changed 7 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

This would in no way be a performance boost. The accessors for reverse lookups are descriptors; they aren't evaluated until they are used.

However, I'm promoting this to accepted. We don't need to add a keyword to the ORM syntax - we can exploit related_name. Here's the use case, writ large:

class Note(Model)
   text = CharField()

class Author(Model):
   name = CharField()
   name_notes = ManyToManyField(Note, related_name='author_names')
   age = IntegerField()
   age_notes = ManyToManyField(Note, related_name='author_ages')
   address = CharField()
   address_notes = ManyToManyField(Note, related_name='author_addresses')

In this example, related_name is a required field (to avoid namespace collisions), and the names must be unique. This means you have to invent unique (and meaningful) names, even if the related object is of no use.

However, if you allowed related_name=None to mean 'dont install the related object', you could avoid the namespace collision, avoid the need to invent unique names, and clean up the model definition.

comment:4 Changed 7 years ago by ubernostrum

  • Triage Stage changed from Accepted to Design decision needed

Russ, are you sure the change is worth the cost? As you've described it, all it does is slightly simplify the case of multiple relations between the same set of models, with no performance gain, and it does so at the cost of a backwards-incompatible change which breaks virtually any model that's ever used a relation and which makes the common case more complex.

comment:5 Changed 7 years ago by russellm

Ah... yeah... that... oops. Missed the great big honking backwards incompatibility. Scratch that idea :-)

One alternative would be to define a NO_REVERSE string that can act as a marker for those fields you don't want to have reverse relations. related_name=NO_REVERSE isn't quite as clean as related_name=None, but it would avoid the backwards incompatibility problem.

However, I'm happy to go with consensus/BDFL judgement on this. I'm still in favour of the general feature, but obviously not at the expense of a clean interface. I also wouldn't lose any sleep if it wasn't implemented, because there _is_ a workaround.

comment:6 Changed 7 years ago by Noam Raphael <spam.noam@…>

I just wanted to say that I would like this feature too - I don't like to invent unused names.

By the way, perhaps related_name=None can be made backwards-compatible. I think that when people write models that depend on the automatic related name, they don't write related_name=None - they just don't write anything. So you can make a constant related_name=AUTOMATIC, which will be the default, and related_name=None will make no related field.

Noam

comment:7 Changed 7 years ago by mtredinnick

I'm -1 on this. There's no demonstrated gain, either performance wise or from a technical perspective. Not worth it.

comment:8 Changed 7 years ago by dcramer

I just want this so it doesn't clutter up all of our exception messages when we're debugging :)

Whether its a performance gain or not (I'm guessing it is, but miniscule), that's another story.

comment:9 Changed 5 years ago by jgeewax

Not sure if this helps, but I had a similar problem and used the following subclass so that I didn't need to define related names for the reverse look ups that didn't matter:

from django.db import models

class NonReversibleManyToManyField(models.ManyToManyField):

_relation_counter = 0


@classmethod
def generate_related_name(cls):

cls._relation_counter += 1
return "anonymous_relation_%s" % cls._relation_counter


def contribute_to_related_class(self, cls, related):

self.rel.related_name = NonReversibleManyToManyField.generate_related_name()
return super(NonReversibleManyToManyField, self).contribute_to_related_class(cls, related)

This seems to pass validation, and the direct look up seems to work fine, it just creates anonymous reverse lookups "anonymous_relation_N" for all the others.

Not sure if this is perfect but it seemed to solve the problem for me.

comment:10 Changed 5 years ago by jpwatts

  • Cc jpwatts added

comment:11 Changed 4 years ago by russellm

  • Component changed from Database layer (models, ORM) to Documentation
  • Triage Stage changed from Design decision needed to Accepted

Accepted as a documentation fix; this is actually possible right now as long as you start your related_name with a "+".

Changed 4 years ago by subsume

Docfix per russel

comment:12 Changed 4 years ago by russellm

  • milestone set to 1.3
  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 4 years ago by DrMeers

I think you mean end related_name with a '+'

comment:14 Changed 4 years ago by DrMeers

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

(In [14049]) Fixed #5537 -- document trailing '+' on related_name for supressing backward relation.

Thanks to dcramer for the report, and Russ for pointing out the workaround.

comment:15 Changed 4 years ago by DrMeers

(In [14050]) [1.2.X] Fixed #5537 -- document trailing '+' on related_name for supressing backward relation.

Thanks to dcramer for the report, and Russ for pointing out the workaround.

Backport of r14049 from trunk.

comment:16 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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.