Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Versioning refinements #458

Merged
merged 5 commits into from
Feb 7, 2021
Merged

Conversation

chisholm
Copy link
Contributor

@chisholm chisholm commented Sep 18, 2020

This intended to tighten up versionability detection and plug some holes. It's not perfect, but hopefully an iterative improvement.

The changes include addressing versionability of a type separately from versionability of an object (i.e. an instance of a type). This was kind of blurred before: STIX objects would be checked for presence of versioning properties; dicts would be checked for support from their object type. This presents opportunities for bugs and misleading error messages.

For example, if a custom STIX type were created whose versioning properties were optional, and you didn't provide sufficient properties on an object, you'd get an error that you "cannot create new version of object of this type". That's actually incorrect: you can creation new versions of instances of the type, you just have to provide sufficient properties on the object. Also, if you tried to version a dict of a versionable type, didn't provide an explicit "modified" property for the new version as a kwarg, and it was also missing from the dict, you would get a crash because code just assumed the "modified" property would be in the dict. In the first case, versionability of the object was checked but not the type; in the second case, versionability of the type was checked, but not the object. So both types of error are made.

There is also the question of what to do if someone tries to version a dict of an unregistered STIX type. Without registration, we can't tell what properties the type supports, so we can't determine its versionability. Do we allow versioning?

Behavior included with this PR:

  • If an object has all versioning properties, it is considered versionable. Versionability of the type is not checked in this case. This allows versioning properties to be added as custom properties as necessary, to enable versioning on an otherwise unversionable object.
  • Versionability determination of a STIX type is the same as before. All versioning properties must be supported.
  • If a STIX type has not been registered and one attempts to version a dict for that type, a lax approach is taken, and the type will be considered versionable. (This does not mean a particular instance of the type is versionable.)
  • Except for the first bullet item, versionability determination of an object is only done if the type has been deemed versionable. An object of versionable type is considered versionable if it has the "created" property. If it does not have a "modified" property, it is assumed equal to "created", and the new version will have a "modified" property added with a timestamp greater than the created property.
  • Exception types have been added to be clearer about why versioning fails. You should no longer get an error saying a type is not versionable, when it is. You should also no longer get a generic KeyError crash if a versionable instance of a versionable type does not have a "modified" property, and one was not given as a kwarg to new_version().

Possible future work: what if neither the type nor instance is considered versionable according to the above criteria, but together, the support/presence criteria are met for versionability? E.g. type supports modified/revoked but not created, and instance has a custom "created" property but has neither "modified" nor "revoked". The type would not be considered versionable, so versionability of the object would not even be considered. But presence of the object's custom "created" property together with the type's support for "modified" and "revoked" put all the ingredients in place for versioning.

@codecov-commenter
Copy link

Codecov Report

Merging #458 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   98.24%   98.25%   +0.01%     
==========================================
  Files         142      142              
  Lines       15740    15838      +98     
==========================================
+ Hits        15463    15562      +99     
+ Misses        277      276       -1     
Impacted Files Coverage Δ
stix2/exceptions.py 100.00% <100.00%> (ø)
stix2/test/v20/test_versioning.py 100.00% <100.00%> (ø)
stix2/test/v21/test_versioning.py 100.00% <100.00%> (ø)
stix2/versioning.py 100.00% <100.00%> (+1.02%) ⬆️

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 5a34d52...bc8fdcd. Read the comment docs.

@rpiazza
Copy link
Contributor

rpiazza commented Sep 18, 2020

I wonder if it is always an error if a type has the revoked property, but not the created property and/or modified property.

@chisholm
Copy link
Contributor Author

I checked the spec (e.g. sections 3.2 and 11.2), but couldn't find anything that put an all-or-nothing requirement on the usage of versioning properties for custom STIX types. E.g. if you use "revoked", you must also use "created" and "modified". It does say "The revoked property is only used by STIX Objects that support versioning...". There is similar description for "modified", but not for "created". That implies a policy which was used by spec authors for spec-defined objects. There is not a "must" or "should" directing creators of custom types to add the other two properties.

It would be an odd situation to have a "revoked" property on an object/type you can never revoke, because it's unversionable. I also don't think you're allowed to use properties named the same as common properties for purposes other than what the spec describes(?). So, odd situation but it seems like the spec allows it.

Btw, I edited the PR description (updated the behavior bullets). I hadn't quite got the correct behavioral description... maybe that is an indication of how tricky it is to get the logic right :)

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #458 (624d71e) into master (5e2d888) will increase coverage by 0.01%.
The diff coverage is 92.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   89.31%   89.33%   +0.01%     
==========================================
  Files         147      147              
  Lines       16245    16349     +104     
==========================================
+ Hits        14509    14605      +96     
- Misses       1736     1744       +8     
Impacted Files Coverage Δ
stix2/exceptions.py 58.08% <56.25%> (+0.23%) ⬆️
stix2/workbench.py 99.25% <75.00%> (-0.75%) ⬇️
stix2/versioning.py 82.40% <91.42%> (+0.11%) ⬆️
stix2/test/v20/test_versioning.py 100.00% <100.00%> (ø)
stix2/test/v21/test_versioning.py 100.00% <100.00%> (ø)
stix2/registry.py 25.00% <0.00%> (+2.77%) ⬆️

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 5e2d888...624d71e. Read the comment docs.

and dicts, and add better raised exception types if versioning
couldn't be done.  I changed workbench monkeypatching a bit, to
copy some class attributes over to the workbench wrapper
class-like callables, since some code expected those attributes
to be there (e.g. the versioning code).
@chisholm chisholm force-pushed the versioning_refinements branch from ac99e12 to 624d71e Compare January 29, 2021 04:02
stix2/versioning.py Outdated Show resolved Hide resolved
@clenk clenk merged commit e513081 into oasis-open:master Feb 7, 2021
@chisholm chisholm deleted the versioning_refinements branch February 9, 2021 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants