Opened 7 years ago

Closed 5 months ago

#10532 closed Bug (fixed)

An overridden get method on a custom manager passed to get_object_or_404 will not be called.

Reported by: anonymous Owned by: Claude Paroz <claude@…>
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: django@…, anubhav9042@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


If a manager is passed in as the first argument to get_object_or_404, _get_queryset will convert it into a queryset via manager.all().
This means that an overridden get method on the manager passed to get_object_or_404 will not be called. This is rather unintuitive.

# In django.shortcuts
def _get_queryset(klass):
    Returns a QuerySet from a Model, Manager, or QuerySet. Created to make
    get_object_or_404 and get_list_or_404 more DRY.
    if isinstance(klass, QuerySet):
        return klass
    elif isinstance(klass, Manager):
        manager = klass
        manager = klass._default_manager
    return manager.all()

def get_object_or_404(klass, *args, **kwargs):
queryset = _get_queryset(klass)
        return queryset.get(*args, **kwargs)

Perhaps it should just be return manager instead of manager.all().

Change History (13)

comment:1 Changed 7 years ago by thatch

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 5 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:3 Changed 5 years ago by ccrisan

Any updates on this bug? So far the only resolution I've found is to use my own get_object_or_404() function.

comment:4 Changed 5 years ago by SmileyChris

If there were any updates on the bug, you'd see them here.

Since it sounds like you're already working with code to work around this issue, perhaps you'd care to produce a patch and test case? That's the kind of thing that gets things moving.

At the very least, start a discussion on the django-dev mailing list (which is a good idea anyway, since this is sitting in the design decision stage).

Last edited 5 years ago by SmileyChris (previous) (diff)

comment:5 Changed 5 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 5 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:7 Changed 3 years ago by akaariai

  • Triage Stage changed from Design decision needed to Accepted

Sounds like a good change to me. I'd go a bit further: if klass has .get() method then use that. So:

    # If it is a model class, or anything else with ._default_manager
    if hasattr(klass, '_default_manager'):
        return klass._default_manager.all()
    return klass # If it is a manager or queryset, then this is OK. Even if it isn't then lets hope it has .get().

This would allow for true duct-typing, anything with .get() is OK, but Model subclasses which have .get() defined for other purposes will still continue to work.

The only question here is if this leads to confusing errors - having a method named .get() is somewhat likely. But I think the risk is worth it.

comment:8 Changed 3 years ago by streeter

  • Cc django@… added

comment:9 Changed 3 years ago by anubhav9042

  • Cc anubhav9042@… added
  • Owner changed from nobody to anubhav9042
  • Status changed from new to assigned

comment:10 Changed 2 years ago by anubhav9042

  • Owner anubhav9042 deleted
  • Status changed from assigned to new

comment:11 Changed 5 months ago by claudep

  • Has patch set

Anssi's suggestion implemented in that PR.

comment:12 Changed 5 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 5 months ago by Claude Paroz <claude@…>

  • Owner set to Claude Paroz <claude@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 12ba20d8:

Fixed #10532 -- Relaxed hard-type checking in get_object/list_or_404 shortcuts

Thanks Anssi Kääriäinen for the patch suggestion, and Tim Graham for the review.

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