Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

panels(templates): avoid evaluating LazyObject #1833

Merged
merged 5 commits into from
Sep 26, 2023
Merged

Conversation

nijel
Copy link
Contributor

@nijel nijel commented Sep 19, 2023

Description

LazyObject is typically used for something expensive to evaluate, so avoid evaluating it just for showing it in the debug toolbar.

Fixes # (issue)

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

I will look at writing tests once I get an approval, that this is a viable approach.

@matthiask
Copy link
Member

Hey,

Thanks for your contribution!

I have questions:

  • SQL queries should be impossible anyway; which other expensive context variables could be wrapped by LazyObject in your experience? I myself am using lazy() for all sorts of things, but mostly to wrap objects which generate SQL queries or translations, so until now evaluating such an object once too much didn't bother me.
  • I had to read the code and I think that we still show the object if it has been evaluated already, is that correct?

@nijel
Copy link
Contributor Author

nijel commented Sep 19, 2023

For me, the reason was an expensive SQL query. I turned something possibly expensive (and rarely used) to a LazyObject (in WeblateOrg/weblate@d49c46e) and the query was still executed when running with Django debug toolbar. That took me some time to figure out that it's not my broken code, but the Django debug toolbar evaluates all template variables to display them.

SimpleLazyObject has __repr__ that shows the value if it has already been evaluated (see https://github.com/django/django/blob/8af3ae4ad9ca475f2428fac950de8df56b575e6a/django/utils/functional.py#L406-L413), that's why I used repr(). This should have been mentioned in the comment, I'll include that.

PS: lazy() seems to do a different thing than LazyObject, so this change covers LazyObject only.

@matthiask
Copy link
Member

Thanks for the explanation! Makes sense.

I will look at writing tests once I get an approval, that this is a viable approach.

I approve :)

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Two minor comments.

docs/changes.rst Outdated Show resolved Hide resolved
tests/panels/test_template.py Show resolved Hide resolved
- this makes it show evaluated querysets which were used in the template
- removes completely context processing when SHOW_TEMPLATE_CONTEXT is
  disabled
LazyObject is typically used for something expensive to evaluate, so avoid evaluating it just for showing it in the debug toolbar.
docs/changes.rst Outdated Show resolved Hide resolved
@matthiask matthiask merged commit 3ceb965 into jazzband:main Sep 26, 2023
21 checks passed
@matthiask
Copy link
Member

Thanks!

@nijel nijel deleted the patch-1 branch September 26, 2023 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants