pyronear / pyronear/pyro-annotator

SequenceGroup.updated_at relies on hand-written stamps at every call site

Open
#273 0 comments 0 reactions 0 assignees View on GitHub

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-out
  • sequence_annotations.py:1400 — bulk-annotate group label
  • sequence_groups.py:302PATCH /sequence_groups/{id} (guarded by if 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

  1. Add onupdate=lambda: datetime.now(UTC) to SequenceGroup.updated_at (Python-side default, no migration).
  2. Delete the three manual group.updated_at = datetime.now(UTC) lines.
  3. 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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.