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

Fixed timestamp fields comparison #198

Merged
merged 12 commits into from
Apr 8, 2024
Merged

Conversation

chrisr3d
Copy link
Contributor

@chrisr3d chrisr3d commented Aug 8, 2022

I just discovered this kind of behavior can happen when 2 timestamp values don't have the same time precision:
image

I did not find in the standard any rule to forbid this difference in time precision, so I guess it should be valid.

The issue comes from the python comparison operators used to compare strings, which will always consider a timestamp value with fewer digits later than the same value with more digits.

I just added an additional test to compare the datetime formatted values before raising a validation exception due to this particular behavior of the strings comparison

In [1]: created = '2022-08-03T07:11:07.029Z'

In [2]: modified = '2022-08-03T07:11:07.029036Z'

In [3]: modified >= created
Out[3]: False

In [4]: from datetime import datetime

In [5]: regex = '%Y-%m-%dT%H:%M:%S.%fZ'

In [6]: datetime.strptime(modified, regex) >= datetime.strptime(created, regex)
Out[6]: True

Avoids raising an exception when the comparison of
timestamp values gives a wrong negative validation
@ejratl
Copy link
Contributor

ejratl commented Nov 22, 2022

I'm struggling with this one a bit. Since I wasn't a maintainer when the pull request was made, I cannot force it to re-run the tests to see if the workflow failures were due an intermittent failure. Locally the tests are also failing, some for unrelated reasons, but it looks like this commit may be impacting the sightings test cases in stix2validator/test/v21/sighting_tests.py
It would also be nice to have a test case added with this change in functionality.

@ejratl
Copy link
Contributor

ejratl commented Feb 13, 2023

@chrisr3d Can you make another commit to this pull request? Perhaps update it to the latest upstream code, please. The tests failed and since the PR was made before I became maintainer, I am not allowed to request that they be re-run. But if you add another commit, then I can re-run the failed tests and get this PR added.

@chrisr3d
Copy link
Contributor Author

I see my changes are causing an issue with the tests on invalid timestamps.
I am investigating on the issue and will push more changes soon.

- Checking if a timestamp is properly formatted
  before trying to load it as a datetime object
- One could still be str if it is not properly formatted
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 94.23077% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 94.49%. Comparing base (1a7e167) to head (cdd96fa).
Report is 4 commits behind head on master.

Files Patch % Lines
stix2validator/v21/musts.py 89.65% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   94.45%   94.49%   +0.03%     
==========================================
  Files          66       66              
  Lines        6173     6208      +35     
==========================================
+ Hits         5831     5866      +35     
  Misses        342      342              

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

@adulau
Copy link
Contributor

adulau commented Apr 3, 2024

@ejratl This makes sense for me as it was more the issue of precision in the timestamp which now should be fixed. Is it ok for you if I merge it?

@ejratl
Copy link
Contributor

ejratl commented Apr 3, 2024

@adulau Yes, please go ahead and do merge it. I was waiting for it to pass the tests and it has done so.

@clenk
Copy link
Contributor

clenk commented Apr 3, 2024

Any chance you could add a unit test or two for this?

- Including missing tests for timestamp values format
  as well as tests for timestamp values comparison
…ffic

- The test for the `end` and `is_active` fields is
  enough here as there is no check on the start
  and end time
@chrisr3d
Copy link
Contributor Author

chrisr3d commented Apr 8, 2024

I added some checks in the v2.0 validator for some timestamp properties that are also tested in v2.1 with the TIMESTAMP_COMPARE mapping.

And I added unit tests for different STIX objects

  • Some were missing any timestamp property test at all
  • For those already having tests on the timestamp format, I added test on the timestamp fields comparison

@adulau
Copy link
Contributor

adulau commented Apr 8, 2024

Thank you!

@adulau adulau merged commit 963af14 into oasis-open:master Apr 8, 2024
9 checks passed
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.

5 participants