From ba41da0907afb22c5c35785205256c7382abd501 Mon Sep 17 00:00:00 2001 From: Kyle Verhoog Date: Mon, 24 Aug 2020 16:42:25 -0400 Subject: [PATCH] django: reintroduce wrapt for view method patching (#1622) Since https://github.com/GrahamDumpleton/wrapt/issues/153 was fixed, we can use wrapt again for the view method patching. --- ddtrace/compat.py | 98 ------------------- ddtrace/contrib/django/patch.py | 9 +- tests/contrib/django/django1_app/urls.py | 2 + tests/contrib/django/django_app/urls.py | 2 + .../django/templates/custom_dispatch.html | 1 + tests/contrib/django/test_django.py | 33 +++++++ tests/contrib/django/views.py | 36 +++++++ 7 files changed, 76 insertions(+), 105 deletions(-) create mode 100644 tests/contrib/django/templates/custom_dispatch.html diff --git a/ddtrace/compat.py b/ddtrace/compat.py index 68975eb3919..606dd06eb08 100644 --- a/ddtrace/compat.py +++ b/ddtrace/compat.py @@ -147,104 +147,6 @@ def make_async_decorator(tracer, fn, *params, **kw_params): return fn -# static version of getattr backported from Python 3.7 -try: - from inspect import getattr_static -except ImportError: - import types - - _sentinel = object() - - def _static_getmro(klass): - return type.__dict__["__mro__"].__get__(klass) - - def _check_instance(obj, attr): - instance_dict = {} - try: - instance_dict = object.__getattribute__(obj, "__dict__") - except AttributeError: - pass - return dict.get(instance_dict, attr, _sentinel) - - def _check_class(klass, attr): - for entry in _static_getmro(klass): - if _shadowed_dict(type(entry)) is _sentinel: - try: - return entry.__dict__[attr] - except KeyError: - pass - return _sentinel - - def _is_type(obj): - try: - _static_getmro(obj) - except TypeError: - return False - return True - - def _shadowed_dict(klass): - dict_attr = type.__dict__["__dict__"] - for entry in _static_getmro(klass): - try: - class_dict = dict_attr.__get__(entry)["__dict__"] - except KeyError: - pass - else: - if not ( - type(class_dict) is types.GetSetDescriptorType # noqa: E721 - and class_dict.__name__ == "__dict__" # noqa: E721,E261,W504 - and class_dict.__objclass__ is entry # noqa: E261,W504 - ): - return class_dict - return _sentinel - - def getattr_static(obj, attr, default=_sentinel): - """Retrieve attributes without triggering dynamic lookup via the - descriptor protocol, __getattr__ or __getattribute__. - - Note: this function may not be able to retrieve all attributes - that getattr can fetch (like dynamically created attributes) - and may find attributes that getattr can't (like descriptors - that raise AttributeError). It can also return descriptor objects - instead of instance members in some cases. See the - documentation for details. - """ - instance_result = _sentinel - if not _is_type(obj): - klass = type(obj) - dict_attr = _shadowed_dict(klass) - if dict_attr is _sentinel or type(dict_attr) is types.MemberDescriptorType: # noqa: E261,E721,W504 - instance_result = _check_instance(obj, attr) - else: - klass = obj - - klass_result = _check_class(klass, attr) - - if instance_result is not _sentinel and klass_result is not _sentinel: - if ( - _check_class(type(klass_result), "__get__") is not _sentinel - and _check_class(type(klass_result), "__set__") is not _sentinel # noqa: W504,E261,E721 - ): - return klass_result - - if instance_result is not _sentinel: - return instance_result - if klass_result is not _sentinel: - return klass_result - - if obj is klass: - # for types we check the metaclass too - for entry in _static_getmro(type(klass)): - if _shadowed_dict(type(entry)) is _sentinel: - try: - return entry.__dict__[attr] - except KeyError: - pass - if default is not _sentinel: - return default - raise AttributeError(attr) - - # DEV: There is `six.u()` which does something similar, but doesn't have the guard around `hasattr(s, 'decode')` def to_unicode(s): """ Return a unicode string for the given bytes or string instance. """ diff --git a/ddtrace/contrib/django/patch.py b/ddtrace/contrib/django/patch.py index 3941c528610..97534f9094c 100644 --- a/ddtrace/contrib/django/patch.py +++ b/ddtrace/contrib/django/patch.py @@ -12,7 +12,6 @@ from ddtrace import config, Pin from ddtrace.vendor import debtcollector, six, wrapt -from ddtrace.compat import getattr_static from ddtrace.constants import ANALYTICS_SAMPLE_RATE_KEY from ddtrace.contrib import func_name, dbapi from ddtrace.ext import http, sql as sqlx, SpanTypes @@ -488,17 +487,13 @@ def instrument_view(django, view): for name in list(http_method_names) + list(lifecycle_methods): try: # View methods can be staticmethods - func = getattr_static(view, name, None) + func = getattr(view, name, None) if not func or isinstance(func, wrapt.ObjectProxy): continue resource = "{0}.{1}".format(func_name(view), name) op_name = "django.view.{0}".format(name) - - # Set attribute here rather than using wrapt.wrappers.wrap_function_wrapper - # since it will not resolve attribute to staticmethods - wrapper = wrapt.FunctionWrapper(func, traced_func(django, name=op_name, resource=resource)) - setattr(view, name, wrapper) + wrap(view, name, traced_func(django, name=op_name, resource=resource)) except Exception: log.debug("Failed to instrument Django view %r function %s", view, name, exc_info=True) diff --git a/tests/contrib/django/django1_app/urls.py b/tests/contrib/django/django1_app/urls.py index f184ee31edb..d5e81e9e0b3 100644 --- a/tests/contrib/django/django1_app/urls.py +++ b/tests/contrib/django/django1_app/urls.py @@ -20,4 +20,6 @@ url(r"^template-view/$", views.template_view, name="template-view"), url(r"^template-simple-view/$", views.template_simple_view, name="template-simple-view"), url(r"^template-list-view/$", views.template_list_view, name="template-list-view"), + url(r"^composed-template-view/$", views.ComposedTemplateView.as_view(), name="composed-template-view"), + url(r"^composed-get-view/$", views.ComposedGetView.as_view(), name="composed-get-view"), ] diff --git a/tests/contrib/django/django_app/urls.py b/tests/contrib/django/django_app/urls.py index e46686c2bf2..7d752d848b6 100644 --- a/tests/contrib/django/django_app/urls.py +++ b/tests/contrib/django/django_app/urls.py @@ -49,4 +49,6 @@ def authenticated_view(request): re_path(r"re-path.*/", repath_view), path("path/", path_view), path("include/", include("tests.contrib.django.django_app.extra_urls")), + url(r"^composed-template-view/$", views.ComposedTemplateView.as_view(), name="composed-template-view"), + url(r"^composed-get-view/$", views.ComposedGetView.as_view(), name="composed-get-view"), ] diff --git a/tests/contrib/django/templates/custom_dispatch.html b/tests/contrib/django/templates/custom_dispatch.html new file mode 100644 index 00000000000..6d187e84101 --- /dev/null +++ b/tests/contrib/django/templates/custom_dispatch.html @@ -0,0 +1 @@ +custom dispatch {{ dispatch_call_counter }} diff --git a/tests/contrib/django/test_django.py b/tests/contrib/django/test_django.py index 1c911f77016..3100f80a279 100644 --- a/tests/contrib/django/test_django.py +++ b/tests/contrib/django/test_django.py @@ -1363,3 +1363,36 @@ def test_django_use_handler_resource_format(client, test_spans): resource = "GET tests.contrib.django.views.index" root.assert_matches(resource=resource, parent_id=None, span_type="http") + + +def test_custom_dispatch_template_view(client, test_spans): + """ + Test that a template view with a custom dispatch method inherited from a + mixin is called. + """ + resp = client.get("/composed-template-view/") + assert resp.status_code == 200 + assert resp.content.strip() == b"custom dispatch 2" + + spans = test_spans.get_spans() + assert [s.resource for s in spans if s.resource.endswith("dispatch")] == [ + "tests.contrib.django.views.ComposedTemplateView.dispatch", + ] + + +def test_custom_dispatch_get_view(client, test_spans): + """ + Test that a get method on a view with a custom dispatch method inherited + from a mixin is called. + """ + resp = client.get("/composed-get-view/") + assert resp.status_code == 200 + assert resp.content.strip() == b"custom get" + + spans = test_spans.get_spans() + assert [s.resource for s in spans if s.resource.endswith("dispatch")] == [ + "tests.contrib.django.views.ComposedGetView.dispatch", + ] + assert [s.resource for s in spans if s.resource.endswith("get")] == [ + "tests.contrib.django.views.ComposedGetView.get", + ] diff --git a/tests/contrib/django/views.py b/tests/contrib/django/views.py index 1b9f811dbc8..a393f856b60 100644 --- a/tests/contrib/django/views.py +++ b/tests/contrib/django/views.py @@ -106,3 +106,39 @@ def template_list_view(request): For testing resolving a list of templates """ return TemplateResponse(request, ["doesntexist.html", "basic.html"]) + + +class CustomDispatchMixin(View): + def dispatch(self, request): + self.dispatch_call_counter += 1 + return super(CustomDispatchMixin, self).dispatch(request) + + +class AnotherCustomDispatchMixin(View): + def dispatch(self, request): + self.dispatch_call_counter += 1 + return super(AnotherCustomDispatchMixin, self).dispatch(request) + + +class ComposedTemplateView(TemplateView, CustomDispatchMixin, AnotherCustomDispatchMixin): + template_name = "custom_dispatch.html" + dispatch_call_counter = 0 + + def get_context_data(self, **kwargs): + context = super(ComposedTemplateView, self).get_context_data(**kwargs) + context['dispatch_call_counter'] = self.dispatch_call_counter + return context + + +class CustomGetView(View): + def get(self, request): + return HttpResponse("custom get") + + +class ComposedGetView(CustomGetView, CustomDispatchMixin): + dispatch_call_counter = 0 + + def get(self, request): + if self.dispatch_call_counter == 1: + return super(ComposedGetView, self).get(request) + raise Exception("Custom dispatch not called.")