Dictionary Prop Adjustments

[ryantxu1]

Address #588

• don't allow 'string_list' as one of the values when a list is given for valid_types
• in clean, check that the value of the dictionary items is the correct type as noted in specifics.
• some Tests to check for both the correct use of the DictionaryProperty definitions and instantiations

[rpiazza]

• don't allow 'string_list' as one of the values when a list is given for valid_types - if it just has ['string_list'], it is ok, the problem is when it has ['string_list', 'string'] or ['string_list', 'integer']. if you choose 'string_list', then no value can be an in or str
• self.specifics is always a list, so you can say valid_types == string or any of the others. I think my list of values might have been misleading.
• values that are list cannot contain integers, so that else clause seems wrong
• valid_types=['string', 'integer'] means that the value can be a string or integer, but not a list

[chisholm]

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).

[rpiazza]

@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.

[ryantxu1]

@rpiazza @chisholm

Sorry I misinterpreted how the types are handled! Is this better?

[rpiazza]

@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]

@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.

[rpiazza]

Again, looks good to me - but Andy knows this stuff better than I :-)

Here are some things I noticed:

• on line 483 you say "Dictionary Property does not support this value's type:". But I think the clean could fail for other reasons, ask Andy
• the loop on 474 - If you find that its true for one of the valid types, you can break out of the loop, right?

[codecov-commenter]

Codecov Report

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 ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           dictionary-property     #589      +/-   ##
=======================================================
+ Coverage                87.42%   87.45%   +0.02%     
=======================================================
  Files                      155      155              
  Lines                    18096    18161      +65     
=======================================================
+ Hits                     15820    15882      +62     
- Misses                    2276     2279       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.