Changing QuerySet.__repr__ to avoid unexpected queries

Django Internals ORM
1

Currently, calling repr on an unevaluated queryset runs the query on the database. While this is really useful in the django shell, it is a footgun in production code. I want to propose that we change the implementation so we only execute a query for an unevaluated queryset if we detect that we are in the interactive mode:

    def __repr__(self):
        # sys.ps1 only exists if we are in interactive mode
        if self._result_cache is not None or hasattr(sys, "ps1"):
            data = list(self[: REPR_OUTPUT_SIZE + 1])
            if len(data) > REPR_OUTPUT_SIZE:
                data[-1] = "...(remaining elements truncated)..."
            return "<%s %r>" % (self.__class__.__name__, data)
        return f"<{self.__class__.__name__} (unevaluated)>"

Fixing this footgun has been proposed in the past (#20393 (django.db.models.query.QuerySet.__repr__ should not have side-effects) – Django, #30863 (Queryset __repr__ can overload a database server in some cases) – Django, https://groups.google.com/g/django-developers/c/WzRZ0IfBipc) but the topic didn’t gain consensus.

The major objections have been:

  • This is very valuable behaviour for learning and debugging.
  • This causing problems is rare and has workarounds.

I am hoping that we can retain the current behaviour in the interactive shell while fixing this in other contexts (by checking for sys.ps1: Tell if Python is in interactive mode - Stack Overflow). I think there has been a steady stream of people hitting this problem in various contexts over the years and finding the current behaviour very surprising and difficult to diagnose, and in some cases annoying to work around, so I think this is worth fixing.

3 Likes
2

Thank you Lily for resurfacing this conversation. Reading all the past tickets and mailing list post has been very informative for me!

In my opinion, evaluating a queryset when calling repr feels like a Python antipattern, or if that’s too much of a strong statement, at least unexpected. I have always (naïvely?) assumed repr to be safe to be called in any context and under any load, specially when logging and debugging (safe as in no side-effects, no real code executed, resource-wise cheap).

Having said that, and having paid special attention to:

  1. how Python defincs/documents __repr__, and
  2. the previous arguments for the -1,

my suggestion is to follow the approach proposed by Lily but also show the SQL that is about to be executed (the latter was also suggested by some people in the original mail thread). Why this?

  • Python’s docs says that __repr__ should compute the “official” string representation of an object. If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment)

If the repr of a queryset includes its class name and its .query.sql_with_params(), one way to use that to recreate the object would be:

<ClassName as taken from repr>.objects.raw(*<ClassName.objects.all().query.sql_with_params() taken from repr> )

Con: do the params would need sanitization? Perhaps…
Con: Shall we show the query without params? we may loose some useful information
Con: What if the SQL query is enormous? that may be an issue, but would it be bigger than the representation of the 21 model instances?

Pros: when debugging, IMO, is extremely useful to know which query is being executed, more so in the context of a failure. What I mean is that, to me, an unevaluated queryset is the SQL to be executed. And seeing the SQL gives (me) more information than a list of <ClassName: ClassName object (pk)>.

  • The -1’s as summarized by Lily would be covered? attenuated? neutralized? if we consider some of the suggestions to keep evaluating the queryset when in the interactive shell, or when settings.DEBUG is true, or when the result is already cached. This way, we don’t need to update every example out there and we could just add a small clarification paragraph in the docs?

One other option would be to have a dedicated/newish settings value (:scream:), defaulting to True, that, when unset, querysets are not evaluated on repr calls. This way the world continues the same for most users while those needing this do not need to resort to monkeypatching. Also, debugging/tracing tools like Sentry and others, could advice on their docs to set this new value to False, or even check it to choose how to handle the printing of debug/stack information.

What do you think?

Thank you!

3 Likes
3

Are there other examples in popular python libraries where repr causes such a large side effect? As a user of django and many python libraries it does feel unexpected.

4

Thanks for the vote of confidence and for thoroughly exploring the design space here.

My current thinking is that we should:

  • Change __repr__ for unevaluated querysets to match @nessita’s suggested implementation.
  • Keep the current implementation for evaluated querysets (those with _result_cache set).
  • Detect whether we’re in interactive mode and if so, use the current implementation.
  • Optionally move the current implementation to __str__ and delegate to it from __repr__ for evaluated querysets and interactive mode.
  • Optionally add a new setting to allow a user to opt back in to the old behaviour (to smooth the upgrade process).

Con: do the params would need sanitization? Perhaps…

Do you have any examples of params you think might need sanitization? I would hope we can rely on the repr of params also being sensible.

Con: Shall we show the query without params? we may loose some useful information

I don’t have a strong opinion here and I think it would be reasonable to pick one and be open to changing to the other option in a later release

Con: What if the SQL query is enormous? that may be an issue, but would it be bigger than the representation of the 21 model instances?

We should probably have a character cutoff (1000 maybe?) on the outout of sql_with_params() when we interpolate it into the string.

5

Are there other examples in popular python libraries where repr causes such a large side effect? As a user of django and many python libraries it does feel unexpected.

I’m not aware of any other examples in Django. I have seen expensive reprs elsewhere, but I don’t remember the specific libraries.

6

I’m somewhat undecided on this one. I agree with everybody that we can’t just drop the evaluation altogether, as it’s too useful when debugging/learning/building, despite its large side effect.

I do agree with Lily and many others in the past that trying not to do this in production by default is a very good idea – it’s exactly the sort of thing I’d add to every of my project’s SensibleBaseModel class, and naturally I’d prefer if Django did that for me!

However, I think the best solution is to change behaviour based on settings.DEBUG, because that’s already a commonly used and established setting for this kind of thing. (I know, it’s an overloaded meaning – maybe a separate setting defaulting to DEBUG could also be argued for, and I don’t have a terribly strong opinion on that beyond disliking yet another setting.)

Here are some reasons against other proposed solutions, which imo leave settings.DEBUG as the best solution:

  • It’s been suggested to restrict the behaviour to one of str/repr. I’m not generally a fan of distinguishing between repr and str, which, I think, is down to me using tons of Django, and Django treating the two (largely) the same. Suddenly changing that behaviour would be not discoverable and would clash with how Django usually behaves.
  • Only print contents if the result cache has been evaluated. I think this is more footgun-y than not, as we all know that caching seems trivial at first, until it bites you. The mailinglist discussion Lily linked has some examples where printing a result cache would be surprising, which you really want to avoid in a potentially stressful debugging situation, where it’s easy to forget about odd caching.
  • The main proposal here, to rely on sys.ps1. Not only does this feel rather brittle to me, but, crucially, it would not allow you to str()-evaluate querysets in pdb, and I think that’s just as valid a use case as interactive shell debugging or print lines …
1 Like
7

Thanks @rixx for the thoughtful response.

However, I think the best solution is to change behaviour based on settings.DEBUG, because that’s already a commonly used and established setting for this kind of thing. (I know, it’s an overloaded meaning – maybe a separate setting defaulting to DEBUG could also be argued for, and I don’t have a terribly strong opinion on that beyond disliking yet another setting.)

For the use-case where I ran into this, settings.DEBUG would not be a good fit. Specifically, in Kolo (a dynamic code analysis tool I work on) we want to support users with DEBUG=True, but we also want to avoid generating extra queries while we introspect our users’ code. Currently we monkeypatch QuerySet.__repr__ (and if this lands we will still need to monkeypatch older Django versions), but long term I would love to drop that code.

I am open to putting this under a new setting, since we can document that as a strong recommendation in Kolo.

It’s been suggested to restrict the behaviour to one of str/repr. I’m not generally a fan of distinguishing between repr and str, which, I think, is down to me using tons of Django, and Django treating the two (largely) the same. Suddenly changing that behaviour would be not discoverable and would clash with how Django usually behaves.

  • The main proposal here, to rely on sys.ps1. Not only does this feel rather brittle to me, but, crucially, it would not allow you to str()-evaluate querysets in pdb, and I think that’s just as valid a use case as interactive shell debugging or print lines …

One of the reasons I was considering moving the current implementation to __str__ was to support the pdb debugging use-case.
As for sys.ps1 being too brittle, it is documented as “These are only defined if the interpreter is in interactive mode.” so I think it is safe to rely on this guarantee.

Only print contents if the result cache has been evaluated. I think this is more footgun-y than not, as we all know that caching seems trivial at first, until it bites you. The mailinglist discussion Lily linked has some examples where printing a result cache would be surprising, which you really want to avoid in a potentially stressful debugging situation, where it’s easy to forget about odd caching.

I don’t mind if we keep the current implementation when the result cache exists or if we move to the new repr @nessita suggested for all cases. I proposed keeping the current implementation to reduce the impact of the change, but I’m not too tied to it.

For the caching footgun, am I right that this is the example you’re considering?

I did hit one odd case while writing the tests. Essentially, if you do:

queryset = SomeModel.objects.all()
list(queryset)
SomeModel.objects.create(...)
repr(queryset)

The newly created model won’t appear in the repr. I believe the existing behavior will show the newly created object. I’m still not a huge fan of this repr behavior (I lean towards just displaying the SQL like RawQuerySet).

I haven’t tested this, but I would personally be more surprised if the example code above did include the newly created model.

8

First off – the caching footgun I was thinking about is the one you quoted – and, reconsidering, maybe I was too easily convinced by the original argument and cached display is actually fine. I agree with you that a changed output would be beyond weird, so showing cached content is probably fine!

I didn’t check the Python docs – I retract the point of sys.ps1 being brittle, but would still want to see support for interactive debuggers (that don’t set sys.ps1, but do have sys.gettrace, apparently). I also think we need to keep print statements in DEBUG=True working as per usual, as people use print statements just as much as interactive debugging, and taking that away is not something we should do.

I believe this means that we should either work with settings.DEBUG even if it doesn’t satisfy Kolo’s requirements, or add a new setting, or distinguish between str and repr.

So, in the end, I’m in favour of any solution that allows to prevent string representation on querysets from running queries, particularly in production, while also defaulting to the current level (or similar) of representation in interactive shells and debuggers (and, if possible, print statements in dev mode).

2 Likes
9

@nessita Do you think we can move forward with this or do we need to wait for more responses?

10

My unsolicited advice would be to at least see Mariusz & Simon (and maybe James) opinions as they’re current active contributors who were initially against this.

11

Gonna try to argue here from a “green slate”-perspective with python ideas in mind:

There is a reason, why python separates __str__ from __repr__, so it could be used here as an advantage to separate the concerns. Which directly boils down to the question what str(queryset) vs. repr(queryet) should be like, suggestion:

  • str(queryset) - Imho we need a way, what a queryset might look like. This could be done by str eval, which is also common to do heavy lifting and is not strictly side-effect free. Here we could just keep the current inspection logic. (And no - it should not be isatty or PS1 dependent at all, it should be reliable under any calling circumstances…)
  • repr(queryset) - This is quite the opposite regarding heavy lifting and side effects. Normally one would not expect this to do anything like that, as python devs using it most likely are interested in its type str and/or hints regarding its behavior for the heavy lifting. E.g. for querysets this could just print something like QuerySet<model: ...., ...> or maybe even with its SQL string or appropriate exception (e.g. QuerySet<model: ...., query: "SELECT ...."> or QuerySet<model: ...., exception: "not constructable for xy reason">).

I am pretty sure, thats not fully doable for the existing ORM, as it established expectations
around its str/repr outputs for a long time, thus has to respects habits (also used by meta tooling). I still strongly would like to argue against making this further isatty or PS1 dependent, as this is really surprising behavior when testing it in the interactive shell vs. other invocations (stumbled over this several times already).

1 Like
12

I agree with @shangxiao that we may need to allow for some extra time for the initial set of people who were not on board to share their views on the latest proposal. I believe this may address their concerns, but let’s reach out and ask them! @felixxm @carltongibson @charettes @ubernostrum what do you think?

13

Grrr. It’s difficult. :thinking: Reading this I find myself agreeing with @rixx almost entirely.

<likely-not-helpful>
If I were to do something very crude here, I’d be inclined to add a _evalute_repr boolean to QuerySet, and branch on that in the __repr__() implementation. Tools like Kolo could set it False before doing their thing. Folks, who quite likely already have QuerySet subclasses in play, could do similar if they don’t like the default behaviour. It might be that that was enough.
</likely-not-helpful>

It’s enough of a pain that we should probably do something. (Update: although see @felixxm’s reply)

1 Like
14

I am still against changing __repr__() for exactly the same reasons I mentioned in the mailing list thread :person_shrugging: . First of all, the current long-term behavior can only cause issues in a really specific cases. Secondly, using __repr__() in a production code outside tests and interactive shell sounds unusual. Also, checking ps1 is a bit hacky and will brake tests based on a queryset representation.

Last but not least, if fetching 20 rows is an issue, folks can change the django.db.models.query.REPR_OUTPUT_SIZE constant, e.g.

import django.db.models.query

django.db.models.query.REPR_OUTPUT_SIZE = 0

My 2¢

Best,
Mariusz

1 Like
15

Proposals to have __repr__() include some sort of way to “reconstruct” the query always run into the fact that there are edge cases in that direction, too – there might be single-use iterators involved that are just gone once __repr__() has consumed them, for example, as brought up in the mailing-list thread.

So I don’t think Django can usefully do that.

But I want to try to get at the reason why this is coming up over and over. There’s a requested solution (change the __repr__() behavior), but I still have trouble finding a clear statement of the problem. Mostly the motivation seems to be either that the query is unexpected, or that the query is overloading the DB.

I tend to agree with the prior mailing-list discussion that the case of overloading the DB is not really something Django can solve: if evaluating a query with a LIMIT 21 brings down the database, that’s an issue with the query or with the DB design. Even if it’s coming down because the query is being repeated a huge number of times, that’s an issue with whatever’s triggering it (error-logging code, for example, should probably be doing something like exponential backoff anyway as a best practice because theoretically anything you try to evaluate in an error handler could be expensive or cause further problems).

Which leaves only the unexpected-behavior issue. I think that could probably be handled as a documentation issue rather than a technical issue.

If there’s another problem I’m missing here, please let me know. As-is, I think the behavior is too useful for learning/debugging to get rid of, and likely not something where Django can auto-detect when and whether to do. As an absolute last resort I suppose there is the suggested option to put an explicit “don’t evaluate when __repr__()-ing” flag on QuerySet, but that feels non-discoverable and likely wouldn’t help with situations where Library A wants Application B’s model to have that flag set.

16

As an absolute last resort I suppose there is the suggested option to put an explicit “don’t evaluate when __repr__()-ing” flag on QuerySet, but that feels non-discoverable and likely wouldn’t help with situations where Library A wants Application B’s model to have that flag set.

I think django.db.models.query.REPR_OUTPUT_SIZE could be a good candidate for that as @felixxm pointed out. It doesn’t solve the Library A wants Application B’s model problem but from skimming through the previous discussion this is rarely requested.

With trivial changes to QuerySet.__repr__ to make it a noop when the value is set to 0 (it currently still evaluates the query) it seems like it could provide an escape hatch in the cases where the system is strongly opinionated about disabling this feature.

2 Likes
17

There’s a requested solution (change the __repr__() behavior), but I still have trouble finding a clear statement of the problem. Mostly the motivation seems to be either that the query is unexpected, or that the query is overloading the DB.

I ran into this in the context of dynamic code analysis, using sys.setprofile (sys.settrace would be similar). In Kolo, we introspect a user’s program as it runs to extract interesting information about it. Specifically, we look at the local variables in each function call of our user’s code and we record their repr when they’re not straightforwardly serializable as JSON. This led to a bug report about Kolo generating extra queries in a user’s code: Middleware still causes extra queries · Issue #4 · kolofordjango/kolo · GitHub (I think the earlier report was through a private channel.)

I was able to work around this by monkeypatching Queryset.__repr__, but since other people have run into issues, I’m proposing this change upstream.

This is the same sort of problem that the original poster in the django-developers mailing list ran into: https://groups.google.com/g/django-developers/c/WzRZ0IfBipc/m/04FBC8IzAQAJ

The argument that you can work around it by not calling repr is certainly true, but in practice, I don’t think it’s reasonable. Production error reporting tools (like Sentry and New Relic) will call repr when reporting on exceptions.

And the reporters of the two tickets: https://code.djangoproject.com/ticket/20393:

In trying to track down some timeouts and strange queries in our apps we saw some strange queries being run by django. We were querying for a single record with a FOR UPDATE, getting an error, then django would run the same query 3 times but with a limit of 21 and then fetch all of the records, each time.

Eventually I tracked this down to our error-handling middleware calling repr() on a QuerySet, which then queried the database. __repr__ is supposed to be a string-representation of an object in its current state and should not gave side-effects like making database queries, especially in the case where the query uses FOR UPDATE or similar logic.

And https://code.djangoproject.com/ticket/30863:

Django will throw an exception (rightfully so).

As part of the usual error reporting process in debug mode, Django may eventually call repr() on the “base” queryset (that is essentially Result.objects.all()).

QuerySet.repr tries to be helpful by printing the first 21 results of the evaluated query. Because the base queryset orders by the un-indexed “name” column, this can easily overload the database when it does “SELECT … FROM Result ORDER BY name LIMIT 21” (trying to sort hundreds of millions of rows by an unindexed column)

Even with debug mode turned off, some error reporting tools like Sentry will call repr on the queryset, creating the same problem in production.

Personally I think that __repr__ should not do IO. I do however, acknowledge the practical benefit of the current implementation when working in interactive mode, which is why I suggested the change I did.

Which leaves only the unexpected-behavior issue. I think that could probably be handled as a documentation issue rather than a technical issue.

Documentation means that authors of tools like Kolo (and potentially Sentry and Newrelic - I haven’t checked how they handle this) will continue to need to monkeypatch Queryset.

If there’s another problem I’m missing here, please let me know. As-is, I think the behavior is too useful for learning/debugging to get rid of, and likely not something where Django can auto-detect when and whether to do. As an absolute last resort I suppose there is the suggested option to put an explicit “don’t evaluate when __repr__()-ing” flag on QuerySet, but that feels non-discoverable and likely wouldn’t help with situations where Library A wants Application B’s model to have that flag set.

I think Django can detect when to do this. Specifically:

  • In interactive mode with sys.ps1.
  • In print debugging by moving the current implementation to __str__.
  • In pdb debugging by moving the implementation to __str__ - users can call str or print in their debugging session.

I think the rest of the time it’s preferable to avoid doing unexpected queries, or the solution to one of the above cases applies.

18

First of all, the current long-term behavior can only cause issues in a really specific cases.

This feels like the definition of a footgun to me - it’s fine a lot of the time but then can really bite you unexpectedly.

Secondly, using __repr__() in a production code outside tests and interactive shell sounds unusual.

I think it’s pretty reasonable in logging, tracing or error reporting code.

Also, checking ps1 is a bit hacky and will brake tests based on a queryset representation.

Breaking existing tests is something I hadn’t considered. I think we have a reasonable upgrade path though, if we move the current implementation to __str__ and potentially add a backwards compatibility setting.

19

If I were to do something very crude here, I’d be inclined to add a _evalute_repr boolean to QuerySet, and branch on that in the __repr__() implementation. Tools like Kolo could set it False before doing their thing. Folks, who quite likely already have QuerySet subclasses in play, could do similar if they don’t like the default behaviour. It might be that that was enough.

This isn’t my ideal solution when thinking from Kolo’s perspective. I’d end up toggling the flag every time we enter and exit the sys.setprofile callback, which is extra overhead. It would be cleaner than the current monkeypatch though.

20

I looked into Sentry and discovered they had to workaround this in version 0.7.0:

Do not evaluate Django QuerySet when trying to capture local variables. Also an internal hook was added to overwrite repr for local vars.