Fix ValueError in calculate_work() when licensed_through is empty (PP-3977)#3227
Fix ValueError in calculate_work() when licensed_through is empty (PP-3977)#3227dbernstein wants to merge 1 commit intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3227 +/- ##
=======================================
Coverage 93.30% 93.30%
=======================================
Files 497 497
Lines 46144 46145 +1
Branches 6318 6319 +1
=======================================
+ Hits 43055 43057 +2
+ Misses 2004 2003 -1
Partials 1085 1085 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm not sure why we are seeing this now (likely it is something with the parallel imports in overdrive, but it does appear that the code did not previously assume that there would be a work associated with the license pool when it was created. This change just makes that assumption explicit. |
jonathangreen
left a comment
There was a problem hiding this comment.
I'm a bit concerned about this fix @dbernstein. I think we at least need to look more deeply into it. Before the recent Overdrive changes we never hit this error, so something changed in the Overdrive importer to cause us to hit this.
If it is in fact it is just transient because self.identifier.licensed_through is empty only because the relationship cache is stale then I don't think this behaviour is what we want. In that case, any sibling LicensePools already in the database keep pointing at the old Work while self is moved to a new one, which breaks the assumption that all LicensePools with a given Identifier must share a work and can leave duplicate works for the same identifier.
d7cab14 to
5cd942c
Compare
Fix ValueError in calculate_work() when licensed_through is transiently empty
Description
Tighten the condition in
LicensePool.calculate_work()that reads a consensusWorkfromIdentifier.licensed_through. Change the bareelse:toelif len(existing_works) == 1:so the destructuring assignment[self.work] = existing_worksis only attempted when the set contains exactly one element.Motivation and Context
LicensePool.calculate_work()builds a set of existing works from all pools sharing the same identifier:It then branches on the size of that set:
The
elsebranch is reached wheneverlen(existing_works) <= 1, including the empty case. Whenlicensed_throughis transiently empty — e.g. because the SQLAlchemy relationship collection has not yet been populated during a flush — the bare destructuring raises:This was observed in production as a Celery task failure on
apply.bibliographic_apply. With the fix, when the set is emptyself.workis left unchanged and the method falls through to the existing "create a new Work" logic below.How Has This Been Tested?
Added
TestWorkConsolidation::test_calculate_work_does_not_raise_when_licensed_through_is_empty, a regression test that temporarily mocksIdentifier.licensed_throughto return[]and asserts thatcalculate_work()returns a valid newWorkwithout raising. The fullTestWorkConsolidationsuite (27 tests) was run and all pass.Checklist