-
Notifications
You must be signed in to change notification settings - Fork 196
Description
In a recent PR #263 the to_serializable
function misbehaved and serialized a set of strings into the string "{'pytm/threatlib/threats.json'}"
.
The issue is caused by the default to_serializable
, which is used because there seems to be no rule to serialize the member variable threatsFile
of the class TM
.
to_serializable
is a @singledispatch function.
Lines 1999 to 2002 in 0c8f23c
@singledispatch | |
def to_serializable(val): | |
"""Used by default.""" | |
return str(val) |
But the potential of @singledispatch
is not fully used here since the it is only used in the default variant (seen above) and the two overload variants serialize(obj, nested=True)
and serialize(obj, nested=False)
Lines 2005 to 2016 in 0c8f23c
@to_serializable.register(TM) | |
def ts_tm(obj): | |
return serialize(obj, nested=True) | |
@to_serializable.register(Controls) | |
@to_serializable.register(Data) | |
@to_serializable.register(Threat) | |
@to_serializable.register(Element) | |
@to_serializable.register(Finding) | |
def ts_element(obj): | |
return serialize(obj, nested=False) |
The reason why the string "{'pytm/threatlib/threats.json'}"
is created is that the default to_serializable
is used.
This is just the same as
json.dumps(str(set(["./a.json"])))
The issue is created by
Line 2007 in 0c8f23c
return serialize(obj, nested=True) |
Because of this check for not nested
in serialize()
.
Line 2048 in 0c8f23c
not nested |
Lines 2019 to 2054 in 0c8f23c
def serialize(obj, nested=False): | |
"""Used if *obj* is an instance of TM, Element, Threat or Finding.""" | |
klass = obj.__class__ | |
result = {} | |
if isinstance(obj, (Actor, Asset)): | |
result["__class__"] = klass.__name__ | |
for i in dir(obj): | |
if ( | |
i.startswith("__") | |
or callable(getattr(klass, i, {})) | |
or ( | |
isinstance(obj, TM) | |
and i in ("_sf", "_duplicate_ignored_attrs", "_threats") | |
) | |
or (isinstance(obj, Element) and i in ("_is_drawn", "uuid")) | |
or (isinstance(obj, Finding) and i == "element") | |
): | |
continue | |
value = getattr(obj, i) | |
if isinstance(obj, TM) and i == "_elements": | |
value = [e for e in value if isinstance(e, (Actor, Asset))] | |
if value is not None: | |
if isinstance(value, (Element, Data)): | |
value = value.name | |
elif isinstance(obj, Threat) and i == "target": | |
value = [v.__name__ for v in value] | |
elif i in ("levels", "sourceFiles", "assumptions"): | |
value = list(value) | |
elif ( | |
not nested | |
and not isinstance(value, str) | |
and isinstance(value, Iterable) | |
): | |
value = [v.id if isinstance(v, Finding) else v.name for v in value] | |
result[i.lstrip("_")] = value | |
return result |
The serialize
function is overloaded with special handling of member variables and requires a rewrite.
All the checks seem to be for specific classes which are all handle in the same function.
Also why is there the check for nested
?
This is basically equivalent to checking if the class is TM
.
In summary we have two types of overloading in the function to_serializable
, one via @singledispatch
and one via the use of the use of isinstance
and the nested
parameter in serialize
.
I propose to move to just use @singledispatch
and move the isinstance
functionality of serialize
into to_serializable
. Maybe remove serialize
in the process or at least make it less complex.
The serialize
function is also used in sqlDump
.
Line 1272 in 0c8f23c
for k, v in serialize(e).items(): |
Is is unclear if it can be replaced with
to_serializable
.