Ensure new_version() actually creates a new version with a correct "modified" property value.

[chisholm]

Fixes #329 .

Now, if the current timestamp is not later than the old modified property value, the new modified value is set to the old value plus one millisecond.

[codecov-io]

Codecov Report

Merging #330 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
- Coverage   98.17%   98.12%   -0.06%     
==========================================
  Files         123      124       +1     
  Lines       14160    13873     -287     
==========================================
- Hits        13902    13613     -289     
- Misses        258      260       +2

Impacted Files	Coverage Δ
stix2/utils.py	97.23% <100%> (-0.49%)	⬇️
stix2/datastore/filesystem.py	92.88% <0%> (-0.12%)	⬇️
stix2/v21/observables.py	97.14% <0%> (-0.05%)	⬇️
stix2/properties.py	98.36% <0%> (-0.03%)	⬇️
stix2/patterns.py	98% <0%> (-0.02%)	⬇️
stix2/test/v21/test_datastore_filesystem.py	99.17% <0%> (-0.02%)	⬇️
stix2/test/v20/test_datastore_filesystem.py	99.19% <0%> (-0.02%)	⬇️
stix2/test/v21/test_relationship.py	100% <0%> (ø)	⬆️
stix2/test/v20/test_campaign.py	100% <0%> (ø)	⬆️
... and 65 more

---

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 4b8fda0...b8d15d8. Read the comment docs.

[clenk]

Is there a reason to use microseconds=1000 instead of milliseconds=1?

[chisholm]

timedelta doesn't have a milliseconds field, but I guess it does support it as a kwarg. I hadn't realized that.

One thing I did think of though, is that if new_modified is later but less than a millisecond later, you may still wind up with a duplicate modified field due to loss of precision when it is truncated to milliseconds. What is really needed is to ensure that new_modified and old_modified represent different milliseconds. This is guaranteed if they're >= 1 millisecond different, but not necessarily if they are < 1 millisecond different (but it's still possible). So it probably actually needs further modification.

[chisholm]

I pushed an update to hopefully handle the case when the current timestamp is too close to the old modified timestamp, but they are not equal. It may look funny, but you can have negative timedeltas, and the comparisons work such that the code works, as far as I can tell. You can't really write a unit test for this, the way the function is currently designed, because the test would have to guarantee that a new version is made within a particular short time period. So I tried testing it in other ways (pasting code into a separate script where I could play with it more). We could probably make it more unit-testable if we redesigned it some.

[rpiazza]

@chisholm -
Did you see the resolution of oasis-tcs/cti-stix2#219. It is no longer the case that it must be exactly 3 digits.

[chisholm]

As long as this library truncates to 3 decimal digits, we will need to ensure that timestamps are produced which will not accidentally become equal when extra precision is lost. We may need a separate issue to relax the millisecond precision requirement throughout the codebase. Part of that will probably be a revision to the design of the new_version() function. But I think this fix should probably be merged so we can ensure correct behavior in the meantime.

Edit: oops, I see you created issue #350 already!

[clenk]

Superseded by #374.