FIX Surface AttackResultEntry.timestamp on hydrated AttackResult#1653
FIX Surface AttackResultEntry.timestamp on hydrated AttackResult#1653thirteeneight wants to merge 2 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
behnam-o
left a comment
There was a problem hiding this comment.
looks great to me, just some minor comments! thanks for finding this gap and addressing it!
There was a problem hiding this comment.
I think if the entry already has a populated timestamp, we'd want to set it one memory model too, i.e:
self.timestamp = entry.timestamp or datetime.now(tz=timezone.utc)
There was a problem hiding this comment.
Done, exact line you suggested. Round-trip's lossless now.
| # AttackResultEntries.timestamp when loaded from memory; None when the | ||
| # AttackResult has never been persisted. Downstream consumers (e.g. the | ||
| # backend mapper) use this to populate user-facing creation times. | ||
| timestamp: Optional[datetime] = None |
There was a problem hiding this comment.
to be consistent with non-nullability of this in memory also, can we set it as
timestamp: datetime = field(default_factory=lambda: datetime.now(timezone.utc))
There was a problem hiding this comment.
In, ty. One ripple: get_attack_result() was passing _ensure_utc(self.timestamp) straight to the constructor, mypy started complaining once the field stoped being Optional. Mirrored your or datetime.now(tz=timezone.utc) on the hydrator side too. Tests updated for the new default and the entry-preserving path.
| elif ar.timestamp is not None: | ||
| created_at = ar.timestamp | ||
| else: | ||
| created_at = datetime.now(timezone.utc) |
There was a problem hiding this comment.
just an observation: I'm trying to find out where the metadata fields "create_at" is even set ... I can't seem to find any reference to it being set ...
I think we can fix that in a later change, @romanlutz if you know where this is populated, can you correct me please?
There was a problem hiding this comment.
Left this alone since you flagged for a later change. Happy to fold it in here if you'd rather not split it.
AttackResultEntry stores a non-nullable timestamp column, but get_attack_result() dropped it when rebuilding the AttackResult. The backend mapper then fell back to datetime.now() in attack_result_to_summary, so the UI showed today's date on every row no matter when it was actually persisted. Adds an optional timestamp field to AttackResult, passes it through _ensure_utc() in get_attack_result(), and has attack_result_to_summary prefer ar.timestamp over datetime.now() when metadata["created_at"] is absent. The metadata override path is unchanged, and callers that do not set the new field get None (same behavior as before). Fixes microsoft#1651
18d0bc5 to
25c17d2
Compare
|
Hey Behnam, thanks for the quick review! Both suggestions are in, dropped a reply on each comment with what changed. Let me know if it lines up with what you had in mind. |
- AttackResult.timestamp: datetime with default_factory, no longer Optional. - AttackResultEntry: preserve entry.timestamp on construction; defensive fallback on hydration to keep the non-optional field satisfied. - Tests updated for the new default and the preservation path.
25c17d2 to
6e4078b
Compare
Description
Fixes #1651.
AttackResultEntrypersists a non-nullabletimestampcolumn, butget_attack_result()dropped it when rebuilding the domainAttackResult. The backend mapper'sattack_result_to_summarythen fell back todatetime.now(timezone.utc), so the UI showed today's date on every row no matter when it was actually persisted.Three small, additive changes:
timestamp: Optional[datetime] = Nonefield to theAttackResultdataclass.timestamp=_ensure_utc(self.timestamp)inAttackResultEntry.get_attack_result().attack_result_to_summary, preferar.timestampoverdatetime.now()whenmetadata["created_at"]is absent. The metadata override path is preserved unchanged.Backwards compatible:
AttackResultEntries.timestamphas been non-nullable since the entry was introduced, so every existing row has a value to hydrate from. Callers constructing anAttackResultwithout the new field getNone, which the mapper treats the same as missing metadata.Filed as draft pending maintainer feedback on #1651.
Tests and Documentation
New unit tests:
tests/unit/models/test_attack_result.py::TestAttackResultTimestamp— default isNone, aware datetime is preserved, round-trip throughAttackResultEntry, naive SQLite timestamps are normalized to UTC on hydration.tests/unit/backend/test_mappers.py::TestAttackResultToSummary— three new cases coveringar.timestamppreferred when metadata is absent, metadata still wins when both are present, fallback todatetime.now()when both are absent.Verification:
pytest tests/unit/— 1339 passed, 0 regressions.mypy --stricton the three modified source files — clean.ruff formatandruff check— clean.