Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30894 closed Cleanup/optimization (wontfix)

Reverse OneToOneField relation: `prefetch_related` uses `related_name` while `select_related` uses `related_query_name`

Reported by: Adam Sołtysik Owned by: nobody
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The documentation (https://docs.djangoproject.com/en/2.2/ref/models/querysets/#select-related) states:

You can also refer to the reverse direction of a OneToOneField in the list of fields passed to select_related — that is, you can traverse a OneToOneField back to the object on which the field is defined. Instead of specifying the field name, use the related_name for the field on the related object.

But in fact related_query_name is used by select_related and related_name is used by prefetch_related .

When we have the following models:

class A(Model):
    pass
class B(Model):
    a = OneToOneField('A', related_name='b_set', related_query_name='b')

then a.select_related('b') works and a.prefetch_related('b') does not:

AttributeError: Cannot find 'b' on A object, 'b' is an invalid parameter to prefetch_related()

or if class A happens to have a property named b:

ValueError: 'b' does not resolve to an item that supports prefetching - this is an invalid parameter to prefetch_related().

And vice versa, a.prefetch_related('b_set') works and a.select_related('b_set') does not:

django.core.exceptions.FieldError: Invalid field name(s) given in select_related: 'b_set'. Choices are: b

The documentation implies that both methods should use related_name, but I think it would be more intuitive if both used related_query_name, given that we can write lookups like in filter queries etc., and such a change would probably be less backward-incompatible.

Change History (8)

comment:1 by Carlton Gibson, 5 years ago

Hi Adam.

It looks to me as if this case is already covered by the test suite (and works)...

We have a LinkedList model:

class LinkedList(models.Model):
    name = models.CharField(max_length=50)
    previous_item = models.OneToOneField(
        'self', models.CASCADE,
        related_name='next_item',
        blank=True, null=True,
    )

This is using the related_name 'next_item'.

Then in the test case, we use that in `select_related`:

    def test_self_relation(self):
        item1 = LinkedList.objects.create(name='item1')
        LinkedList.objects.create(name='item2', previous_item=item1)
        with self.assertNumQueries(1):
            item1_db = LinkedList.objects.select_related('next_item').get(name='item1')
            self.assertEqual(item1_db.next_item.name, 'item2')

comment:2 by Carlton Gibson, 5 years ago

Component: Database layer (models, ORM)Documentation
Resolution: wontfix
Status: newclosed
Type: BugCleanup/optimization

OK, yes. Your models work the same. It's the presence of the related_query_name which causes the failure. Remove it and it works, because (as per the docs) related_query_name defaults to related_name if that's set.

At best this is a documentation cleanup/optimization but setting related_query_name explicitly is a much more specialised use case than setting related_name. I think the related_query_name docs are clear enough that setting it overrides the name of the reverse filter, and that a clarification in the select_related() docs would only muddy the water for the majority of users. (One should under the consequences before setting related_query_name.)

Version 1, edited 5 years ago by Carlton Gibson (previous) (next) (diff)

comment:3 by Adam Sołtysik, 5 years ago

I think the related_query_name docs are clear enough that setting it overrides the name of the reverse filter, and that a clarification in the select_related() docs would only muddy the waters for the majority of users.

OK, but why doesn't it override the name of the reverse filter in the case of prefetch_related() then? My report is about the inconsistency between select_related() and prefetch_related(). I want to use different related_name and related_query_name in my models and I would expect that related_query_name can be used in both select_related() and prefetch_related().

comment:4 by Carlton Gibson, 5 years ago

Well, they're not really the same thing... prefetch_related() is intended for Many relations, not one-to-one, and it first of all needs to pick the related object descriptor off of the model in order to build the right queryset, for the follow-up query. Thus it needs an actual attribute on the model.

comment:5 by Adam Sołtysik, 5 years ago

This only explains why it was easier to implement that way, but doesn't prove it shouldn't work the other way. I started using prefetch_related() instead of select_related() when I found out that it is much faster with LIMITed queries (at least on PostgreSQL). The fact that I had to change relation names was a bit confusing. But if you don't think this should be changed, then so be it.

comment:6 by Simon Charette, 5 years ago

I think this should be addressed, it doesn't make much sense that related_query_name is used everywhere but in prefetch_related. It was probably simply overlooked in prefetch_related given it works in select_related.

It should not be too hard to deprecate given the rare usages of related_query_name by accepting both related_query_name and related_name during the deprecation period and warning when related_name != related_query_name and related_name is used.

comment:7 by Carlton Gibson, 5 years ago

Component: DocumentationDatabase layer (models, ORM)

Hey Simon. I'm happy if you want to reopen and Accept, but I'm not entirely sure it's the best change...

related_query_name serves to make filtering a little prettier where the related_name doesn't read right, i.e. you might want to write
...filter(tag__name='...') rather that ...filter(tag_set__name='...') (or tags being the example from the docs). That reads better because you're saying where the tag name is ..., i.e. checking the property on the individual, rather than the set.

But with prefetch_related(), you want to prefetch the set, so you want prefetch_related('tag_set') (or tags) — you want the related_name.

(So if the issue is inconsistency, maybe it's select_related that has it backwards — via get_related_name(), but we can't change that now I'd guess...)

Q: how does get_prefetcher() look if we're not retrieving the actual descriptor off of the model class?

As I say, I leave to you. Thanks.

(For my future ref c.f. #18668, #15962, ...)

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:8 by Adam Sołtysik, 5 years ago

I think I should add a bit more about my exact use case. I have OneToOneField with _something as related_name and something as related_query_name, and I also have a property something that returns _something if it exists, and a default object if it doesn't. This way everywhere in my code I use only something without underscore -- except for prefetch_related(), and it's even worse with chained relations, as I end up with triple underscores, like prefetch_related(_something1___something2).

However, I've just a found another solution for the problem of missing OneToOneFields: https://stackoverflow.com/a/32106815, which doesn't make it necessary to use different related_name and related_query_name. I'm going to test it, and if it works, then the issue will be lower priority for me. Although I still think the two methods should work consistently.

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