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

Fix deterministic ids #402

Merged
merged 10 commits into from
Jun 8, 2020
Merged

Conversation

chisholm
Copy link
Contributor

@chisholm chisholm commented Jun 2, 2020

Fixes #395, #396 .

Revamped deterministic ID generation code:

  • "Primary" object initialization is now done first, and the resulting property values are what are used to create the deterministic ID instead of whatever kwargs had been passed into the constructor.

  • Conversion to a JSON-serializable object (as preparation for canonicalization) should now handle all types of property values.

    The way this conversion should be done isn't clear from the spec. E.g. it says to use the "list of ID Contributing Properties" and create canonical JSON from that, but (a) not all property values are compatible with JSON, and (b) it doesn't describe what from each property to use (name+value or just value?). For (a), property values which are incompatible with JSON are first serialized according to the JSON serialization rules for the type. This produces a string, which is JSON compatible and so can be canonicalized. This notably applies to timestamps, for which the library uses instances of a datetime subclass for convenience, which will get their spec-prescribed textual format. For (b), the decision is that both names and values are important, so we create a canonical JSON object, rather than taking the word "list" as given in the spec, literally.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2020

Codecov Report

Merging #402 into master will increase coverage by 0.01%.
The diff coverage is 98.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #402      +/-   ##
==========================================
+ Coverage   98.43%   98.44%   +0.01%     
==========================================
  Files         127      128       +1     
  Lines       14652    14694      +42     
==========================================
+ Hits        14423    14466      +43     
+ Misses        229      228       -1     
Impacted Files Coverage Δ
stix2/base.py 98.14% <95.12%> (+0.25%) ⬆️
stix2/test/v21/test_deterministic_ids.py 100.00% <100.00%> (ø)
stix2/canonicalization/Canonicalize.py 60.64% <0.00%> (-0.41%) ⬇️
stix2/patterns.py 99.33% <0.00%> (-0.01%) ⬇️
stix2/v20/__init__.py 100.00% <0.00%> (ø)
stix2/v21/__init__.py 100.00% <0.00%> (ø)
stix2/test/v20/test_pattern_expressions.py 100.00% <0.00%> (ø)
stix2/pattern_visitor.py 90.67% <0.00%> (+0.06%) ⬆️

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 41525f9...5a5484d. Read the comment docs.

@rpiazza
Copy link
Contributor

rpiazza commented Jun 2, 2020

Can you see any issues with using refs as contributing properties? There are a few objects that use refs. I know I have to be careful about the order in which I create the ids when creating the deterministic ids by hand, but maybe it would never be an issue using python-stix2.

@chisholm
Copy link
Contributor Author

chisholm commented Jun 2, 2020

By hand, I guess you'd have to create the referent object IDs first, then you can populate the ref properties with those IDs and then canonicalize. It is similar with the stix2 library: you need some value to use for the ref property, so you have to create the referent object (or its ID) first.

Circular references are impossible if both SCOs require UUIDv5's. You'd have to switch one to a v4 to make that work.

@chisholm chisholm force-pushed the fix_deterministic_ids branch from 4d44d26 to 5a5484d Compare June 5, 2020 18:21
Copy link
Contributor

@clenk clenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @chisholm!

@clenk clenk merged commit 6faf6b9 into oasis-open:master Jun 8, 2020
@emmanvg emmanvg added this to the 2.0.0 milestone Jun 8, 2020
@chisholm chisholm deleted the fix_deterministic_ids branch June 8, 2020 21:39
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.

CustomExtensions for File with TimestampPropertys can't handle datetime objects
5 participants