Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 44 additions & 14 deletions _appmap/event.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,7 @@ def display_string(val):
return value


def describe_value(val):
val_type = type(val)
ret = {
"class": fqname(val_type),
"object_id": id(val),
"value": display_string(val),
}

def _is_list_or_dict(val_type):
# We cannot use isinstance here because it uses __class__
# and val could be overloading it and calling it could cause side effects.
#
Expand All @@ -81,7 +74,45 @@ def describe_value(val):
# If the object hasn't been evaluated before it could change the
# observed behavior by doing that prematurely (perhaps even before
# the evaluation can even succeed).
if issubclass(val_type, (list, dict)):

return issubclass(val_type, list), issubclass(val_type, dict)


def _describe_schema(name, val, depth, max_depth):

val_type = type(val)

ret = {}
if name is not None:
ret["name"] = name
ret["class"] = fqname(val_type)

islist, isdict = _is_list_or_dict(val_type)
if not (islist or isdict) or (depth >= max_depth and isdict):
return ret

if islist:
elts = [(None, v) for v in val]
schema_key = "items"
elif isdict:
elts = val.items()
schema_key = "properties"

schema = [_describe_schema(k, v, depth + 1, max_depth) for k, v in elts]
# schema will be [None] if depth is exceeded, don't use it
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to be the only difference between this implementation and Ruby. In short, an array must always provide a type so in Ruby we'll go beyond the max depth to obtain the typing information for an array. However, we've since added a default which will pass OpenAPI validation in the absence of an array type.

This should work just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I ported over all the tests from appmap-ruby. Maybe one showing this behavior is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test to check this: getappmap/appmap-ruby#321 .

if any(schema):
ret[schema_key] = schema

return ret


def describe_value(name, val, max_depth=5):
ret = {
"object_id": id(val),
"value": display_string(val),
}
ret.update(_describe_schema(name, val, 0, max_depth))
if any(_is_list_or_dict(type(val))):
ret["size"] = len(val)

return ret
Expand Down Expand Up @@ -136,8 +167,8 @@ def __repr__(self):
return "<Param name: %s kind: %s>" % (self.name, self.kind)

def to_dict(self, value):
ret = {"name": self.name, "kind": self.kind}
ret.update(describe_value(value))
ret = {"kind": self.kind}
ret.update(describe_value(self.name, value))
return ret


Expand Down Expand Up @@ -321,8 +352,7 @@ def __init__(self, message_parameters):
super().__init__("call")
self.message = []
for name, value in message_parameters.items():
message_object = {"name": name}
message_object.update(describe_value(value))
message_object = describe_value(name, value)
self.message.append(message_object)


Expand Down Expand Up @@ -405,7 +435,7 @@ class FuncReturnEvent(ReturnEvent):

def __init__(self, parent_id, elapsed, return_value):
super().__init__(parent_id, elapsed)
self.return_value = describe_value(return_value)
self.return_value = describe_value(None, return_value)


class HttpResponseEvent(ReturnEvent):
Expand Down
110 changes: 110 additions & 0 deletions _appmap/test/test_describe_value.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import pytest

from _appmap.event import describe_value
from _appmap.test.helpers import DictIncluding


def test_describe_value_does_not_call_class():
"""describe_value should not call __class__
__class__ could be overloaded in the value and
could cause side effects."""

class WithOverloadedClass:
# pylint: disable=missing-class-docstring,too-few-public-methods
@property
def __class__(self):
raise Exception("__class__ called")

describe_value(None, WithOverloadedClass())


class TestDictValue:
@pytest.fixture
def value(self):
return {"id": 1, "contents": "some text"}

def test_one_level_schema(self, value):
actual = describe_value(None, value)
assert actual == DictIncluding(
{
"properties": [
{"name": "id", "class": "builtins.int"},
{"name": "contents", "class": "builtins.str"},
]
}
)


class TestNestedDictValue:
@pytest.fixture
def value(self):
return {"page": {"page_number": 1, "page_size": 20, "total": 2383}}

def test_two_level_schema(self, value):
actual = describe_value(None, value)
assert actual == DictIncluding(
{
"properties": [
{
"name": "page",
"class": "builtins.dict",
"properties": [
{"name": "page_number", "class": "builtins.int"},
{"name": "page_size", "class": "builtins.int"},
{"name": "total", "class": "builtins.int"},
],
}
]
}
)

def test_respects_max_depth(self, value):
expected = {"properties": [{"name": "page", "class": "builtins.dict"}]}
actual = describe_value(None, value, max_depth=1)
assert actual == DictIncluding(expected)


class TestListOfDicts:
@pytest.fixture
def value(self):
return [{"id": 1, "contents": "some text"}, {"id": 2}]

def test_an_array_containing_schema(self, value):
actual = describe_value(None, value)
assert actual["class"] == "builtins.list"
assert actual["items"][0] == DictIncluding(
{
"class": "builtins.dict",
"properties": [
{"name": "id", "class": "builtins.int"},
{"name": "contents", "class": "builtins.str"},
],
}
)
assert actual["items"][1] == DictIncluding(
{
"class": "builtins.dict",
"properties": [{"name": "id", "class": "builtins.int"}],
}
)


class TestNestedArrays:
@pytest.fixture
def value(self):
return [[["one"]]]

def test_arrays_ignore_max_depth(self, value):
actual = describe_value(None, value, max_depth=1)
expected = {
"class": "builtins.list",
"items": [
{
"class": "builtins.list",
"items": [
{"class": "builtins.list", "items": [{"class": "builtins.str"}]}
],
}
],
}
assert actual == DictIncluding(expected)
16 changes: 1 addition & 15 deletions _appmap/test/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

import appmap
from _appmap.env import Env
from _appmap.event import _EventIds, describe_value
from _appmap.event import _EventIds


# pylint: disable=import-error
Expand Down Expand Up @@ -39,20 +39,6 @@ def add_thread_id(q):
assert len(set(all_tids)) == len(all_tids) # Should all be unique


def test_describe_value_does_not_call_class():
"""describe_value should not call __class__
__class__ could be overloaded in the value and
could cause side effects."""

class WithOverloadedClass:
# pylint: disable=missing-class-docstring,too-few-public-methods
@property
def __class__(self):
raise Exception("__class__ called")

describe_value(WithOverloadedClass())


@pytest.mark.appmap_enabled
@pytest.mark.usefixtures("with_data_dir")
class TestEvents:
Expand Down
4 changes: 4 additions & 0 deletions _appmap/test/test_params.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,10 @@ def test_optional_2_keyword(self, params):
"class": "builtins.dict",
"kind": "keyrest",
"size": 2,
"properties": [
{"name": "p1", "class": "builtins.int"},
{"name": "p2", "class": "builtins.int"},
],
},
)

Expand Down
2 changes: 1 addition & 1 deletion _appmap/web_framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class TemplateEvent(Event): # pylint: disable=too-few-public-methods

def __init__(self, path, instance=None):
super().__init__("call")
self.receiver = describe_value(instance)
self.receiver = describe_value(None, instance)
self.path = root_relative_path(path)

def to_dict(self, attrs=None):
Expand Down