fix: reenable instrumentation of properties

Have APPMAP_INSTRUMENT_PROPERTIES default to true once again.

Special-case properties that have operator.attrgetter as their getter.
Django uses this pattern extensively, and this special-case fixes some
failures when trying to generate AppMap data for Django tests.

Author: apotterri

_appmap/event.py

@@ -1,6 +1,7 @@
 # pylint: disable=missing-module-docstring,missing-class-docstring,missing-function-docstring
 import inspect
 import logging
+import operator
 import threading
 from functools import lru_cache, partial
 from inspect import Parameter, Signature
@@ -180,18 +181,19 @@ class CallEvent(Event):
     __slots__ = ["_fn", "_fqfn", "static", "receiver", "parameters", "labels", "auxtype"]
 
     @staticmethod
-    def make(fn, fntype):
+    def make(filterable):
         """
         Return a factory for creating new CallEvents based on
         introspecting the given function.
         """
 
         # Delete the labels so the app doesn't see them.
+        fn = filterable.obj
         labels = getattr(fn, "_appmap_labels", None)
         if labels:
             del fn._appmap_labels
 
-        return partial(CallEvent, fn, fntype, labels=labels)
+        return partial(CallEvent, filterable, labels=labels)
 
     @staticmethod
     def make_params(filterable):
@@ -312,10 +314,28 @@ def comment(self):
             comment = inspect.getcomments(self._fn)
         return comment
 
-    def __init__(self, fn, fntype, parameters, labels):
+    def __init__(self, filterable, parameters, labels):
         super().__init__("call")
+        fn = filterable.obj
         self._fn = fn
-        self._fqfn = FqFnName(fn)
+        if type(fn) is not operator.attrgetter:
+            # fn is a regular function
+            modname = fn.__module__
+            qualname = fn.__qualname__
+        elif (parts := filterable.fqname.split(".")) and len(parts) > 2:
+            # fn is an attrgetter, which will is being used as the getter for a property. If
+            # filterable.fqname has enough components to be the fully-qualified name of a class
+            # member, set the module name and qualname based on those components.
+            modname = ".".join(parts[:-2])
+            qualname = ".".join(parts[-2:])
+        else:
+            # The two previous cases should handle all known possibilities, but don't crash if
+            # somethign else sneaks in.
+            modname = "unknown"
+            qualname = "unknown"
+        self._fqfn = FqFnName(modname, qualname)
+
+        fntype = filterable.fntype
         self.static = fntype in FnType.STATIC | FnType.CLASS | FnType.MODULE
         self.receiver = None
         if fntype in FnType.CLASS | FnType.INSTANCE:

_appmap/importer.py

@@ -134,7 +134,7 @@ def is_member_func(m):
         static_value = inspect.getattr_static(cls, key)
         # Don't use isinstance to check the type of static_value -- we don't want to invoke the
         # descriptor protocol.
-        if Importer.instrument_properties and type(static_value) is property:  # pylint: disable=unidiomatic-typecheck
+        if Importer.instrument_properties and type(static_value) is property:
             properties[key] = (
                 static_value,
                 {
@@ -167,7 +167,7 @@ def initialize(cls):
         cls.filter_chain = []
         cls._skip_instrumenting = ("appmap", "_appmap")
         cls.instrument_properties = (
-            Env.current.get("APPMAP_INSTRUMENT_PROPERTIES", "false").lower() == "true"
+            Env.current.get("APPMAP_INSTRUMENT_PROPERTIES", "true").lower() == "true"
         )
 
     @classmethod
@@ -221,7 +221,8 @@ def instrument_functions(filterable, selected_functions=None):
                     new_fn = cls.instrument_function(prop_name, filterableFn, selected_functions)
                     if new_fn != fn:
                         new_fn = wrapt.FunctionWrapper(fn, new_fn)
-                        # Set _appmap_instrumented on the FunctionWrapper, not on the wrapped function
+                        # Set _appmap_instrumented on the FunctionWrapper, not on the wrapped
+                        # function.
                         new_fn._appmap_instrumented = True  # pylint: disable=protected-access
 
                     instrumented_fns[k] = new_fn

_appmap/instrument.py

@@ -121,7 +121,7 @@ def instrument(filterable):
     logger.debug("hooking %s", filterable.fqname)
     fn = filterable.obj
 
-    make_call_event = event.CallEvent.make(fn, filterable.fntype)
+    make_call_event = event.CallEvent.make(filterable)
     params = CallEvent.make_params(filterable)
 
     # django depends on being able to find the cache_clear attribute

_appmap/test/data/appmap.yml

@@ -4,6 +4,7 @@ packages:
 - path: example_class.Super
   shallow: true
 - path: example_class
+- path: properties_class
 - path: appmap_testing
 - path: package1
 - dist: PyYAML

_appmap/test/data/example_class.py

@@ -114,55 +114,6 @@ def with_comment(self):
     def return_self(self):
         return self
 
-    def __init__(self):
-        self._read_only = "read only"
-        self._fully_accessible = "fully accessible"
-        self._undecorated = "undecorated"
-
-    @property
-    def read_only(self):
-        """Read-only"""
-        return self._read_only
-
-    @property
-    def fully_accessible(self):
-        """Fully-accessible"""
-        return self._fully_accessible
-
-    @fully_accessible.setter
-    def fully_accessible(self, v):
-        self._fully_accessible = v
-
-    @fully_accessible.deleter
-    def fully_accessible(self):
-        del self._fully_accessible
-
-    def get_undecorated(self):
-        return self._undecorated
-
-    def set_undecorated(self, value):
-        self._undecorated = value
-
-    def delete_undecorated(self):
-        del self._undecorated
-
-    undecorated_property = property(get_undecorated, set_undecorated, delete_undecorated)
-
-    def set_write_only(self, v):
-        self._write_only = v
-
-    def del_write_only(self):
-        del self._write_only
-
-    write_only = property(None, set_write_only, del_write_only, "Write-only")
-
-    def raise_base_exception(self) -> NoReturn:
-        raise BaseException("not derived from Exception") # pylint: disable=broad-exception-raised
 
 def modfunc():
     return "Hello world!"
-
-if __name__ == "__main__":
-    ec = ExampleClass()
-    ec.fully_accessible = "updated"
-    print(ec.fully_accessible)

_appmap/test/data/properties_class.py

@@ -0,0 +1,56 @@
+from functools import cached_property
+import operator
+from typing import NoReturn
+
+
+class PropertiesClass:
+    def __init__(self):
+        self._read_only = "read only"
+        self._fully_accessible = "fully accessible"
+        self._undecorated = "undecorated"
+
+    @property
+    def read_only(self):
+        """Read-only"""
+        return self._read_only
+
+    @property
+    def fully_accessible(self):
+        """Fully-accessible"""
+        return self._fully_accessible
+
+    @fully_accessible.setter
+    def fully_accessible(self, v):
+        self._fully_accessible = v
+
+    @fully_accessible.deleter
+    def fully_accessible(self):
+        del self._fully_accessible
+
+    def get_undecorated(self):
+        return self._undecorated
+
+    def set_undecorated(self, value):
+        self._undecorated = value
+
+    def delete_undecorated(self):
+        del self._undecorated
+
+    undecorated_property = property(get_undecorated, set_undecorated, delete_undecorated)
+
+    def set_write_only(self, v):
+        self._write_only = v
+
+    def del_write_only(self):
+        del self._write_only
+
+    write_only = property(None, set_write_only, del_write_only, "Write-only")
+
+    def raise_base_exception(self) -> NoReturn:
+        raise BaseException("not derived from Exception")  # pylint: disable=broad-exception-raised
+
+    @cached_property
+    def cached_read_only(self):
+        return self._read_only
+
+    operator_read_only = property(operator.attrgetter("cached_read_only"))

_appmap/test/test_params.py

@@ -28,8 +28,7 @@ def __init__(self, C):
 
     @classmethod
     def prepare(cls, ffn):
-        fn = ffn.obj
-        make_call_event = CallEvent.make(fn, ffn.fntype)
+        make_call_event = CallEvent.make(ffn)
         params = CallEvent.make_params(ffn)
 
         def wrapped_fn(_, instance, args, kwargs):

_appmap/test/test_properties.py

@@ -5,47 +5,43 @@
 import pytest
 from _appmap.test.helpers import DictIncluding
 
-pytestmark = pytest.mark.skip(reason="property instrumentation is broken in Django")
+pytestmark = pytest.mark.appmap_enabled
 
 @pytest.fixture(autouse=True)
 def setup(with_data_dir):  # pylint: disable=unused-argument
-    # with_data_dir sets up sys.path so example_class can be imported
+    # with_data_dir sets up sys.path so properties_class can be imported
     pass
 
 
 def test_getter_instrumented(events):
-    from example_class import ExampleClass
+    from properties_class import PropertiesClass
 
-    ec = ExampleClass()
+    ec = PropertiesClass()
 
-    actual = ExampleClass.read_only.__doc__
+    actual = PropertiesClass.read_only.__doc__
     assert actual == "Read-only"
 
     assert ec.read_only == "read only"
 
     with pytest.raises(AttributeError, match=r".*(has no setter|can't set attribute).*"):
-        # E           AttributeError: can't set attribute
-
         ec.read_only = "not allowed"
 
     with pytest.raises(AttributeError, match=r".*(has no deleter|can't delete attribute).*"):
         del ec.read_only
 
     assert len(events) == 2
-    assert events[0].to_dict() == DictIncluding(
-        {
-            "event": "call",
-            "defined_class": "example_class.ExampleClass",
-            "method_id": "read_only (get)",
-        }
-    )
+    assert events[0].to_dict() == DictIncluding({
+        "event": "call",
+        "defined_class": "properties_class.PropertiesClass",
+        "method_id": "read_only (get)",
+    })
 
 
 def test_accessible_instrumented(events):
-    from example_class import ExampleClass
+    from properties_class import PropertiesClass
 
-    ec = ExampleClass()
-    assert ExampleClass.fully_accessible.__doc__ == "Fully-accessible"
+    ec = PropertiesClass()
+    assert PropertiesClass.fully_accessible.__doc__ == "Fully-accessible"
 
     assert ec.fully_accessible == "fully accessible"
 
@@ -55,48 +51,61 @@ def test_accessible_instrumented(events):
 
     del ec.fully_accessible
 
-    # assert len(events) == 6
-    assert events[0].to_dict() == DictIncluding(
-        {
-            "event": "call",
-            "defined_class": "example_class.ExampleClass",
-            "method_id": "fully_accessible (get)",
-        }
-    )
-
-    assert events[2].to_dict() == DictIncluding(
-        {
-            "event": "call",
-            "defined_class": "example_class.ExampleClass",
-            "method_id": "fully_accessible (set)",
-        }
-    )
-
-    assert events[4].to_dict() == DictIncluding(
-        {
-            "event": "call",
-            "defined_class": "example_class.ExampleClass",
-            "method_id": "fully_accessible (del)",
-        }
-    )
+    assert len(events) == 6
+    assert events[0].to_dict() == DictIncluding({
+        "event": "call",
+        "defined_class": "properties_class.PropertiesClass",
+        "method_id": "fully_accessible (get)",
+    })
+
+    assert events[2].to_dict() == DictIncluding({
+        "event": "call",
+        "defined_class": "properties_class.PropertiesClass",
+        "method_id": "fully_accessible (set)",
+    })
+
+    assert events[4].to_dict() == DictIncluding({
+        "event": "call",
+        "defined_class": "properties_class.PropertiesClass",
+        "method_id": "fully_accessible (del)",
+    })
 
 
 def test_writable_instrumented(events):
-    from example_class import ExampleClass
+    from properties_class import PropertiesClass
 
-    ec = ExampleClass()
-    assert ExampleClass.write_only.__doc__ == "Write-only"
+    ec = PropertiesClass()
+    assert PropertiesClass.write_only.__doc__ == "Write-only"
 
     with pytest.raises(AttributeError, match=r".*(has no getter|unreadable attribute).*"):
         _ = ec.write_only
 
     ec.write_only = "updated example"
 
     assert len(events) == 2
-    assert events[0].to_dict() == DictIncluding(
-        {
-            "event": "call",
-            "defined_class": "example_class.ExampleClass",
-            "method_id": "set_write_only (set)",
-        }
-    )
+    assert events[0].to_dict() == DictIncluding({
+        "event": "call",
+        "defined_class": "properties_class.PropertiesClass",
+        "method_id": "set_write_only (set)",
+    })
+
+
+def test_operator_attrgetter(events):
+    from properties_class import PropertiesClass
+
+    ec = PropertiesClass()
+
+    assert ec.operator_read_only == "read only"
+
+    with pytest.raises(AttributeError, match=r".*(has no setter|can't set attribute).*"):
+        ec.operator_read_only = "not allowed"
+
+    with pytest.raises(AttributeError, match=r".*(has no deleter|can't delete attribute).*"):
+        del ec.operator_read_only
+
+    assert len(events) == 2
+    assert events[0].to_dict() == DictIncluding({
+        "event": "call",
+        "defined_class": "properties_class.PropertiesClass",
+        "method_id": "operator_read_only (get)",
+    })