Fix sync_artwork clobbering is_primary via duplicate list entries
CI / lint (push) Canceled after 0s
CI / typecheck (push) Canceled after 0s
CI / unit-tests (push) Canceled after 0s
CI / integration-tests (push) Canceled after 0s
CI / container-build (push) Canceled after 0s
CI / dependency-audit (push) Canceled after 0s
CI / lint (push) Canceled after 0s
CI / typecheck (push) Canceled after 0s
CI / unit-tests (push) Canceled after 0s
CI / integration-tests (push) Canceled after 0s
CI / container-build (push) Canceled after 0s
CI / dependency-audit (push) Canceled after 0s
TMDb's images.posters/backdrops alternates commonly include the *same* image as the top-level poster_path/backdrop_path -- so normalize_movie legitimately produces the same provider_asset_id twice in one artwork_list: once from the top-level field (is_primary=True), once from the general alternates loop (is_primary=False, the default). sync_artwork matched both occurrences to the same existing DB row by provider_asset_id and did a plain overwrite each time, so whichever occurrence it processed last won -- the non-primary duplicate always came second in list order, so it silently un-set is_primary right after the first occurrence had just set it. Caught by re-ingesting The Matrix on a devbox with the artwork download feature live: the movie ingested successfully, but zero download jobs were ever enqueued because get_primary_artwork_ids found nothing marked is_primary -- the flag had been clobbered back to false immediately after being set. Fixed by resetting is_primary on all existing rows up front (so a provider dropping its "primary" pick on an image actually clears, not just adds) and accumulating with OR within the batch instead of a plain overwrite, so any primary occurrence in one sync sticks regardless of processing order. Also dedupes brand-new rows within the same batch the same way, closing the equivalent bug for images that don't have an existing row yet. Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
fef914db36
commit
7c4bfe6cf4
@@ -160,3 +160,116 @@ async def test_sync_artwork_adds_new_rows_on_resync(db_session: AsyncSession):
|
||||
.all()
|
||||
)
|
||||
assert {r.provider_asset_id for r in rows} == {"a.jpg", "c.jpg"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_artwork_duplicate_entry_in_same_batch_does_not_clobber_primary(
|
||||
db_session: AsyncSession,
|
||||
):
|
||||
"""Regression test: TMDb's images.posters/backdrops alternates list
|
||||
commonly includes the *same* image as poster_path/backdrop_path, so one
|
||||
normalize_movie() call can legitimately produce the same
|
||||
provider_asset_id twice in one artwork_list -- once from the top-level
|
||||
field (is_primary=True), once from the general alternates loop
|
||||
(is_primary=False, the default). Whichever occurrence sync_artwork
|
||||
processes second must not un-set what the first one set, regardless of
|
||||
which order they appear in."""
|
||||
|
||||
provider = await make_provider(db_session)
|
||||
entity_id = uuid.uuid4()
|
||||
|
||||
# Primary occurrence first, duplicate non-primary occurrence second --
|
||||
# matches normalize_movie()'s actual ordering (poster_path is appended
|
||||
# before the images.posters loop).
|
||||
await sync_artwork(
|
||||
db_session,
|
||||
entity_type="movie",
|
||||
entity_id=entity_id,
|
||||
provider_id=provider.id,
|
||||
artwork_list=[
|
||||
_artwork("shared.jpg", is_primary=True),
|
||||
_artwork("shared.jpg", is_primary=False),
|
||||
],
|
||||
)
|
||||
await db_session.commit()
|
||||
|
||||
row = (
|
||||
await db_session.execute(select(Artwork).where(Artwork.entity_id == entity_id))
|
||||
).scalar_one()
|
||||
assert row.is_primary is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_artwork_duplicate_new_entry_does_not_create_two_rows(
|
||||
db_session: AsyncSession,
|
||||
):
|
||||
"""Same as above, but for an image that doesn't already have a row --
|
||||
the second occurrence in the batch must update the row the first
|
||||
occurrence just created, not insert a second one for the same
|
||||
provider_asset_id."""
|
||||
|
||||
provider = await make_provider(db_session)
|
||||
entity_id = uuid.uuid4()
|
||||
|
||||
await sync_artwork(
|
||||
db_session,
|
||||
entity_type="movie",
|
||||
entity_id=entity_id,
|
||||
provider_id=provider.id,
|
||||
artwork_list=[
|
||||
_artwork("new.jpg", is_primary=False),
|
||||
_artwork("new.jpg", is_primary=True),
|
||||
],
|
||||
)
|
||||
await db_session.commit()
|
||||
|
||||
rows = (
|
||||
(
|
||||
await db_session.execute(
|
||||
select(Artwork).where(
|
||||
Artwork.entity_id == entity_id, Artwork.provider_asset_id == "new.jpg"
|
||||
)
|
||||
)
|
||||
)
|
||||
.scalars()
|
||||
.all()
|
||||
)
|
||||
assert len(rows) == 1
|
||||
assert rows[0].is_primary is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_artwork_clears_primary_when_provider_no_longer_marks_it(
|
||||
db_session: AsyncSession,
|
||||
):
|
||||
provider = await make_provider(db_session)
|
||||
entity_id = uuid.uuid4()
|
||||
|
||||
await sync_artwork(
|
||||
db_session,
|
||||
entity_type="movie",
|
||||
entity_id=entity_id,
|
||||
provider_id=provider.id,
|
||||
artwork_list=[_artwork("a.jpg", is_primary=True)],
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
# Re-sync where the provider now picks a *different* image as primary.
|
||||
await sync_artwork(
|
||||
db_session,
|
||||
entity_type="movie",
|
||||
entity_id=entity_id,
|
||||
provider_id=provider.id,
|
||||
artwork_list=[_artwork("a.jpg", is_primary=False), _artwork("b.jpg", is_primary=True)],
|
||||
)
|
||||
await db_session.commit()
|
||||
|
||||
rows = {
|
||||
r.provider_asset_id: r.is_primary
|
||||
for r in (
|
||||
await db_session.execute(select(Artwork).where(Artwork.entity_id == entity_id))
|
||||
)
|
||||
.scalars()
|
||||
.all()
|
||||
}
|
||||
assert rows == {"a.jpg": False, "b.jpg": True}
|
||||
|
||||
Reference in New Issue
Block a user