fixed start/stop qualifier constant issue

[rpiazza]

No description provided.

[codecov-commenter]

Codecov Report

Merging #400 into master will decrease coverage by 0.00%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
- Coverage   98.44%   98.43%   -0.01%     
==========================================
  Files         127      127              
  Lines       14634    14650      +16     
==========================================
+ Hits        14406    14421      +15     
- Misses        228      229       +1     

Impacted Files	Coverage Δ
stix2/pattern_visitor.py	90.61% <88.88%> (-0.07%)	⬇️
stix2/patterns.py	99.34% <100.00%> (+<0.01%)	⬆️
stix2/test/v20/test_pattern_expressions.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 7e41825...53dfe40. Read the comment docs.

[chisholm]

Well I guess the obvious question is: how do we feel about putting TimestampConstant's in an AST for a STIX 2.0 pattern which doesn't have timestamp constants? I suppose it could act to smooth over 2.0/2.1 differences, but... is that what the AST should do?

[rpiazza]

The rest of the tests in v20/test-pattern-expressions assume that the AST will contain a TimestampConstant. The pattern-visitor is version agnostic - so I think this is appropriate.

As you implied, the 2.0 grammar doesn't accept t-prefixed timestamps.

Did you have something else in mind?

[chisholm]

It was unexpected, for me (I had expected StringLiteral AST nodes). But I also wasn't totally convinced it was wrong. I thought I should bring it up and see if anyone else had opinions. AST's aren't supposed to exactly mirror the grammar. The names of the classes though, are at least partly derived from STIX spec terminology, and there is no timestamp constant in STIX 2.0. That's probably why it feels weird. But as I said, it could even be helpful if people want to write spec-version-neutral pattern processing code.

On the other hand, if stringifying an AST derived from a STIX 2.0 pattern is supposed to recover the same pattern, it wouldn't work: you'd get 2.1 timestamp literals in your 2.0 pattern. For example:

>>> import stix2
>>> stix2.DEFAULT_VERSION
'2.0'
>>> import stix2.pattern_visitor
>>> ast = stix2.pattern_visitor.create_pattern_object("[ipv4-addr:value = '1.2.3.4'] START '2016-06-01T00:00:00Z' STOP '2017-03-12T08:30:00Z'", version="2.0")
>>> str(ast)
"[ipv4-addr:value = '1.2.3.4'] START t'2016-06-01T00:00:00Z' STOP t'2017-03-12T08:30:00Z'"

The stix2 DEFAULT_VERSION is 2.0, yet you get a 2.1 pattern (not even sure the default version should matter, but pointing that out). Following your hint, I had a peek at the tests, and I am surprised that it's testing 2.1 syntax, e.g.:

cti-python-stix2/stix2/test/v20/test_pattern_expressions.py

Lines 463 to 467 in 7e41825

	qual = stix2.StartStopQualifier(
	stix2.TimestampConstant('2016-06-01T00:00:00Z'),
	datetime.datetime(2017, 3, 12, 8, 30, 0),
	)
	assert str(qual) == "START t'2016-06-01T00:00:00Z' STOP t'2017-03-12T08:30:00Z'"

In principle I am okay with a more abstract spec-agnostic AST, but the round-trip changeover from 2.0 to 2.1 syntax is again unexpected and doesn't seem like the right behavior.

@clenk what do you think?

[clenk]

I think the 2.0 tests should be testing 2.0 syntax; they shouldn't have the t prefix. My preference would be to not smooth over the differences so that there aren't surprises like this.

[rpiazza]

ok - I guess this implies dropping TimestampConstant in the 2.0 tests. Hopefully, it won't blow up :-(

[clenk]

@rpiazza the Travis build is failing, can you look into it?

[rpiazza]

@clenk - should be good to go.

[clenk]

Looks fine to me, thanks @rpiazza!