Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an admin endpoint to manually update external IDs for media items, which helps fix incorrect media item mappings. When updating an ID causes a conflict with an existing media item, the system automatically merges the items together.
Changes:
- Introduced a new admin endpoint
/media/update-external-idsfor updating external IDs on media items - Extracted media field merging logic into a reusable
_merge_media_fieldsfunction - Added comprehensive unit tests for the new admin endpoint models
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| backend/src/librarysync/api/routes_admin.py | Adds new admin endpoint for updating media external IDs with conflict detection and merging logic |
| backend/src/librarysync/core/watch_pipeline.py | Extracts media field merging logic into reusable _merge_media_fields helper function |
| backend/src/librarysync/jobs/merge_history.py | Refactors to use the new shared _merge_media_fields function |
| backend/tests/test_routes_admin.py | Adds unit tests for MediaIdUpdate and MediaUpdateRequest models |
| { | ||
| "id": update_item.id, | ||
| "status": "error", | ||
| "message": str(e)[:500], |
There was a problem hiding this comment.
The magic number 500 is used to truncate error messages. Consider extracting this to a named constant (e.g., MAX_ERROR_MESSAGE_LENGTH = 500) for better maintainability and consistency if this pattern is used elsewhere.
| current_value = getattr(target_media, f"{field}_id") | ||
| if current_value == value.lower() if field == "imdb" else value: | ||
| continue | ||
| setattr(target_media, f"{field}_id", value.lower() if field == "imdb" else value) |
There was a problem hiding this comment.
The IMDb ID normalization logic (converting to lowercase) is duplicated in lines 515, 517, 532, and 559. Consider extracting this into a helper function like _normalize_id_value(field: str, value: str) -> str to reduce duplication and ensure consistency.
| watched_id = payload.get("watched_item_id") | ||
| if watched_id in watched_ids: | ||
| payload["watched_item_id"] = watched_id |
There was a problem hiding this comment.
This line appears to be a no-op, as it's setting payload['watched_item_id'] to the same value it already contains (watched_id). If the intention is to update the payload, this needs to be fixed. If this is placeholder code for future logic, it should be removed or clarified with a comment.
| watched_id = payload.get("watched_item_id") | |
| if watched_id in watched_ids: | |
| payload["watched_item_id"] = watched_id |
| { | ||
| "results": results, | ||
| "total_updated": total_updated, | ||
| "total_merged": total_merged, | ||
| "total_unchanged": total_unchanged, | ||
| "total_errors": total_errors, | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 30 days ago
In general, to fix this class of problem you should avoid returning exception messages (or stack traces) directly to the client. Instead, log the detailed exception on the server, and send back a generic, non-sensitive error message such as “Internal error during media update” along with a status code or simple error code. The client can still react to the failure, while sensitive implementation details remain server-side.
For this specific function update_media_external_ids in backend/src/librarysync/api/routes_admin.py, we should modify the except Exception as e: block. Instead of using "message": str(e)[:500], we should:
- Log the exception on the server (using an existing logging mechanism if available; since we don’t see one in the snippet, we can safely import Python’s standard
loggingand uselogger.exception). - Return a generic message in the JSON payload, e.g.
"message": "Unexpected error while updating media external IDs". - Optionally include a stable, non-sensitive code like
"error_code": "INTERNAL_ERROR"to help debugging without leaking details.
Concretely:
- Add an import:
import loggingnear the top ofroutes_admin.py. - Create a module-level logger:
logger = logging.getLogger(__name__). - Update the
exceptblock at lines 446–455 to calllogger.exception(...)and change the"message"field to a generic string.
No behavior outside of error content changes; the structure of the JSON and counters (total_errors, etc.) remain the same.
| @@ -1,5 +1,6 @@ | ||
| from datetime import datetime, timedelta, timezone | ||
| from typing import Any | ||
| import logging | ||
|
|
||
| from fastapi import APIRouter, Depends, Query | ||
| from fastapi.responses import JSONResponse | ||
| @@ -29,6 +30,8 @@ | ||
| from librarysync.jobs.metadata_cache import METADATA_CACHE_JOB | ||
| from librarysync.jobs.watchlist_refresh import WATCHLIST_REFRESH_JOB | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| router = APIRouter(prefix="/api/admin", tags=["admin"]) | ||
|
|
||
| IMPORT_EVENT_PROVIDERS = { | ||
| @@ -445,11 +448,15 @@ | ||
|
|
||
| except Exception as e: | ||
| await db.rollback() | ||
| logger.exception( | ||
| "Unexpected error while updating media external IDs for id=%s", | ||
| update_item.id, | ||
| ) | ||
| results.append( | ||
| { | ||
| "id": update_item.id, | ||
| "status": "error", | ||
| "message": str(e)[:500], | ||
| "message": "Unexpected error while updating media external IDs", | ||
| } | ||
| ) | ||
| total_errors += 1 |
|
The error I'm trying to fix only came up once. Deleting the wrong media_item_id might be the cleaner way. for reference docker exec -t librarysync-db psql -U librarysync -d librarysync -c "WITH target_episodes AS (
SELECT id FROM episode_items WHERE show_media_item_id = '649edc2d-de7b-459a-bb11-17fe639ba645'
), target_watched AS (
SELECT id FROM watched_items
WHERE media_item_id = '649edc2d-de7b-459a-bb11-17fe639ba645'
OR episode_item_id IN (SELECT id FROM target_episodes)
), target_syncs AS (
SELECT id FROM watch_syncs WHERE watched_item_id IN (SELECT id FROM target_watched)
), deleted_outbox AS (
DELETE FROM outbox
WHERE payload->>'media_item_id' = '649edc2d-de7b-459a-bb11-17fe639ba645'
OR payload->>'watched_item_id' IN (SELECT id::text FROM target_watched)
OR payload->>'watch_sync_id' IN (SELECT id::text FROM target_syncs)
RETURNING 1
)
DELETE FROM media_items WHERE id = '649edc2d-de7b-459a-bb11-17fe639ba645';" |
No description provided.