Opened 2 years ago

Closed 23 months ago

Last modified 4 months ago

#20278 closed Bug (fixed)

Passing self to object query may cause infinite regression

Reported by: anonymous Owned by: nobody
Component: Database layer (models, ORM) Version: 1.5
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

This commit introduced passing the kwargs to the DoesNotExist Query Exception:

https://github.com/django/django/commit/d5b93d3281fe93cbef5de84a52

This involves a call of the unicode() method on the given query objects and may lead to an infinity regression, if the unicode method depends on the same query.

The error occured in my custom translation system:

class Model:

    (...)

    def defaultTranslation(self):
        """
        :return: ModelTranslation
        """
        try:
            defaultTranslation = ModelTranslation.objects.get(base=self, language__code=settings.FALLBACK_LANGUAGE)
        except ObjectDoesNotExist:
            raise CustomException('Model {0} does not have a default translation.'.format(str(self.id)))
        return defaultTranslation

    def __unicode__(self):
        try:
            return self.defaultTranslation.name
        except CustomException:
            return self.abbreviation

Changing the query to reference the id fixes the problem.

            defaultTranslation = ModelTranslation.objects.get(base_id=self.id, language__code=settings.FALLBACK_LANGUAGE)

Change History (6)

comment:1 Changed 2 years ago by carljm

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

Verified - accepting and marking release blocker, since this is a regression in 1.5.

I think the correct fix is to simply roll back https://github.com/django/django/commit/d5b93d3281fe93cbef5de84a52 - there's just too much risk in triggering arbitrary __unicode__ methods (that can do just about anything) deep in the guts of an ORM query execution, it's not worth the debugging benefits of that commit.

comment:2 Changed 2 years ago by akaariai

Interestingly the kwargs were passed to MultipleObjectsReturned even before my commit. So, changing that too seems like a good idea.

BTW the exception message isn't that useful anyways. The get parameters alone aren't good enough, the whole condition used by the query could be useful. This is really clear when using .latest() for example, this can result in DoesNotExist exception with lookup parameters of {}.

comment:3 Changed 2 years ago by carljm

Ah, good point about MultipleObjectsReturned. Yes, I agree that we should remove that too, for the same reason.

comment:4 Changed 23 months ago by Anssi Kääriäinen <akaariai@…>

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

In 266c0bb23e9d64c47ace4d162e582febd5a1e336:

Fixed #20278 -- ensured .get() exceptions do not recurse infinitely

A regression caused by d5b93d3281fe93cbef5de84a52 made .get() error
reporting recurse infinitely on certain rare conditions. Fixed this by
not trying to print the given lookup kwargs.

comment:5 Changed 23 months ago by Anssi Kääriäinen <akaariai@…>

In 0eddedf7db782a05e098c7024a9056aef7283b81:

[1.5.x] Fixed #20278 -- ensured .get() exceptions do not recurse infinitely

A regression caused by d5b93d3281fe93cbef5de84a52 made .get() error
reporting recurse infinitely on certain rare conditions. Fixed this by
not trying to print the given lookup kwargs.

Backpatch of 266c0bb23e9d64c47ace4d162e582febd5a1e336

comment:6 Changed 4 months ago by Tim Graham <timograham@…>

In d8f00e1918ce4df76920f3d79bc8d805fa69e29e:

Added a comment for test of refs #20278.

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