improv: Check reference prop types for all customs

[maybe-sybr]

Previously the requirement that properties ending in _ref(s) were instances of an appropriate type capable of containing a STIX identifier (the current interpretation of section 3.1 of the spec) was only enforced for custom observables. This change refactors the property checks for custom objects to enforce this and the STIX 2.1 property name requirement (also from section 3.1) in a common helper, therefore extending the enforcement of both requirements to all custom object types created by downstream projects.

ref: #511

[codecov-commenter]

Codecov Report

Merging #513 (6b89520) into master (eaf578f) will decrease coverage by 0.01%. The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
- Coverage   87.22%   87.20%   -0.02%     
==========================================
  Files         153      153              
  Lines       17925    17915      -10     
==========================================
- Hits        15635    15623      -12     
- Misses       2290     2292       +2     

Impacted Files	Coverage Δ
stix2/registration.py	72.30% <77.77%> (-6.36%)	:arrow_down:
stix2/test/v20/test_custom.py	100.00% <100.00%> (ø)
stix2/test/v21/test_custom.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eaf578f...6b89520. Read the comment docs.

[maybe-sybr]

Lint should be gone now

[maybe-sybr]

I've rebased this on top of master now that #468 is in to resolve the conflict in _register_extension. Everything appears to still be behaving as expected.

[maybe-sybr]

I've de-drafted this and fixed/removed the todos (the threads above can still be responded to though) to make life easier if you're satisfied with merging this prior to releasing 3.0.0.

I'd like to implement one of the options for adding extra valid reference types, and I have an example of the ABCMeta approach on my staging branch at maybe-sybr/cti-python-stix2@a8fe790 , but if I have to keep a staging branch on top of 3.0.0 just for that one change, I could live with that :)

[maybe-sybr]

@rpiazza I've rebased this on top of v3.0.0 and resolved a conflict with the new top level property validation done for custom extensions. The tests pass but I'll make a quick comment on the diff for @chisholm to sanity check to make sure I didn't mess the expected behaviour.

[chisholm]

Overall, I think doing the ref property checking consistently makes sense. It didn't make much sense to do it only for observables. And I definitely like getting rid of the mro lookups! I always thought those were ugly. It's.... approximately correct I think, but not exactly. E.g. if you registered a custom 2.0 SDO but used ObjectReferenceProperty for its ref properties, I think it would pass the checks. But it shouldn't, since that property class is intended only for 2.0 SCO ref properties.

[maybe-sybr]

Overall, I think doing the ref property checking consistently makes sense. It didn't make much sense to do it only for observables. And I definitely like getting rid of the mro lookups! I always thought those were ugly.

Great, hopefully with the context around my use case for ReferenceProperty variants as well, my suggestions in https://github.com/oasis-open/cti-python-stix2/pull/513#pullrequestreview-697757608 will make some more sense. LMK what you think.

It's.... approximately correct I think, but not exactly. E.g. if you registered a custom 2.0 SDO but used ObjectReferenceProperty for its ref properties, I think it would pass the checks. But it shouldn't, since that property class is intended only for 2.0 SCO ref properties.

I've just had a quick skim through the 2.0 spec (specifically part 3 and 4) and I think I understand how this has evolved now. I suppose the most correct thing would be to have a a nested mapping like VALID_REF_TYPES[stix_version][stix_object_type], but that does sound pretty clunky for a single case. Alternatively, we could allow this logic to stand since I assume the 2.0 code will be relatively static in future, and the only risk is for downstream authors doing the "wrong thing". Happy to make any changes at your suggestion.

[maybe-sybr]

Just marking this as a draft while it needs autosquashing.

[chisholm]

It's.... approximately correct I think, but not exactly. E.g. if you registered a custom 2.0 SDO but used ObjectReferenceProperty for its ref properties, I think it would pass the checks. But it shouldn't, since that property class is intended only for 2.0 SCO ref properties.

I've just had a quick skim through the 2.0 spec (specifically part 3 and 4) and I think I understand how this has evolved now. I suppose the most correct thing would be to have a a nested mapping like VALID_REF_TYPES[stix_version][stix_object_type], but that does sound pretty clunky for a single case. Alternatively, we could allow this logic to stand since I assume the 2.0 code will be relatively static in future, and the only risk is for downstream authors doing the "wrong thing". Happy to make any changes at your suggestion.

Nah, I think treatment would only need to depend on the "class" of object, e.g. SDO vs SCO. So it would only need to be (notionally) VALID_REF_TYPES[stix_version][stix_type_class]. And maybe as simple as VALID_REF_TYPES[2.0 SCOs] and VALID_REF_TYPES[everything else].

I don't know how important it is now to be fanatically correct w.r.t. STIX 2.0, but this is a reference implementation... :)

[chisholm]

If we're going to do this, let's just do it right. It should be easy to adapt your code to:

• Check for ObjectReferenceProperty for 2.0 SCOs
• Check for ReferenceProperty for all other objects

Then ensure there are unit tests to:

• ensure a 2.0 SCO with ReferenceProperty errors
• ensure a 2.0 non-SCO with ObjectReferenceProperty errors
• ensure a 2.1 object errors with ObjectReferenceProperty

[maybe-sybr]

Check out the new diff, @chisholm . I've decided against having a more complex mapping since the only special case we have to deal with is 2.0 observables and it's much more obvious to codify that with a single conditional in the reference property validation helper. I imagine that future iterations of the 2.X spec will aim to maintain coherence for all properties that appear to be references, so building infrastructure to support future deviations is just an exercise in complexity for its own sake. That should be fairly easy to pivot back to in future if the need arises (or in this PR if that's your preference, just LMK).

There are tests for custom objects and observables to ensure that reference vs object reference properties are accepted and rejected appropriately. I don't do any testing for acceptance of subclasses, LMK if you'd like to see that. Since supporting that was the point of #511, I don't mind putting it in, but since the use case is now more dubiously useful, I could also see it being valid to omit them.

[maybe-sybr]

Thanks @chisholm :)