Closed ryantxu1 closed 2 months ago
We already have a way of expressing types via the Property subclasses. I think they should be used. Instead of "string_list", use ListProperty(StringProperty). You get much more generality for free that way, e.g. ListProperty(BooleanProperty), DictionaryProperty(DictionaryProperty(IntegerProperty)), etc. You can leverage the cleaning code which is already there, and gain consistency with how the type is dealt with in other places. E.g. insisting on ints for "integer" is not consistent with how they are handled in other properties.
That full generality may be challenging to map into a relational database, but if there are limitations to that then it's good to know. Someone might create an object extension with a dictionary property which maps to something other than strings or ints, and want to use these facilities to enforce compliance. They can theoretically do pretty much whatever they want, and the library should support expressing that.
Edit: spec section 2.3 says for dictionary values:
dictionary values MUST be valid property base types.
I think this includes all the basic common datatypes at least (integer, boolean, float, etc). So I think restricting values to strings, integers, and lists of strings, may lose spec compliance (e.g. prevent one from describing/processing compliant STIX content).
@chisholm,
When I originally wrote it, I used the Property classes, but the code got a little messy because StringProperty is a class, but ListProperty(StringProperty) is a class instance.
However, because of the normative text you quoted, we should probably go back to using the Property classes.
I have to think about what the implications are for the RelationalDB store.
@rpiazza @chisholm
Sorry I misinterpreted how the types are handled! Is this better?
@chisholm,
You say "You can leverage the cleaning code which is already there, and gain consistency with how the type is dealt with in other places." What Property class are you looking at? ReferenceProperties also takes property names.
@chisholm,
You say "You can leverage the cleaning code which is already there, and gain consistency with how the type is dealt with in other places." What Property class are you looking at? ReferenceProperties also takes property names.
An example was:
E.g. insisting on ints for "integer" is not consistent with how they are handled in other properties.
IntegerProperty will try to convert non-ints to ints (via int()
). The DictionaryProperty code did not do that, which was not consistent with how integers were handled elsewhere. The property cleaners do sometimes try to be more lenient in what values they will accept and convert.
ReferenceProperty takes STIX object types and type categories (SCO, SDO, etc) in its constructor. That's not what we're dealing with here. We're dealing with property types (data types in the spec, section 2), which is different. There is no property of type "malware", for example. You wouldn't embed a whole object like that as a property value. You'd refer to it via a reference property, which is what ReferenceProperty is for. The property data type in this example is "identifier" (maybe the class should have been called IdentifierProperty?). Dealing with data types is what the property classes are for, so I thought we should use them.
Again, looks good to me - but Andy knows this stuff better than I :-)
Here are some things I noticed:
Attention: Patch coverage is 82.75862%
with 15 lines
in your changes are missing coverage. Please review.
Project coverage is 87.45%. Comparing base (
f46d523
) to head (9311c66
).
Files | Patch % | Lines |
---|---|---|
stix2/properties.py | 70.00% | 15 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Address #588