TIMX-655 - cascading researchdatabases deletion safeguard - #278
Conversation
Why these changes are being introduced: Post first pass implementation, it was observed that we could theoretically get zero identifiers back from the Springshare API (e.g. bad data), which would be treated as *all* identifieres in the TIMDEX dataset as old and should be deleted. This seems incredibly unlikely and should not proceed. How this addresses that need: * set a DELETION_COUNT_THRESHOLD constant of 80% * if records marked for deletion exceed this percentage of total records from the source itself, raise an exception Side effects of this change: * Cascading deletes for bad Springshare data should be guarded against Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-655
d03478e to
4360257
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new safeguard is directionally correct, but the threshold value is currently reported misleadingly in the raised error message (showing 0.8% instead of 80%), and one new test is redundant rather than directly exercising the threshold logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a safeguard to the researchdatabases Springshare-derived source to prevent cascading deletes when the Springshare API returns unexpectedly few/zero identifiers, which could otherwise be interpreted as “delete everything” from the TIMDEX dataset.
Changes:
- Introduces a deletion threshold (80%) that triggers a
CriticalErrorwhen exceeded. - Adds tests covering under-threshold behavior, over-threshold failure, and empty-dataset behavior.
File summaries
| File | Description |
|---|---|
| transmogrifier/sources/xml/researchdatabases.py | Adds a deletion-ratio threshold check and raises a CriticalError when excessive deletes are detected. |
| tests/sources/xml/test_researchdatabases.py | Adds unit tests intended to validate the new deletion-threshold safeguard behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ehanson8
left a comment
ehanson8
left a comment
There was a problem hiding this comment.
Smart fix and clear tests, approved! One curiosity question
| logger = logging.getLogger(__name__) | ||
|
|
||
| # percentage of total records that if marked for deletion, indicate a problem | ||
| DELETION_PERCENT_THRESHOLD = 0.8 |
There was a problem hiding this comment.
Just curious and non-blocking, how did you arrive at 80% for the threshold?
There was a problem hiding this comment.
100% arbitrary. We probably would have been safe with 100% as the threshold -- we're attemping a full wipe for some reason -- but anything over 80% felt really suspect. I'd imagine normal percentages is likely closer to 0-1%; deletes are rare.
Good question though!
Purpose and background context
Why these changes are being introduced:
Post first pass implementation, it was observed that we could theoretically get zero identifiers back from the Springshare API (e.g. bad data), which would be treated as all identifieres in the TIMDEX dataset as old and should be deleted. This seems incredibly unlikely but should not be allowed to happen.
How this addresses that need:
DELETION_COUNT_THRESHOLDconstant of 80%How can a reviewer manually see the effects of these changes?
See added test.
Includes new or updated dependencies?
NO
Changes expectations for external applications?
YES: Cascading deletes for bad Springshare data should be guarded against
What are the relevant tickets?
Code review