Skip to content

Commit c296a27

Browse files
Yun-Kimwconti27
andauthored
fix(django): guard against cache truthiness [backport #5477 to 1.12] (#5646)
Fixes #5460 Add guard against when a Django cache result has ambiguous truth because the class doesn't implement the __bool__() function. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines) are followed. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] PR description includes explicit acknowledgement/acceptance of the performance implications of this PR as reported in the benchmarks PR comment. ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. Co-authored-by: William Conti <[email protected]>
1 parent 8b7d216 commit c296a27

File tree

3 files changed

+46
-1
lines changed

3 files changed

+46
-1
lines changed

ddtrace/contrib/django/patch.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,8 +179,9 @@ def traced_cache(django, pin, func, instance, args, kwargs):
179179
elif command_name == "get":
180180
# if valid result and check for special case for Django~3.0 that returns an empty Sentinel object as
181181
# missing key
182-
if result and result != getattr(instance, "_missing_key", None):
182+
if result is not None and result != getattr(instance, "_missing_key", None):
183183
span.set_metric(db.ROWCOUNT, 1)
184+
# else result is invalid or None, set row count to 0
184185
else:
185186
span.set_metric(db.ROWCOUNT, 0)
186187
return result
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
django: Adds fix for bug where Django cache return object throws an error if it does not implement ``__bool__()``.

tests/contrib/django/test_django.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -771,6 +771,46 @@ def test_cache_get_rowcount_missing_key(test_spans):
771771
assert_dict_issuperset(span.get_metrics(), {"db.row_count": 0})
772772

773773

774+
class NoBool:
775+
def __bool__(self):
776+
raise NotImplementedError
777+
778+
779+
def test_cache_get_rowcount_empty_key(test_spans):
780+
# get the default cache
781+
cache = django.core.cache.caches["default"]
782+
cache.set(1, NoBool())
783+
784+
result = cache.get(1)
785+
786+
assert isinstance(result, NoBool) is True
787+
788+
spans = test_spans.get_spans()
789+
assert len(spans) == 2
790+
791+
get_span = spans[1]
792+
assert get_span.service == "django"
793+
assert get_span.resource == "django.core.cache.backends.locmem.get"
794+
795+
assert_dict_issuperset(get_span.get_metrics(), {"db.row_count": 1})
796+
797+
798+
def test_cache_get_rowcount_missing_key_with_default(test_spans):
799+
# get the default cache
800+
cache = django.core.cache.caches["default"]
801+
802+
# This is the diff with `test_cache_get_rowcount_missing_key`,
803+
# we are setting a default value to be returned in case of a cache miss
804+
cache.get("missing_key", default="default_value")
805+
806+
spans = test_spans.get_spans()
807+
assert len(spans) == 1
808+
span = spans[0]
809+
assert span.service == "django"
810+
assert span.resource == "django.core.cache.backends.locmem.get"
811+
assert_dict_issuperset(span.get_metrics(), {"db.row_count": 1})
812+
813+
774814
def test_cache_get_unicode(test_spans):
775815
# get the default cache
776816
cache = django.core.cache.caches["default"]

0 commit comments

Comments
 (0)