-
Notifications
You must be signed in to change notification settings - Fork 223
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
#2114 repeating query in django view #2158
base: main
Are you sure you want to change the base?
#2114 repeating query in django view #2158
Conversation
💚 CLA has been signed |
cla/check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't poke much with Django internals, also we should not assume django is installed
elasticapm/utils/encoding.py
Outdated
elif isinstance(value, QuerySet): | ||
value._result_cache = [] | ||
ret = repr(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif isinstance(value, QuerySet): | |
value._result_cache = [] | |
ret = repr(value) | |
elif DjangoQuerySet is not None and isinstance(value, DjangoQuerySet) and getattr(value, "_result_cache", True) is None: | |
# if we have a Django QuerySet a None result cache it may mean that the underlying query failed | |
# so represent it as an empty list instead of retrying the query again | |
ret = "<%s %r>" % (value.__class__.__name__, []) |
Instead of erasing the values everytime shouldn't we just do something is value._result_cache is None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, there is a little less magic to your solution 👌
elasticapm/utils/encoding.py
Outdated
@@ -36,6 +36,7 @@ | |||
import uuid | |||
from decimal import Decimal | |||
|
|||
from django.db.models import QuerySet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from django.db.models import QuerySet | |
try: | |
from django.db.models import QuerySet as DjangoQuerySet | |
except ImportError: | |
DjangoQuerySet = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point i will change it 👌
What does this pull request do?
In this PR i add another condition to
elif
statement inutils.encoding.transform
function to handle displaying representation of django QuerySet object in custom way. We need to do it because if we try to executerepr
function on unexecutedQuerySet
which causes Timeout on database, it will execute byutils.encoding.transform
function.I also added a simple test simulating database Timeout.
By overwriting
_result_cache
on django QuerySet object we ensure that django will not try to execute any query to db.Related issues
Closes #2114