Opened 12 years ago

Closed 11 years ago

Last modified 10 years 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 by Carl Meyer, 12 years ago

Component: UncategorizedDatabase layer (models, ORM)
Easy pickings: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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 by Anssi Kääriäinen, 12 years ago

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 by Carl Meyer, 12 years ago

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

comment:4 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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 by Tim Graham <timograham@…>, 10 years ago

In d8f00e1918ce4df76920f3d79bc8d805fa69e29e:

Added a comment for test of refs #20278.

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