pyronear / pyronear/pyro-annotator
SequenceGroup.updated_at relies on hand-written stamps at every call site
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 0
- Forks
- 1
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 2
Description
Follow-up to #216 / #272.
Context
#216 was sequences_annotations.updated_at never being stamped. #272 fixed it — and the audit alongside it — by moving the stamp onto the column itself:
updated_at: Optional[datetime] = Field(
default=None,
sa_column=Column(DateTime(timezone=True), onupdate=lambda: datetime.now(UTC)),
)
SequenceGroup was left out of that PR to keep the diff to the reported issue. It still stamps by hand.
Current state
Three sites mutate a group, and all three stamp correctly today — this is not a live bug for these paths:
sequence_annotations.py:913— group-label write during validated-group fan-outsequence_annotations.py:1400— bulk-annotate group labelsequence_groups.py:302—PATCH /sequence_groups/{id}(guarded byif changes:)
Why it's worth fixing anyway
Correctness depends on every future author remembering a line that nothing enforces. That is precisely how #216 happened: the annotation stamp was never wired up, and because NULL is a silent value rather than an error, it went unnoticed until it cost a debugging detour during the #212 verification session. The group model is one forgotten line away from the same failure, and the same silence.
One actual gap
Membership changes do not stamp the group at all. DELETE /sequence_groups/{group_id}/members/{sequence_id} (sequence_groups.py:336) and the re-include endpoint (:361) mutate only the Sequence row — seq.sequence_group_id / seq.is_group_excluded — so the group's own updated_at is untouched when its membership changes.
Worth deciding deliberately rather than by accident: should "group last modified" cover membership, or only the group's own columns? Note onupdate alone would not close this, since no UPDATE is emitted against sequence_groups on those paths — it needs an explicit touch either way.
Proposed fix
- Add
onupdate=lambda: datetime.now(UTC)toSequenceGroup.updated_at(Python-side default, no migration). - Delete the three manual
group.updated_at = datetime.now(UTC)lines. - Add a test that a group PATCH stamps
updated_at— and, if we decide membership counts, one for the member endpoints too.
Also worth a decision: User
crud_user.py:74,107 hand-stamps User.updated_at the same way. Converging it on onupdate would be consistent, but note that an explicit assignment always wins over onupdate, so the manual lines must be removed for the hook to take effect — leaving both in place would be misleading rather than redundant.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with SequenceGroup.updated_at and the mutation sites in sequence_annotations.py:913 and :1400 and sequence_groups.py:302, :336, and :361. Decide whether membership changes count toward the group timestamp, then verify the PATCH behavior and any chosen member-endpoint behavior with tests. Also determine whether the separate User hand-stamping in crud_user.py:74,107 belongs in this change.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100