Fix: VOD provider handling - deduplication, NULL names, and malformed data#3
Fix: VOD provider handling - deduplication, NULL names, and malformed data#3cmc0619 wants to merge 26 commits into
Conversation
- Fix xc_series_stream to filter by stream_id instead of episode_id, resolving MultipleObjectsReturned error when episodes have multiple sources - Add priority-based selection when multiple accounts have the same stream_id - Fix refresh_series_episodes to only delete M3UEpisodeRelation, not Episode objects, preventing duplicate key violations during sync - Add ignore_conflicts to bulk episode creation for race condition handling - Re-fetch episode IDs after bulk_create to ensure relations reference correct objects
…N7EkKFW1gSCf4oKVfaVoRV Fix: Resolve VOD series episode streaming with multiple sources
…icts Use object identity (id()) to track which episode objects need PK resolution instead of checking pk attribute, since ignore_conflicts=True never sets PKs.
…f4oKVfaVoRV Fix: Properly resolve episode PKs after bulk_create with ignore_confl…
- Fetch all episodes for series instead of building complex Q filters - Add logging when episode key lookup fails - Filter out relations with unresolved episodes to prevent bulk_create error - Log warning when relations are skipped due to unresolved episodes
Dispatcharr#556 Dispatcharr#569) When providers return multiple stream_ids for the same episode (different quality versions), the code was trying to create duplicate Episode objects, causing unique constraint violations and playback errors. Changes: - Added batch_episodes dict to track episodes processed in current batch - Modified episode lookup to check batch before creating new Episode - Deduplicate updates to prevent same Episode being updated multiple times - Enhanced logging to show stream-to-episode deduplication stats Example: Provider returns 5 streams for S09E02 (1080p, 720p, 480p, etc.) Before: Tried to create 5 Episode objects -> duplicate key error After: Creates 1 Episode, 5 M3UEpisodeRelation objects -> correct Resolves: - Issue Dispatcharr#556: duplicate key constraint violation on episode refresh - Issue Dispatcharr#569: MultipleObjectsReturned error during series playback Files modified: - apps/vod/tasks.py: Core deduplication logic in batch_process_episodes() Files added: - EPISODE_DEDUP_FIX.md: Comprehensive documentation - test_episode_dedup.py: Test script for verification
Added ffprobe-based verification tool to analyze if multiple stream_ids for the same episode are actually different quality versions or just duplicates from the provider. Usage: # Analyze a Dispatcharr episode python verify_stream_differences.py --episode <episode_uuid> # Manual URL comparison python verify_stream_differences.py < urls.txt The tool will: - Use ffprobe to extract resolution, bitrate, codec, fps - Compare all streams for an episode - Report if they're identical (duplicates) or different (quality variants) This helps determine if the deduplication fix is handling legitimate quality options or if the provider is sending duplicate data.
Created simplified version of stream verification that can run outside the Django environment. This tool can: 1. Query Dispatcharr API to get stream URLs for an episode 2. Use ffprobe to analyze each stream's resolution, bitrate, codec 3. Compare streams to determine if they're duplicates or quality variants Usage: # Compare episode streams via API python compare_streams.py --episode <uuid> [base_url] # Compare URLs directly python compare_streams.py <url1> <url2> <url3>... The tool will report whether streams are: - IDENTICAL (duplicates - provider bug) - DIFFERENT (legitimate quality variants) This helps determine proper vod2strm behavior and whether to add duplicate filtering to the VOD refresh task.
Added explicit cleanup of tracking dictionaries and lists after episode batch processing completes. This helps garbage collection in low-memory environments like test containers. Memory savings: - Clear batch_episodes dict (maps episode keys to objects) - Clear episodes_to_update_set (tracks updated episode IDs) - Delete batch operation lists explicitly Note: The test crash at 172 MB occurred during movie category refresh, before episode processing runs, so unrelated to deduplication changes. However, this cleanup improves memory efficiency regardless.
…1XhVh1SQBWk6RBt5HXp612c
Some providers return episode data as a flat list instead of the expected
dict organized by season numbers. This was causing:
Error: 'list' object has no attribute 'items'
The code now handles both formats:
- Dict format: {"1": [...episodes...], "2": [...episodes...]}
- List format: [...episodes...] (extracts season from episode data)
When list format detected, attempts to extract season_number from each
episode's own data. If not present, defaults to season 0.
This resolves the 337 "series without episodes" issue where episode
refresh was failing silently due to unexpected provider response format.
Rewrote compare_streams.py to automatically find and analyze episodes with duplicate streams without requiring manual UUID input. Changes: - Auto-discovery: Queries database for episodes with multiple streams - Provider iteration: Cycles through all active M3U accounts - Smart selection: Picks first 10 episodes with duplicate streams per provider - Django integration: Properly sets up Django ORM for container execution - Comprehensive analysis: Uses ffprobe to compare resolution, bitrate, codec, size - Clear reporting: Identifies if streams are identical duplicates or quality variants - Summary statistics: Reports overall duplicate vs different stream counts Usage: python compare_streams.py This tool helps verify that the episode deduplication fix in apps/vod/tasks.py is working correctly by revealing whether providers are sending true duplicates or legitimate quality variants.
Fixes database constraint violations when providers send NULL or empty
names for movies, series, or episodes.
Changes:
- Movie names: Use 'MovieNameNull' if name is NULL/empty
- Series names: Use 'SeriesNameNull' if name is NULL/empty
- Episode titles: Use 'EpisodeNameNull' if title is NULL/empty
Root cause:
- Previous code: movie_data.get('name', 'Unknown') only uses default if key is missing
- If key exists but value is NULL or '', it uses the NULL/empty value
- New code: movie_data.get('name') or 'MovieNameNull' handles both cases
This prevents "null value in column 'name' violates not-null constraint" errors.
Fixes "bulk_update() prohibited to prevent data loss due to unsaved related object 'movie'" error during VOD refresh. Root cause: - When updating existing M3UMovieRelation/M3USeriesRelation objects, the code sets relation.movie/series to a newly created object - These new objects don't have database IDs yet (created with bulk_create) - bulk_update fails because it can't update a relation pointing to an unsaved object Solution: - After bulk_create of movies/series, re-fetch them with IDs - Update BOTH relations_to_create AND relations_to_update to point to the database objects with IDs - Previous code only fixed relations_to_create, forgot relations_to_update Applied fix to both: - process_movie_batch() at line 658-661 - process_series_batch() at line 995-998
Added FIXES_DETAILED.md with in-depth explanations of all fixes applied to resolve issues Dispatcharr#556 and Dispatcharr#569. This document provides: - Detailed problem descriptions with actual error messages - Root cause analysis explaining WHY each issue occurred - Step-by-step scenarios showing how bugs manifested - Complete fix explanations with code examples - Testing procedures for each fix - Impact analysis Documented fixes: 1. Episode deduplication (batch_episodes tracking) 2. Malformed episode data handling (list vs dict) 3. NULL/empty name handling (or operator) 4. Bulk update unsaved relations (ID mapping) 5. Memory cleanup improvements 6. Stream comparison tool evolution This serves as both technical documentation for maintainers and historical record of the debugging process.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds batch-level episode deduplication and robust provider-data handling across VOD import, proxy, and tooling: prevents duplicate Episode rows when providers return multiple stream_ids by using an in-batch cache, deferring PK resolution after bulk_create, remapping relations, and adding verification scripts, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as VOD Provider
participant Task as Batch Processor (apps/vod/tasks.py)
participant Cache as In‑Memory Batch Cache
participant DB as Database
participant Relation as M3UEpisodeRelation
Note over Task,Cache: Per-batch deduplication and relation queuing
Provider->>Task: Send stream entries (multiple stream_ids for same episode)
activate Task
loop per stream entry
Task->>Cache: Check key (series_id, season, episode)
alt key in cache
Cache-->>Task: Return Episode mapping (existing or queued placeholder)
else not in cache
Task->>DB: Query Episode by (series, season, episode)
alt exists
DB-->>Task: Return existing Episode
else not found
Task->>Task: Queue Episode for bulk_create (placeholder)
end
Task->>Cache: Store Episode mapping
end
Task->>Relation: Queue M3UEpisodeRelation linking stream_id → Episode placeholder
end
Note right of Task: Finalize batch
Task->>DB: Bulk create Episodes (ignore_conflicts=True)
Task->>DB: Re-fetch created Episodes to resolve PKs
Task->>Relation: Update queued relations with resolved Episode FKs
Task->>DB: Bulk create/update M3UEpisodeRelation records
DB-->>Task: Confirm writes
deactivate Task
Note over DB,Relation: Result: single Episode per key + N relations (one per stream)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
apps/vod/tasks.py (3)
377-378: Consistent NULL-name fallbacks for movies/series/episodesUsing
movie_data.get('name') or 'MovieNameNull',SeriesNameNull, andEpisodeNameNullcorrectly prevents NOT NULL violations for empty/None names and aligns with the PR’s intent to allow partial imports instead of crashing. If you want to reuse these across modules, consider centralizing the sentinel values as constants, but the current approach is functionally sound.Also applies to: 695-697, 1317-1322
658-662: Good fix for bulk_update with newly created movie/series referencesUpdating both
relations_to_createandrelations_to_updateto remaprelation.movie/relation.seriesvia thecreated_movies/created_seriesmaps beforebulk_create/bulk_updateeliminates the “bulk_update() prohibited due to unsaved related object” error while keeping the batch flow intact. The id-based mapping is already used for create-relations, so extending it to update-relations is consistent and correct.Also applies to: 995-999
1279-1512: Episode dedup, PK resolution after ignore_conflicts, and cleanup look correctThe episode batch-processing flow now:
- Deduplicates episodes per
(series_id, season_number, episode_number)inbatch_episodes, reusing a singleEpisodeinstance for multiple stream_ids.- Tracks updated episodes via
episodes_to_update_setto avoid queuing duplicates forbulk_update.- Uses
Episode.objects.bulk_create(..., ignore_conflicts=True)plus a fullEpisode.objects.filter(series=series)re-fetch keyed by(series_id, season_number, episode_number)to resolve PKs for episodes that may already exist from other accounts.- Rewrites any
relations_to_createthat still reference unsavedEpisodeobjects to the real DB instances, and then filters out any relations whose episode PK could not be resolved beforebulk_create.- Logs dedup statistics and explicitly clears large in-memory structures at the end to help GC.
This is consistent with the Episode model’s
unique_togetherconstraint and the goal of “one Episode, many M3UEpisodeRelation” per logical episode, while being robust to race conditions across accounts. Aside from the season-numberint()guard noted separately, the algorithm is sound.PR_DESCRIPTION.md (1)
1-152: Docs clearly capture behavior and testing; minor markdown nit onlyThe description does a good job tying together episode dedup, NULL-name handling, malformed episode formats, bulk_update fixes, and the new comparison tooling with concrete examples and test steps. As a tiny polish, you could ensure all fenced code blocks specify a language (markdownlint MD040), but that’s purely cosmetic.
verify_stream_differences.py (3)
73-87: Avoidevalonr_frame_rate; parse explicitly for safety and robustnessUsing
eval(video_stream.get('r_frame_rate', '0/1'))is unnecessary here and brittle if ffprobe ever returns unexpected strings. It’s safer to parse the"<num>/<den>"form yourself:- 'fps': eval(video_stream.get('r_frame_rate', '0/1')), + 'fps': ( + lambda v: (lambda n, d: float(n) / float(d) if float(d) else 0.0)( + *v.split('/', 1) + ) + )(video_stream.get('r_frame_rate', '0/1')),(or use
fractions.Fractionfor clarity).You might also consider:
- Using
logger.exception(...)in the genericexcept Exceptionblock so stack traces are logged automatically.- Narrowing the exception if you want stricter failure modes, though for a diagnostic CLI script the broad catch is acceptable.
90-170: compare_streams logic is sound; consider a couple of small robustness tweaksThe
compare_streamsfunction correctly:
- Probes each URL and prints per-stream metadata.
- Discards failed probes before comparison.
- Treats streams as “identical” if resolution matches and bitrate difference is within 100 kbps, otherwise prints a table of characteristics.
Two optional tweaks:
- Add
strict=Truetozip(stream_ids, stream_urls)to catch length mismatches early.- If you care about ordering side effects, prefer
next(iter(valid_results.values()))overlist(valid_results.values())[0].Behavior is otherwise solid for the intended analysis workflow.
172-223: analyze_dispatcharr_episode integrates cleanly with Django modelsThe Django integration that loads
Episodeand activeM3UEpisodeRelationrows by UUID, then feeds provider-built URLs intocompare_streams, is straightforward and matches howM3UEpisodeRelation.get_stream_url()is defined. Error handling messages and traceback printing are appropriate for a command-line diagnostic tool.EPISODE_DEDUP_FIX.md (1)
8-10: Optional: Add language identifiers to fenced code blocks for better rendering.Several fenced code blocks in this documentation lack language identifiers (e.g., lines 8-10, 115-117, and others). Adding language specifiers improves syntax highlighting and readability in most markdown renderers.
For example:
-``` +```text ERROR: duplicate key value violates unique constraint "vod_episode_series_id_season_number__73053ba7_uniq" DETAIL: Key (series_id, season_number, episode_number)=(11, 9, 2) already exists.Similar changes apply to other code blocks throughout the document. </blockquote></details> <details> <summary>compare_streams.py (4)</summary><blockquote> `75-86`: **Refine exception handling for better error visibility.** The function catches broad `Exception` types which can hide unexpected errors. Consider: 1. Moving the success return to an `else` block (line 79) 2. Being more specific about caught exceptions ```diff if audio_stream: metadata['audio_codec'] = audio_stream.get('codec_name', 'unknown') metadata['audio_channels'] = audio_stream.get('channels', 0) - - return metadata + return metadata + except subprocess.TimeoutExpired: print(f" ✗ ffprobe timeout after {timeout}s") return None - except Exception as e: + except (json.JSONDecodeError, KeyError, ValueError) as e: print(f" ✗ ffprobe error: {e}") return None
125-125: Optional: Remove unnecessary f-string prefixes.Lines 125 and 151 use f-strings without any placeholders, which is unnecessary.
else: - print(f" No episodes with multiple streams found\n") + print(" No episodes with multiple streams found\n")Similar change applies to line 151.
183-183: Optional: Use next(iter()) for better performance.Static analysis suggests using
next(iter())instead of single-element list slicing for accessing the first element.- first = list(valid_results.values())[0] + first = next(iter(valid_results.values())) all_identical = TrueThis avoids creating an intermediate list.
255-260: Consider more specific exception handling.The broad
Exceptioncatch on line 258 could hide unexpected errors. For a diagnostic tool processing multiple episodes, consider catching more specific exceptions while letting truly unexpected errors surface.except Episode.DoesNotExist: print(f"ERROR: Episode with ID {episode_id} not found\n") continue - except Exception as e: + except (KeyError, ValueError, TypeError) as e: print(f"ERROR analyzing episode {episode_id}: {e}\n") continueThis catches common data-related errors while allowing unexpected issues to propagate for debugging.
FIXES_DETAILED.md (1)
27-31: Optional: Add language identifiers to fenced code blocks.Multiple code blocks throughout this document lack language specifiers (lines 27, 115, 179, 268, 274, 486), which reduces syntax highlighting quality in markdown renderers.
For example, the error messages could use
textidentifier:-``` +```text ERROR: duplicate key value violates unique constraint "vod_episode_series_id_season_number__73053ba7_uniq" DETAIL: Key (series_id, season_number, episode_number)=(11, 9, 2) already existsAnd the output examples could use `text` or `console`: ```diff -``` +```console INFO apps.vod.tasks Batch processing 5 stream entries for series MasterChef Junior (2013) ...Also applies to: 115-117, 179-183, 268-271, 274-276, 486-532 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5b193249a89fc83e94476c283f0a36b6edd89c73 and 49b0e6d03080b7f1047127bb1cbb42fadf84e21d. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `EPISODE_DEDUP_FIX.md` (1 hunks) * `FIXES_DETAILED.md` (1 hunks) * `PR_DESCRIPTION.md` (1 hunks) * `apps/output/views.py` (1 hunks) * `apps/vod/tasks.py` (10 hunks) * `compare_streams.py` (1 hunks) * `test_episode_dedup.py` (1 hunks) * `verify_stream_differences.py` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (5)</summary> <details> <summary>test_episode_dedup.py (3)</summary><blockquote> <details> <summary>apps/vod/models.py (3)</summary> * `Series` (76-111) * `Episode` (153-187) * `M3UEpisodeRelation` (257-295) </details> <details> <summary>apps/m3u/models.py (1)</summary> * `M3UAccount` (13-156) </details> <details> <summary>apps/vod/tasks.py (1)</summary> * `batch_process_episodes` (1248-1511) </details> </blockquote></details> <details> <summary>verify_stream_differences.py (1)</summary><blockquote> <details> <summary>apps/vod/models.py (2)</summary> * `Episode` (153-187) * `M3UEpisodeRelation` (257-295) </details> </blockquote></details> <details> <summary>apps/output/views.py (1)</summary><blockquote> <details> <summary>apps/vod/models.py (1)</summary> * `M3UEpisodeRelation` (257-295) </details> </blockquote></details> <details> <summary>compare_streams.py (2)</summary><blockquote> <details> <summary>apps/vod/models.py (2)</summary> * `Episode` (153-187) * `M3UEpisodeRelation` (257-295) </details> <details> <summary>apps/m3u/models.py (1)</summary> * `M3UAccount` (13-156) </details> </blockquote></details> <details> <summary>apps/vod/tasks.py (1)</summary><blockquote> <details> <summary>apps/vod/models.py (2)</summary> * `M3UEpisodeRelation` (257-295) * `Episode` (153-187) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>PR_DESCRIPTION.md</summary> 8-8: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 14-14: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 83-83: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 94-94: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 104-104: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>FIXES_DETAILED.md</summary> 27-27: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 115-115: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 179-179: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 268-268: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 274-274: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 486-486: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Ruff (0.14.7)</summary> <details> <summary>test_episode_dedup.py</summary> 1-1: Shebang is present but file is not executable (EXE001) --- 121-121: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 128-128: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 133-133: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 143-143: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 148-148: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 197-197: Consider moving this statement to an `else` block (TRY300) --- 199-199: Do not catch blind exception: `Exception` (BLE001) </details> <details> <summary>verify_stream_differences.py</summary> 1-1: Shebang is present but file is not executable (EXE001) --- 39-39: `subprocess` call: check for execution of untrusted input (S603) --- 73-73: Use of possibly insecure function; consider using `ast.literal_eval` (S307) --- 80-80: Consider moving this statement to an `else` block (TRY300) --- 83-83: Use `logging.exception` instead of `logging.error` Replace with `exception` (TRY400) --- 85-85: Do not catch blind exception: `Exception` (BLE001) --- 86-86: Use `logging.exception` instead of `logging.error` Replace with `exception` (TRY400) --- 107-107: `zip()` without an explicit `strict=` parameter Add explicit value for parameter `strict=` (B905) --- 143-143: Prefer `next(iter(valid_results.values()))` over single element slice Replace with `next(iter(valid_results.values()))` (RUF015) --- 146-146: Loop control variable `stream_id` not used within loop body (B007) --- 219-219: Do not catch blind exception: `Exception` (BLE001) </details> <details> <summary>compare_streams.py</summary> 37-37: `subprocess` call: check for execution of untrusted input (S603) --- 71-71: Use of possibly insecure function; consider using `ast.literal_eval` (S307) --- 79-79: Consider moving this statement to an `else` block (TRY300) --- 84-84: Do not catch blind exception: `Exception` (BLE001) --- 125-125: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 151-151: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 183-183: Prefer `next(iter(valid_results.values()))` over single element slice Replace with `next(iter(valid_results.values()))` (RUF015) --- 258-258: Do not catch blind exception: `Exception` (BLE001) --- 287-287: Do not catch blind exception: `Exception` (BLE001) </details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>apps/vod/tasks.py (1)</summary><blockquote> `1219-1225`: **Scoped deletion of episode relations (not episodes) is correct** Switching `refresh_series_episodes` to delete only `M3UEpisodeRelation` for the given `series` and `account` (instead of deleting `Episode` rows) appropriately supports cross-account episode sharing while still handling deletions for that provider. This aligns with the Episode’s `unique_together` constraint and the new dedup model. </blockquote></details> <details> <summary>apps/output/views.py (1)</summary><blockquote> `2889-2895`: **Episode lookup by `stream_id` via `M3UEpisodeRelation` is aligned with dedup model** Changing `xc_series_stream` to resolve the episode through `M3UEpisodeRelation` filtered by `stream_id` and `m3u_account__is_active=True`, ordered by `-m3u_account__priority, id`, correctly mirrors how series episodes are now deduplicated (single Episode, many relations). This ensures the XC URL `/series/.../<stream_id>.<ext>` is mapped through the provider relation rather than assuming a global episode_id, and cleanly supports multiple providers with priority semantics. </blockquote></details> <details> <summary>compare_streams.py (1)</summary><blockquote> `23-42`: **Validate stream URLs before passing to subprocess.** The URL parameter originates from `get_stream_url()` and is passed directly to `subprocess.run()`. While `shell=False` provides some protection, malformed or malicious URLs could still cause issues. Consider adding URL validation: ```python def ffprobe_stream(url, timeout=10): """Use ffprobe to get stream metadata.""" # Validate URL format from urllib.parse import urlparse try: parsed = urlparse(url) if not parsed.scheme in ('http', 'https', 'rtmp', 'rtmps'): print(f" ✗ Invalid URL scheme: {parsed.scheme}") return None except Exception as e: print(f" ✗ Invalid URL: {e}") return None try: cmd = [ 'ffprobe', ...This prevents unexpected protocols or malformed URLs from reaching ffprobe.
| # Get initial counts | ||
| initial_episode_count = Episode.objects.filter(series=series, season_number=9, episode_number=2).count() | ||
| initial_relation_count = M3UEpisodeRelation.objects.filter(m3u_account=account).count() | ||
|
|
||
| print(f"\nBefore batch_process_episodes:") | ||
| print(f" Episodes for S09E02: {initial_episode_count}") | ||
| print(f" Relations for account: {initial_relation_count}") | ||
|
|
||
| # Run the batch processing | ||
| print(f"\nRunning batch_process_episodes...") | ||
| batch_process_episodes(account, series, episodes_data, scan_start_time=timezone.now()) | ||
|
|
||
| # Get final counts | ||
| final_episode_count = Episode.objects.filter(series=series, season_number=9, episode_number=2).count() | ||
| final_relation_count = M3UEpisodeRelation.objects.filter( | ||
| m3u_account=account, | ||
| stream_id__in=["78025", "78026", "78027", "78028", "78029"] | ||
| ).count() | ||
|
|
||
| print(f"\nAfter batch_process_episodes:") | ||
| print(f" Episodes for S09E02: {final_episode_count}") | ||
| print(f" Relations created: {final_relation_count}") | ||
|
|
||
| # Verify expectations | ||
| print(f"\n" + "=" * 80) | ||
| print("VERIFICATION:") | ||
| print("=" * 80) | ||
|
|
||
| episodes_created = final_episode_count - initial_episode_count | ||
| relations_created = final_relation_count - initial_relation_count | ||
|
|
||
| success = True | ||
|
|
||
| # Should only create ONE episode | ||
| if episodes_created == 1: | ||
| print("✓ PASS: Created exactly 1 Episode object (correct)") | ||
| else: | ||
| print(f"✗ FAIL: Created {episodes_created} Episode objects (expected 1)") | ||
| success = False | ||
|
|
||
| # Should create 5 relations | ||
| if relations_created == 5: | ||
| print("✓ PASS: Created exactly 5 M3UEpisodeRelation objects (correct)") | ||
| else: | ||
| print(f"✗ FAIL: Created {relations_created} M3UEpisodeRelation objects (expected 5)") | ||
| success = False |
There was a problem hiding this comment.
Fix relation count deltas and make the test rerunnable
The verification logic mixes different baselines:
initial_relation_countcounts all relations for the account.final_relation_countcounts only relations for the five test stream_ids.
So:
relations_created = final_relation_count - initial_relation_countwill be incorrect (often negative) unless the account had zero relations to begin with. Similarly, episodes_created assumes a clean DB; once the test episode exists, rerunning will report 0 created episodes and fail even though dedup is working.
You can make the script consistent and rerunnable by:
- Measuring counts only for the test stream_ids both before and after.
- Asserting on absolute expectations instead of deltas (or cleaning up any pre-existing test data first).
For example:
- initial_relation_count = M3UEpisodeRelation.objects.filter(m3u_account=account).count()
+ initial_relation_count = M3UEpisodeRelation.objects.filter(
+ m3u_account=account,
+ stream_id__in=["78025", "78026", "78027", "78028", "78029"],
+ ).count()
@@
- relations_created = final_relation_count - initial_relation_count
+ relations_created = final_relation_count - initial_relation_countand/or:
# After running
episodes_for_test = Episode.objects.filter(series=series, season_number=9, episode_number=2)
relations_for_test = M3UEpisodeRelation.objects.filter(
m3u_account=account,
stream_id__in=[...],
)
assert episodes_for_test.count() == 1
assert relations_for_test.count() == 5Optionally, you can also:
- Drop the
fprefix on plain strings (Ruff F541). - Narrow the blanket
except Exceptionin the test harness if you want clearer error reporting.
🧰 Tools
🪛 Ruff (0.14.7)
128-128: f-string without any placeholders
Remove extraneous f prefix
(F541)
133-133: f-string without any placeholders
Remove extraneous f prefix
(F541)
143-143: f-string without any placeholders
Remove extraneous f prefix
(F541)
148-148: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In test_episode_dedup.py around lines 124-169, the test mixes baselines by
taking initial_relation_count as all relations for the account but
final_relation_count only for the five test stream_ids and also assumes no
pre-existing Episode, making reruns flaky; change the test to measure counts
only for the test stream_ids both before and after (e.g., initial_relations =
M3UEpisodeRelation.objects.filter(m3u_account=account,
stream_id__in=[...]).count()), or remove any pre-existing test data before
running, then assert absolute expectations (e.g., episodes_for_test.count() == 1
and relations_for_test.count() == 5) instead of computing deltas; optionally
drop unnecessary f-string prefixes on plain strings and avoid broad except
Exception if clearer errors desired.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for episode_data in all_episodes_data: | ||
| try: | ||
| episode_id = str(episode_data.get('id')) | ||
| episode_name = episode_data.get('title', 'Unknown Episode') | ||
| # Handle NULL or empty episode titles from provider | ||
| episode_name = episode_data.get('title') or 'EpisodeNameNull' | ||
| season_number = episode_data['_season_number'] | ||
| episode_number = episode_data.get('episode_num', 0) | ||
| episode_number = int(episode_data.get('episode_num', 0)) | ||
| info = episode_data.get('info', {}) |
There was a problem hiding this comment.
When processing episodes the code now does episode_number = int(episode_data.get('episode_num', 0)). Providers frequently return blank or null episode_num values when their data is malformed (the scenario this change is meant to handle). In that case int(None)/int('') raises and the except block logs an error then skips the episode entirely, so episodes with missing numbers will never be created or related. The previous implementation tolerated absent/empty values, so this is a regression in robustness. Consider coercing with a safe default instead of an unconditional int().
Useful? React with 👍 / 👎.
Addresses code review feedback from PR #3 (CodeRabbit bot review). CRITICAL SECURITY FIX: - compare_streams.py: Replace unsafe eval() with fractions.Fraction - Previous: eval(video_stream.get('r_frame_rate', '0/1')) - Problem: eval() on external data is a security vulnerability - Fix: float(Fraction(video_stream.get('r_frame_rate', '0/1'))) - Impact: Safely parses frame rates like "24000/1001" without code execution risk MAJOR ROBUSTNESS FIXES: - apps/vod/tasks.py: Guard all int() conversions against malformed data Problem 1: Season number conversion crashes on invalid values - Line 1261/1270: int(season_num) fails on "Season 9", None, empty string - Impact: Entire episode batch fails on single malformed season Problem 2: Episode number conversion crashes on None/empty - Line 1338: int(episode_data.get('episode_num', 0)) crashes on None - Impact: Episodes with missing episode_num are skipped Solution: Added safe_int() helper function - Handles None, empty strings, ValueError, TypeError - Falls back to default (0) instead of crashing - Logs warning when non-numeric values encountered - Allows batch to continue processing valid episodes WHAT WAS CHANGED: 1. Created safe_int(value, default=0) helper in batch_process_episodes() 2. Replaced all bare int() conversions with safe_int() 3. Added logging when malformed values are encountered 4. Added fractions.Fraction import to compare_streams.py WHY THIS MATTERS: This code specifically handles malformed provider data. Using unguarded int() conversions contradicts that goal - a single "Season 9" string or None value would crash the entire batch. The safe_int() wrapper ensures we tolerate malformed data as intended. TESTING: Test with provider data containing: - Season keys: "Season 9", "S01", None, "", non-numeric strings - Episode numbers: None, "", non-numeric values - Frame rates: Valid fractions like "24000/1001", "30/1" Verify: - No eval() security warnings from static analyzers - No crashes on malformed season/episode numbers - Warning logs appear for invalid values - Episodes with valid data still import correctly REVIEWER CREDIT: - CodeRabbit (automated code review bot) - Issues identified as Critical (eval) and Major (int conversions)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
compare_streams.py (1)
24-88: ffprobe_stream: eval issue fixed; consider slightly more defensive parsing.Good change replacing
eval()withFractionfor FPS and wrapping the whole probe/parsing in atry/except– this addresses the earlier security concern around arbitrary code execution from ffprobe metadata. Usingsubprocess.runwith an argument list andshell=Falsealso avoids shell-injection risk from the URL.If you want to make this a bit more robust against odd ffprobe outputs (e.g.
bit_rate=Noneor"N/A"), you could normalize those numeric fields before casting so they fail “soft” without tripping the outerExceptionhandler. For example:@@ def ffprobe_stream(url, timeout=10): - metadata = { - 'resolution': f"{video_stream.get('width', 0)}x{video_stream.get('height', 0)}", - 'width': video_stream.get('width', 0), - 'height': video_stream.get('height', 0), - 'codec': video_stream.get('codec_name', 'unknown'), - 'bitrate': int(data.get('format', {}).get('bit_rate', 0)), - 'duration': float(data.get('format', {}).get('duration', 0)), - 'fps': float(Fraction(video_stream.get('r_frame_rate', '0/1'))) if video_stream.get('r_frame_rate') else 0, - 'file_size': int(data.get('format', {}).get('size', 0)), - } + def safe_int(value, default=0): + try: + return int(value) + except (TypeError, ValueError): + return default + + def safe_float(value, default=0.0): + try: + return float(value) + except (TypeError, ValueError): + return default + + fps_str = video_stream.get('r_frame_rate') + try: + fps = float(Fraction(fps_str)) if fps_str else 0.0 + except (ZeroDivisionError, ValueError): + fps = 0.0 + + metadata = { + 'resolution': f"{video_stream.get('width', 0)}x{video_stream.get('height', 0)}", + 'width': video_stream.get('width', 0), + 'height': video_stream.get('height', 0), + 'codec': video_stream.get('codec_name', 'unknown'), + 'bitrate': safe_int(data.get('format', {}).get('bit_rate')), + 'duration': safe_float(data.get('format', {}).get('duration')), + 'fps': fps, + 'file_size': safe_int(data.get('format', {}).get('size')), + }This keeps the behavior but prevents a single odd field from causing the entire probe to be treated as a fatal error.
🧹 Nitpick comments (4)
apps/vod/tasks.py (2)
1299-1305: Minor: Unused variableepisode_keys.The
episode_keyslist is built but never used in the function. Consider removing it to reduce memory usage and improve code clarity. Theepisode_idslist serves its purpose for querying relations.Apply this diff:
- # Extract episode identifiers - episode_keys = [] episode_ids = [] for episode_data in all_episodes_data: - season_num = episode_data['_season_number'] - episode_num = episode_data.get('episode_num', 0) - episode_keys.append((series.id, season_num, episode_num)) episode_ids.append(str(episode_data.get('id')))
1335-1340: Consider improved handling for missing episode numbers.The
safe_int()conversion correctly prevents crashes, but when multiple episodes in a season have missingepisode_num, they'll all map toepisode_number=0due to the unique constraint on(series, season_number, episode_number). This means multiple stream_ids will point to a single "episode 0" Episode object.While this prevents data loss, consider using a synthetic episode number (e.g., based on array index or stream_id hash) when
episode_numis missing, to create separate Episode objects for each distinct stream.Example approach:
season_number = episode_data['_season_number'] # Safely convert episode_num (could be None, empty string, or malformed) episode_number = safe_int(episode_data.get('episode_num'), 0) + # If episode_num is missing/invalid, use a synthetic number to avoid collisions + if episode_number == 0 and not episode_data.get('episode_num'): + # Use the index in all_episodes_data as a synthetic episode number + # Add 1000 to distinguish from real episode 0 + episode_number = 1000 + all_episodes_data.index(episode_data) info = episode_data.get('info', {})compare_streams.py (2)
90-128: find_episodes_with_duplicate_streams: logic is sound; lint-only nits are optional.The query pattern for finding duplicate episode relations per account looks correct and efficient for this CLI use case. The only issues flagged by Ruff here are purely cosmetic:
if duplicate_episodes:will evaluate the queryset; this is fine for a debug tool, but you could replace it with.exists()if you ever care about minimizing DB hits.- Lines that use f-strings without any interpolation (e.g.
print(f" No episodes with multiple streams found\n")) can drop thefprefix to satisfy F541, if you care about a clean lint run.No functional changes are required; these are strictly cleanliness improvements.
282-292: Top-level exception handling: fine for a CLI, but consider narrowing exceptions if you want stricter linting.Catching
KeyboardInterruptexplicitly and then a broadExceptionat the outermost__main__guard is a reasonable pattern for a one-off diagnostic script like this, since it ensures any unexpected error is surfaced with a traceback and a non-zero exit code.If you want to satisfy linters like Ruff’s BLE001, the only change would be to narrow the broad
Exceptionto specific known failure types (e.g.django.core.exceptions.ImproperlyConfigured,subprocess.SubprocessError, etc.), but that’s optional and doesn’t affect runtime behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/vod/tasks.py(10 hunks)compare_streams.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
compare_streams.py (2)
apps/vod/models.py (2)
Episode(153-187)M3UEpisodeRelation(257-295)apps/m3u/models.py (1)
M3UAccount(13-156)
apps/vod/tasks.py (1)
apps/vod/models.py (2)
M3UEpisodeRelation(257-295)Episode(153-187)
🪛 Ruff (0.14.7)
compare_streams.py
38-38: subprocess call: check for execution of untrusted input
(S603)
80-80: Consider moving this statement to an else block
(TRY300)
85-85: Do not catch blind exception: Exception
(BLE001)
126-126: f-string without any placeholders
Remove extraneous f prefix
(F541)
152-152: f-string without any placeholders
Remove extraneous f prefix
(F541)
184-184: Prefer next(iter(valid_results.values())) over single element slice
Replace with next(iter(valid_results.values()))
(RUF015)
259-259: Do not catch blind exception: Exception
(BLE001)
288-288: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
apps/vod/tasks.py (7)
377-378: LGTM! Improved NULL/empty name handling.The change from
.get('name', 'Unknown')to.get('name') or 'MovieNameNull'correctly handles falsy values (None, empty strings) and provides a descriptive sentinel value that indicates the data source.
658-661: LGTM! Correctly maps newly created Movie objects in relations_to_update.This mirrors the logic for
relations_to_createand prevents Django errors whenbulk_update()encounters unsaved Movie references. Aligns with the PR objective to fix bulk update issues.
695-696: LGTM! Consistent NULL/empty name handling for series.The change correctly handles falsy values with a descriptive sentinel value, consistent with the movie name handling.
995-998: LGTM! Correctly maps newly created Series objects in relations_to_update.This mirrors the movie batch processing fix and prevents bulk_update errors with unsaved Series references.
1219-1225: LGTM! Critical architectural improvement for episode deduplication.This correctly deletes only the account-scoped
M3UEpisodeRelationentries, preserving the sharedEpisodeobjects. This enables the deduplication strategy where multiple accounts/providers can have different stream_ids pointing to the same episode.
1253-1291: LGTM! Robust handling of malformed episode data formats.The
safe_int()helper and dual format support (dict/list) address the previous review concerns about unsafe int() conversions. The warning conditions correctly identify non-numeric season values while allowing legitimate season 0.
1527-1529: LGTM! Explicit memory cleanup for containerized environments.Clearing dictionaries and deleting large lists helps garbage collection in memory-constrained environments. Aligns with the PR objective for memory cleanup.
| def analyze_episode_streams(account, episode): | ||
| """Analyze all streams for a given episode from a specific account.""" | ||
| relations = M3UEpisodeRelation.objects.filter( | ||
| m3u_account=account, | ||
| episode=episode | ||
| ) | ||
|
|
||
| print("=" * 80) | ||
| print(f"Episode: {episode.name}") | ||
| print(f"Series: {episode.series.name}") | ||
| print(f"Season {episode.season_number}, Episode {episode.episode_number}") | ||
| print(f"Provider: {account.name}") | ||
| print(f"Found {relations.count()} stream(s)") | ||
| print("=" * 80) | ||
|
|
||
| results = {} | ||
|
|
||
| for rel in relations: | ||
| stream_url = rel.get_stream_url() | ||
| if not stream_url: | ||
| print(f"\nStream {rel.stream_id}:") | ||
| print(f" ✗ Could not generate stream URL") | ||
| continue | ||
|
|
||
| print(f"\nAnalyzing stream {rel.stream_id}...") | ||
| print(f" URL: {stream_url[:80]}...") | ||
|
|
||
| metadata = ffprobe_stream(stream_url, timeout=15) | ||
| if metadata: | ||
| results[rel.stream_id] = metadata | ||
| print(f" ✓ Resolution: {metadata['resolution']}") | ||
| print(f" ✓ Codec: {metadata['codec']}") | ||
| print(f" ✓ Bitrate: {metadata['bitrate'] / 1000:.0f} kbps") | ||
| print(f" ✓ File size: {metadata['file_size'] / (1024*1024):.1f} MB") | ||
| if metadata['fps'] > 0: | ||
| print(f" ✓ FPS: {metadata['fps']:.2f}") | ||
| if 'audio_codec' in metadata: | ||
| print(f" ✓ Audio: {metadata['audio_codec']} ({metadata.get('audio_channels', 0)} channels)") | ||
| else: | ||
| results[rel.stream_id] = None | ||
|
|
||
| # Compare results | ||
| print("\n" + "=" * 80) | ||
| print("COMPARISON RESULTS") | ||
| print("=" * 80) | ||
|
|
||
| valid_results = {k: v for k, v in results.items() if v is not None} | ||
|
|
||
| if len(valid_results) < 2: | ||
| print("Not enough valid streams to compare\n") | ||
| return False | ||
|
|
||
| # Check if all identical | ||
| first = list(valid_results.values())[0] | ||
| all_identical = True | ||
|
|
||
| for metadata in valid_results.values(): | ||
| if metadata['resolution'] != first['resolution']: | ||
| all_identical = False | ||
| break | ||
| # Allow 5% bitrate variance | ||
| if abs(metadata['bitrate'] - first['bitrate']) > first['bitrate'] * 0.05: | ||
| all_identical = False | ||
| break | ||
| if abs(metadata['file_size'] - first['file_size']) > first['file_size'] * 0.05: | ||
| all_identical = False | ||
| break | ||
|
|
||
| if all_identical: | ||
| print("\n⚠️ WARNING: All streams appear IDENTICAL!\n") | ||
| print(f" Resolution: {first['resolution']}") | ||
| print(f" Bitrate: {first['bitrate'] / 1000:.0f} kbps") | ||
| print(f" Codec: {first['codec']}") | ||
| print(f" File size: {first['file_size'] / (1024*1024):.1f} MB") | ||
| print("\n These are likely DUPLICATES, not different quality versions.") | ||
| print(" The provider may be sending the same stream with multiple IDs.\n") | ||
| return True # Identical | ||
| else: | ||
| print("\n✓ Streams have DIFFERENT characteristics:\n") | ||
| print(f"{'Stream ID':<20} {'Resolution':<15} {'Bitrate':<12} {'Size':<10} {'Codec'}") | ||
| print("-" * 80) | ||
| for stream_id, metadata in valid_results.items(): | ||
| print(f"{stream_id:<20} {metadata['resolution']:<15} " | ||
| f"{metadata['bitrate']/1000:>8.0f} kbps " | ||
| f"{metadata['file_size']/(1024*1024):>6.1f} MB {metadata['codec']}") | ||
| print("\n These appear to be legitimate quality variants.\n") | ||
| return False # Different |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "compare_streams.py" -o -name "*main*" | head -20Repository: cmc0619/Dispatcharr
Length of output: 231
🏁 Script executed:
git ls-files | grep -E "(compare_streams|main)" | head -20Repository: cmc0619/Dispatcharr
Length of output: 103
🏁 Script executed:
git ls-files | head -30Repository: cmc0619/Dispatcharr
Length of output: 665
🏁 Script executed:
fd compare_streamsRepository: cmc0619/Dispatcharr
Length of output: 81
🏁 Script executed:
wc -l compare_streams.pyRepository: cmc0619/Dispatcharr
Length of output: 85
🏁 Script executed:
cat -n compare_streams.pyRepository: cmc0619/Dispatcharr
Length of output: 12105
Tri-state return value needed to distinguish "not comparable" from "different" episodes.
The current code returns False for both "not enough valid streams to compare" (line 181) and "streams are different" (line 217), which causes episodes with insufficient data to be incorrectly counted as "DIFFERENT streams" in the summary. To fix:
-
Return
Noneon line 181 instead ofFalse:if len(valid_results) < 2: print("Not enough valid streams to compare\n") return None
Then update
main()to handle the three states:is_identical = analyze_episode_streams(account, episode) total_analyzed += 1 if is_identical is True: identical_count += 1 elif is_identical is False: different_count += 1 # None (not comparable) is not counted
-
Minor style: Replace
first = list(valid_results.values())[0](line 184) withfirst = next(iter(valid_results.values()))to avoid building an intermediate list and satisfy RUF015.
🧰 Tools
🪛 Ruff (0.14.7)
152-152: f-string without any placeholders
Remove extraneous f prefix
(F541)
184-184: Prefer next(iter(valid_results.values())) over single element slice
Replace with next(iter(valid_results.values()))
(RUF015)
🤖 Prompt for AI Agents
In compare_streams.py around lines 131–217, change the tri-state logic so "not
enough valid streams" returns None instead of False and avoid creating an
intermediate list when getting the first metadata: replace the
len(valid_results) < 2 branch to print the same message but return None, change
the first = list(valid_results.values())[0] to first =
next(iter(valid_results.values())), and update the caller (main) to treat the
analyze_episode_streams return as three-state: True => identical_count++, False
=> different_count++, None => do not count (leave total_analyzed increment
unchanged as appropriate).
| def main(): | ||
| print("=" * 80) | ||
| print("Automatic Stream Comparison Tool") | ||
| print("=" * 80) | ||
| print() | ||
|
|
||
| # Find episodes with duplicate streams | ||
| episodes_by_account = find_episodes_with_duplicate_streams() | ||
|
|
||
| if not episodes_by_account: | ||
| print("No episodes with duplicate streams found in any provider.") | ||
| return | ||
|
|
||
| # Analyze each provider's duplicate episodes | ||
| total_analyzed = 0 | ||
| identical_count = 0 | ||
| different_count = 0 | ||
|
|
||
| for account, episode_ids in episodes_by_account.items(): | ||
| print(f"\n{'=' * 80}") | ||
| print(f"ANALYZING PROVIDER: {account.name}") | ||
| print(f"{'=' * 80}\n") | ||
|
|
||
| for episode_id in episode_ids: | ||
| try: | ||
| episode = Episode.objects.get(id=episode_id) | ||
| is_identical = analyze_episode_streams(account, episode) | ||
| total_analyzed += 1 | ||
|
|
||
| if is_identical: | ||
| identical_count += 1 | ||
| else: | ||
| different_count += 1 | ||
|
|
||
| print("\n" + "-" * 80 + "\n") | ||
|
|
||
| except Episode.DoesNotExist: | ||
| print(f"ERROR: Episode with ID {episode_id} not found\n") | ||
| continue | ||
| except Exception as e: | ||
| print(f"ERROR analyzing episode {episode_id}: {e}\n") | ||
| continue | ||
|
|
||
| # Summary | ||
| print("\n" + "=" * 80) | ||
| print("OVERALL SUMMARY") | ||
| print("=" * 80) | ||
| print(f"Total episodes analyzed: {total_analyzed}") | ||
| print(f"Episodes with IDENTICAL streams: {identical_count}") | ||
| print(f"Episodes with DIFFERENT streams: {different_count}") | ||
|
|
||
| if identical_count > 0: | ||
| print("\n⚠️ Warning: Some providers are sending duplicate streams with different IDs.") | ||
| print(" This is the root cause of issue #556 and #569.") | ||
| print(" The deduplication fix in apps/vod/tasks.py handles this correctly.") | ||
|
|
||
| if different_count > 0: | ||
| print("\n✓ Some streams represent legitimate quality variants (different resolution/bitrate).") | ||
|
|
||
| print("=" * 80) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py"Repository: cmc0619/Dispatcharr
Length of output: 10609
🏁 Script executed:
cat -n ./compare_streams.pyRepository: cmc0619/Dispatcharr
Length of output: 12105
Distinguish "not comparable" episodes from "different" episodes in the summary.
The current analyze_episode_streams() function returns False for two distinct cases: when streams are genuinely different (line 217) and when there aren't enough valid streams to compare due to ffprobe failures or insufficient probe data (line 181). In main(), the loop treats all falsy returns as "different streams" (lines 249–252), which over-reports the different_count and conflates legitimate quality variants with episodes that couldn't be analyzed.
To fix this, modify analyze_episode_streams() to return None for the "not enough valid streams" case instead of False, then update main() to handle three explicit outcomes with an optional skipped_count:
@@ def main():
- total_analyzed = 0
- identical_count = 0
- different_count = 0
+ total_analyzed = 0
+ identical_count = 0
+ different_count = 0
+ skipped_count = 0 # Not enough valid streams / probe failures
@@
- is_identical = analyze_episode_streams(account, episode)
- total_analyzed += 1
-
- if is_identical:
- identical_count += 1
- else:
- different_count += 1
+ is_identical = analyze_episode_streams(account, episode)
+ total_analyzed += 1
+
+ if is_identical is True:
+ identical_count += 1
+ elif is_identical is False:
+ different_count += 1
+ else:
+ skipped_count += 1
@@
- print(f"Episodes with IDENTICAL streams: {identical_count}")
- print(f"Episodes with DIFFERENT streams: {different_count}")
+ print(f"Episodes with IDENTICAL streams: {identical_count}")
+ print(f"Episodes with DIFFERENT streams: {different_count}")
+ if skipped_count:
+ print(f"Episodes not comparable (ffprobe failures or <2 valid streams): {skipped_count}")This ensures the summary accurately reflects what was measured and helps distinguish provider behavior issues from analysis failures.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.7)
259-259: Do not catch blind exception: Exception
(BLE001)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'codec': video_stream.get('codec_name', 'unknown'), | ||
| 'bitrate': int(data.get('format', {}).get('bit_rate', 0)), | ||
| 'duration': float(data.get('format', {}).get('duration', 0)), | ||
| 'fps': eval(video_stream.get('r_frame_rate', '0/1')), |
There was a problem hiding this comment.
ffprobe_stream computes FPS with eval(video_stream.get('r_frame_rate', '0/1')). The r_frame_rate value is derived from the probed media metadata, so when this script is pointed at an untrusted stream the eval executes attacker-controlled text, turning a diagnostic helper into a code-execution vector. Parse the ratio (e.g., with Fraction or split) instead of evaluating arbitrary strings.
Useful? React with 👍 / 👎.
Addresses CodeRabbit review feedback - CRITICAL issue in episode processing.
PROBLEM:
Same "bulk_update() prohibited due to unsaved related object" error that we
fixed for movies (line 658-661) and series (line 995-998), but this time in
episode processing.
THE SCENARIO:
1. Provider sends data for a NEW episode (doesn't exist in database)
2. Code creates Episode object and adds to episodes_to_create (no DB ID yet)
3. Episode is stored in batch_episodes dict for deduplication
4. An EXISTING M3UEpisodeRelation needs updating to point to this new episode
5. Code does: relation.episode = episode (assigns unsaved Episode object)
6. Relation added to relations_to_update (line 1440)
7. Later: bulk_create saves episodes and gets their IDs
8. Code updates relations_to_create to use DB versions (line 1479-1490) ✓
9. Code tries bulk_update(relations_to_update, ['episode', ...]) ✗ FAILS!
WHY IT FAILS:
Django's bulk_update checks all foreign keys before executing. If any relation
in relations_to_update points to an unsaved Episode (no primary key), it
rejects the entire operation.
THE FIX:
Added the SAME ID-mapping logic for relations_to_update that we already had
for relations_to_create:
# After bulk_create, re-fetch episodes with their IDs
all_series_episodes = {
(ep.series_id, ep.season_number, ep.episode_number): ep
for ep in Episode.objects.filter(series=series)
}
# Update relations_to_create (was already working)
for relation in relations_to_create:
if id(relation.episode) in unsaved_episode_set:
relation.episode = all_series_episodes[key]
# Update relations_to_update (THIS WAS MISSING)
for relation in relations_to_update:
if id(relation.episode) in unsaved_episode_set:
relation.episode = all_series_episodes[key]
APPLIED AT:
- apps/vod/tasks.py lines 1492-1503 (episode processing)
CONSISTENCY NOTE:
This completes the pattern we established earlier:
- Movies: lines 658-661
- Series: lines 995-998
- Episodes: lines 1492-1503 (this fix)
All three now correctly update BOTH relations_to_create AND relations_to_update
to reference database objects with IDs before bulk_update.
TESTING:
Trigger episode refresh where:
- A new episode is created
- An existing M3UEpisodeRelation needs to point to that new episode
Verify no "unsaved related object" errors occur.
REVIEWER CREDIT:
- CodeRabbit (automated code review bot)
- Issue identified as Critical
- Add scripts/inspect_duplicates.py to inspect DB relations - Add scripts/analyze_provider_raw.py to analyze raw provider feed
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
scripts/inspect_duplicates.py (3)
5-5: Remove unused import.The
defaultdictimport is never used in the script.Apply this diff:
-from collections import defaultdict
36-39: Cache the duplicate count to avoid redundant queries.Calling
dupes.count()twice evaluates the queryset twice, resulting in two separate database queries.Apply this diff to cache the count:
- print(f"Found {dupes.count()} episodes with multiple streams.") + dupe_count = dupes.count() + print(f"Found {dupe_count} episodes with multiple streams.") - if dupes.count() == 0: + if dupe_count == 0: continue
65-66: Log missing episodes instead of silently ignoring them.The empty
exceptblock silently swallowsEpisode.DoesNotExisterrors, which might hide data integrity issues such as orphaned M3UEpisodeRelation records pointing to non-existent episodes.Apply this diff to add informative logging:
except Episode.DoesNotExist: - pass + print(f" WARNING: Episode ID {ep_id} not found (orphaned relation?)")scripts/analyze_provider_raw.py (2)
33-42: Consider adding error handling for client calls.The
client.get_series()call could raise exceptions if the network fails or the provider returns errors. Adding try/except blocks would make the script more robust.Apply this pattern:
# 1. Fetch all series to get IDs print("Fetching series list...") - all_series = client.get_series() - print(f"Found {len(all_series)} series.") + try: + all_series = client.get_series() + print(f"Found {len(all_series)} series.") + except Exception as e: + print(f"Error fetching series: {e}") + return
49-49: Consider adding error handling for series info fetching.Similar to the series list fetch,
client.get_series_info(series_id)could fail. Adding error handling would prevent the script from crashing mid-analysis.Apply this pattern:
- detailed_info = client.get_series_info(series_id) - episodes = detailed_info.get('episodes', {}) + try: + detailed_info = client.get_series_info(series_id) + episodes = detailed_info.get('episodes', {}) + except Exception as e: + print(f"Error fetching info for series {name}: {e}") + continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/analyze_provider_raw.py(1 hunks)scripts/inspect_duplicates.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/inspect_duplicates.py (2)
apps/vod/models.py (2)
Episode(153-187)M3UEpisodeRelation(257-295)apps/m3u/models.py (1)
M3UAccount(13-156)
🪛 Ruff (0.14.7)
scripts/analyze_provider_raw.py
43-43: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
55-55: Loop control variable season not used within loop body
Rename unused season to _season
(B007)
🔇 Additional comments (4)
scripts/analyze_provider_raw.py (4)
1-14: LGTM!The imports and Django environment setup follow the correct pattern for a standalone script. Model imports are properly placed after
django.setup().
82-86: LGTM!The results reporting logic is clear and provides appropriate feedback based on whether duplicates were found.
88-89: LGTM!The main execution block follows standard Python conventions.
26-31: Remove this comment; the concern is incorrect.The
get_user_agent()method does not returnNone. It either returns aUserAgentmodel instance (which has auser_agentCharField attribute) or raises an exception (UserAgent.DoesNotExistorCoreSettings.DoesNotExist). The chained callaccount.get_user_agent().user_agentwill not raiseAttributeErrordue to aNonereturn value.
| # 2. Pick top 5 series to analyze in depth (or scan all if user wants, but that's slow) | ||
| # We'll scan a few to demonstrate | ||
|
|
||
| duplicates_found = 0 | ||
|
|
||
| for i, series_info in enumerate(all_series[:10]): |
There was a problem hiding this comment.
Fix comment inconsistency and rename unused loop variable.
The comment mentions "top 5 series" but the code samples 10. Additionally, the loop variable i is unused and should be prefixed with an underscore per Python conventions.
Apply this diff:
- # 2. Pick top 5 series to analyze in depth (or scan all if user wants, but that's slow)
+ # 2. Pick top 10 series to analyze in depth (or scan all if user wants, but that's slow)
# We'll scan a few to demonstrate
duplicates_found = 0
- for i, series_info in enumerate(all_series[:10]):
+ for _i, series_info in enumerate(all_series[:10]):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 2. Pick top 5 series to analyze in depth (or scan all if user wants, but that's slow) | |
| # We'll scan a few to demonstrate | |
| duplicates_found = 0 | |
| for i, series_info in enumerate(all_series[:10]): | |
| # 2. Pick top 10 series to analyze in depth (or scan all if user wants, but that's slow) | |
| # We'll scan a few to demonstrate | |
| duplicates_found = 0 | |
| for _i, series_info in enumerate(all_series[:10]): |
🧰 Tools
🪛 Ruff (0.14.7)
43-43: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 Prompt for AI Agents
In scripts/analyze_provider_raw.py around lines 38 to 43, the inline comment
incorrectly says "top 5 series" while the code slices the first 10, and the loop
variable i is unused; update the comment to match the code (e.g., "Pick top 10
series to analyze in depth") or change the slice to [:5] if you prefer five, and
rename the unused loop variable i to _i (or _index) to follow Python
conventions.
| if isinstance(episodes, dict): | ||
| for season, eps in episodes.items(): | ||
| all_eps.extend(eps) |
There was a problem hiding this comment.
Rename unused loop variable.
The loop variable season is not used within the loop body and should be prefixed with an underscore per Python conventions.
Apply this diff:
if isinstance(episodes, dict):
- for season, eps in episodes.items():
+ for _season, eps in episodes.items():
all_eps.extend(eps)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(episodes, dict): | |
| for season, eps in episodes.items(): | |
| all_eps.extend(eps) | |
| if isinstance(episodes, dict): | |
| for _season, eps in episodes.items(): | |
| all_eps.extend(eps) |
🧰 Tools
🪛 Ruff (0.14.7)
55-55: Loop control variable season not used within loop body
Rename unused season to _season
(B007)
🤖 Prompt for AI Agents
In scripts/analyze_provider_raw.py around lines 54 to 56, the for-loop uses an
unused loop variable named "season"; rename it to start with an underscore
(e.g., "_season" or simply "_") to follow Python conventions for unused
variables, leaving the loop body unchanged (for _season, eps in
episodes.items(): all_eps.extend(eps)).
| for ep in all_eps: | ||
| s = ep.get('season') or ep.get('season_number') | ||
| e = ep.get('episode_num') | ||
| stream_id = ep.get('id') | ||
| container = ep.get('container_extension') | ||
|
|
||
| key = (s, e) | ||
| ep_map[key].append({'id': stream_id, 'container': container}) |
There was a problem hiding this comment.
Add validation for None season/episode values.
The code uses .get() which can return None for season, episode, or stream_id. If season or episode is None, episodes will be incorrectly grouped under key (None, None), leading to false duplicate detection. The output format S{s}E{e} will also display as SNoneENone.
Apply this diff to skip episodes with missing data:
for ep in all_eps:
s = ep.get('season') or ep.get('season_number')
e = ep.get('episode_num')
stream_id = ep.get('id')
container = ep.get('container_extension')
+ # Skip episodes with missing critical data
+ if s is None or e is None or stream_id is None:
+ continue
+
key = (s, e)
ep_map[key].append({'id': stream_id, 'container': container})🤖 Prompt for AI Agents
In scripts/analyze_provider_raw.py around lines 65 to 72, the loop uses ep.get()
which can return None for season, episode_num, or id causing episodes to be
grouped under (None,None) and formatted as SNoneENone; update the code to
validate and skip any episode where season or episode_num or id is missing
(None) or not a valid integer/string as expected — e.g., read s =
ep.get('season') or ep.get('season_number'), e = ep.get('episode_num'),
stream_id = ep.get('id'), then if any of s, e, or stream_id is falsy/None (or
cannot be coerced to the expected type) continue to next ep (optionally
increment a skip counter or log skipped entries for debugging) before creating
the key and appending to ep_map.
- Update compare_streams.py and verify_stream_differences.py to include -user_agent "VLC/3.0.18-Vetinari" in ffprobe commands. - This resolves issues where providers block requests without a valid UA.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
compare_streams.py (2)
250-253: Update counting logic to handle tri-state return.This block currently treats any falsy return as "different", but if the tri-state fix is applied to
analyze_episode_streams(), this needs to distinguishFalsefromNone.- if is_identical: - identical_count += 1 - else: - different_count += 1 + if is_identical is True: + identical_count += 1 + elif is_identical is False: + different_count += 1 + # None means not comparable - don't count
180-182: Tri-state return needed to distinguish "not comparable" from "different".Returning
Falsehere conflates two distinct cases: (1) streams that are genuinely different and (2) episodes that couldn't be compared due to probe failures. This causes thedifferent_countinmain()to be inflated.Return
Nonefor "not comparable" and updatemain()to handle three states:if len(valid_results) < 2: print("Not enough valid streams to compare\n") - return False + return None # Not comparableThen in
main():if is_identical is True: identical_count += 1 elif is_identical is False: different_count += 1 # None (not comparable) is not countedverify_stream_differences.py (1)
74-74: Critical: Replaceeval()with safeFractionparsing.This is a security vulnerability. The
r_frame_ratevalue from ffprobe metadata could contain attacker-controlled content when probing untrusted streams. The sibling filecompare_streams.py(line 73) already uses the safe approach withFraction.Apply this diff to match the safe implementation in
compare_streams.py:+from fractions import Fraction + ... - 'fps': eval(video_stream.get('r_frame_rate', '0/1')), + 'fps': float(Fraction(video_stream.get('r_frame_rate', '0/1'))) if video_stream.get('r_frame_rate') else 0,
🧹 Nitpick comments (2)
verify_stream_differences.py (1)
108-108: Addstrict=Truetozip()for safety.If
stream_urlsandstream_idshave mismatched lengths, silent truncation could lead to confusing results.- for stream_id, url in zip(stream_ids, stream_urls): + for stream_id, url in zip(stream_ids, stream_urls, strict=True):compare_streams.py (1)
185-185: Usenext(iter(...))instead of list slice.Avoid building an intermediate list for a single element access.
- first = list(valid_results.values())[0] + first = next(iter(valid_results.values()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compare_streams.py(1 hunks)verify_stream_differences.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
verify_stream_differences.py (2)
compare_streams.py (1)
ffprobe_stream(24-88)apps/vod/models.py (2)
Episode(153-187)M3UEpisodeRelation(257-295)
compare_streams.py (3)
apps/vod/models.py (2)
Episode(153-187)M3UEpisodeRelation(257-295)apps/m3u/models.py (1)
M3UAccount(13-156)verify_stream_differences.py (1)
ffprobe_stream(15-88)
🪛 Ruff (0.14.7)
verify_stream_differences.py
1-1: Shebang is present but file is not executable
(EXE001)
40-40: subprocess call: check for execution of untrusted input
(S603)
74-74: Use of possibly insecure function; consider using ast.literal_eval
(S307)
81-81: Consider moving this statement to an else block
(TRY300)
84-84: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
86-86: Do not catch blind exception: Exception
(BLE001)
87-87: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
108-108: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
144-144: Prefer next(iter(valid_results.values())) over single element slice
Replace with next(iter(valid_results.values()))
(RUF015)
147-147: Loop control variable stream_id not used within loop body
(B007)
220-220: Do not catch blind exception: Exception
(BLE001)
compare_streams.py
39-39: subprocess call: check for execution of untrusted input
(S603)
81-81: Consider moving this statement to an else block
(TRY300)
86-86: Do not catch blind exception: Exception
(BLE001)
127-127: f-string without any placeholders
Remove extraneous f prefix
(F541)
153-153: f-string without any placeholders
Remove extraneous f prefix
(F541)
185-185: Prefer next(iter(valid_results.values())) over single element slice
Replace with next(iter(valid_results.values()))
(RUF015)
260-260: Do not catch blind exception: Exception
(BLE001)
289-289: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
verify_stream_differences.py (2)
173-223: LGTM on the Dispatcharr episode analysis function.The lazy Django setup pattern is appropriate for a standalone utility, and the error handling with traceback printing is helpful for debugging.
226-264: LGTM on the CLI entry point.The argument handling covers both modes (episode analysis and manual URL comparison) with clear usage guidance.
compare_streams.py (3)
24-88: Good: Safe frame rate parsing withFraction.The use of
float(Fraction(...))on line 73 safely parses the frame rate string without code execution risk, unlike theeval()approach inverify_stream_differences.py.
91-129: LGTM on duplicate stream discovery logic.The query efficiently uses Django's annotation to find episodes with multiple stream relations.
Minor: Line 127 has an f-string without placeholders—can be a regular string.
283-293: LGTM on the entry point.The top-level exception handling with traceback printing and clean keyboard interrupt handling is appropriate for a CLI tool.
- Combine verbose JSON metadata output with ffprobe stream health check - Show VALID vs BROKEN/NULL status for each duplicate stream - Fix indentation errors
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/analyze_provider_raw.py (3)
95-100: Fix “top 5” comment and unused loop indexThe inline comment and the loop header are slightly out of sync and trigger Ruff’s B007 warning:
- Comment still says “top 5 series” while the code slices
[:10].- The loop index
iis unused.A minimal tidy-up:
- # 2. Pick top 5 series to analyze in depth (or scan all if user wants, but that's slow) + # 2. Pick top 10 series to analyze in depth (or scan all if user wants, but that's slow) @@ - for i, series_info in enumerate(all_series[:10]): + for _i, series_info in enumerate(all_series[:10]):
111-113: Rename unusedseasonloop variable
seasonisn’t used in the loop body, which Ruff flags as B007. Renaming it to start with an underscore keeps intent clear and quiets the linter:- if isinstance(episodes, dict): - for season, eps in episodes.items(): - all_eps.extend(eps) + if isinstance(episodes, dict): + for _season, eps in episodes.items(): + all_eps.extend(eps)
122-129: Guard againstNoneseason/episode/id when buildingep_mapRight now, episodes with missing
season,episode_num, oridwill be grouped under(None, None)and reported as duplicates (and printed asSNoneENone), which can be misleading.Consider skipping entries missing these critical fields before adding them to
ep_map:for ep in all_eps: s = ep.get('season') or ep.get('season_number') e = ep.get('episode_num') stream_id = ep.get('id') container = ep.get('container_extension') - - key = (s, e) - ep_map[key].append({'id': stream_id, 'container': container}) + + # Skip episodes with missing critical data + if s is None or e is None or stream_id is None: + continue + + key = (s, e) + ep_map[key].append({'id': stream_id, 'container': container})This keeps the duplicate detection focused on well-formed episodes and avoids noisy false positives.
🧹 Nitpick comments (1)
scripts/analyze_provider_raw.py (1)
17-71: Tighten JSON and exception handling incheck_stream_health()The logic is sound, but a few small tweaks would improve robustness and satisfy linters:
- Avoid the bare
exceptaroundjson.loads, and catch the specific decode error.- Narrow the final
except Exceptionto only the expected subprocess errors (e.g.,subprocess.TimeoutExpired,OSError) so real coding bugs surface.- Drop the
str()call inside the f-string (or use an explicit conversion flag) to silence RUF010.For example:
- try: - data = json.loads(result.stdout) - except: - return False, "Invalid JSON output" + try: + data = json.loads(result.stdout) + except json.JSONDecodeError as decode_err: + return False, f"Invalid JSON output: {decode_err!s}" @@ - except Exception as e: - return False, f"Exception: {str(e)}" + except (subprocess.TimeoutExpired, OSError) as e: + return False, f"ffprobe failed: {e!s}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/analyze_provider_raw.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
scripts/analyze_provider_raw.py
34-34: subprocess call: check for execution of untrusted input
(S603)
46-46: Do not use bare except
(E722)
70-70: Do not catch blind exception: Exception
(BLE001)
71-71: Use explicit conversion flag
Replace with conversion flag
(RUF010)
100-100: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
112-112: Loop control variable season not used within loop body
Rename unused season to _season
(B007)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
compare_streams.py (2)
182-184: Tri-state return needed: returnNonefor "not comparable" instead ofFalse.This issue was flagged in a previous review but appears unaddressed. Returning
Falsefor "not enough valid streams" conflates it with "streams are different", causing over-reporting in the summary. ReturnNoneinstead to distinguish these cases.if len(valid_results) < 2: print("Not enough valid streams to compare\n") - return False + return None
246-265: HandleNonereturn value fromanalyze_episode_streams.If
analyze_episode_streamsreturnsNone(after fixing line 184), the current logic will count it as "different" due to Python's truthiness evaluation. Add explicit tri-state handling with a skipped counter.+ skipped_count = 0 + for account, episode_ids in episodes_by_account.items(): ... for episode_id in episode_ids: try: episode = Episode.objects.get(id=episode_id) is_identical = analyze_episode_streams(account, episode) total_analyzed += 1 - if is_identical: + if is_identical is True: identical_count += 1 - else: + elif is_identical is False: different_count += 1 + else: + skipped_count += 1Then update the summary (around line 272) to report
skipped_countif non-zero.
🧹 Nitpick comments (12)
scripts/debug_episode_integrity.py (1)
14-48: Duplicate detection logic is solid; consider small UX tweaks for episode displayThe aggregation over
(series, season_number, episode_number)withCount('id')andcount__gt=1is correct and lines up with theEpisodeuniqueness constraint. The script will do a good job of surfacing integrity issues.Two optional refinements you might consider:
- More readable handling of
Noneseason/episode numbersRight now,
S{season} E{ep_num}will render asSNone ENonewhen those fields are null. To align with theEpisode.__str__convention (S00E00) and make the output more readable, you could do:- print(f"Series: {series_name} | S{season} E{ep_num} | Count: {count}") + season_display = season or 0 + ep_display = ep_num or 0 + print(f"Series: {series_name} | S{season_display:02d} E{ep_display:02d} | Count: {count}")
- Optional: avoid relying on QuerySet truthiness
if not duplicates:is valid but a bit implicit. If you prefer more explicit semantics, you could materialize once and then reuse:duplicates = list( Episode.objects.values('series', 'season_number', 'episode_number') .annotate(count=Count('id')) .filter(count__gt=1) .order_by('-count') ) if not duplicates: ...Not required, but makes it clearer that you’re working with a concrete result set.
Overall, the function and
__main__guard look good as‑is.scripts/inspect_episode_216433.py (1)
2-44: Solid one-off debug script; consider making the target episode configurable and slightly more defensiveThe script is straightforward and correct for its intended debugging use: Django setup is fine, the ORM queries are simple, and the output is clear and useful.
Two small, optional ergonomics upgrades you may want:
Make
target_ep_idconfigurable instead of hard‑coded.
This keeps the script reusable for other episodes without editing the file each time. For example:def inspect_episode():ID from the user's log
target_ep_id = 216433
Default ID from the user's log; override via CLI arg:
python inspect_episode_216433.py <episode_id>- if len(sys.argv) > 1:
try:target_ep_id = int(sys.argv[1])except ValueError:print(f"Invalid episode id '{sys.argv[1]}', expected integer.")return- else:
target_ep_id = 216433
- Optional: guard against missing related objects if those are possible.
If there’s any chance ofep.seriesorrel.m3u_accountbeingNonein corrupted data, you could use safe accessors (e.g.,getattr(ep.series, "name", "<no series>"),getattr(rel.m3u_account, "name", "<no account>")) to avoid the script dying while inspecting bad rows.These are not blockers; the current script is fine to merge as a targeted inspection tool.
scripts/check_specific_streams.py (2)
19-21: Use.exists()for QuerySet emptiness check.Using
if not relationsforces a full query evaluation. For clarity and potential efficiency, use.exists()instead.- if not relations: + if not relations.exists(): print("❌ Neither stream found in the database. A refresh has not imported them yet, or they were skipped.") return
23-29: Add defensive check for missing series.If
rel.episode.seriesisNone, accessing.namewill raiseAttributeError. Consider adding a defensive check for robustness in this diagnostic script.for rel in relations: print(f"\n✅ Found Stream ID: {rel.stream_id}") print(f" Episode ID: {rel.episode.id}") print(f" Episode: {rel.episode}") - print(f" Series: {rel.episode.series.name}") + print(f" Series: {rel.episode.series.name if rel.episode.series else 'N/A'}") print(f" Created: {rel.created_at}") print(f" Updated: {rel.updated_at}")scripts/debug_query.py (1)
39-46: Useepisode_idfor consistency with manual grouping.The manual grouping on line 32 uses
episode_id, while the ORM query usesepisode. For consistency and clarity, consider usingepisode_idin both places.orm_dupes = ( M3UEpisodeRelation.objects .filter(m3u_account=account) - .values('episode') + .values('episode_id') .annotate(stream_count=Count('id')) .filter(stream_count__gt=1) )compare_streams.py (2)
187-187: Usenext(iter(...))instead oflist(...)[0].This avoids creating an intermediate list just to get the first element. Per static analysis (RUF015) and previous review feedback.
- first = list(valid_results.values())[0] + first = next(iter(valid_results.values()))
129-129: Remove extraneousfprefix.This f-string has no placeholders. Per Ruff F541.
- print(f" No episodes with multiple streams found\n") + print(" No episodes with multiple streams found\n")apps/proxy/vod_proxy/views.py (5)
202-202: Prefix unused variable with underscore.
current_connectionsis unpacked but never used. Per Ruff RUF059, prefix with underscore to indicate intentional discard.- m3u_profile, current_connections = profile_result + m3u_profile, _current_connections = profile_resultSame applies to line 331 in the HEAD handler.
367-370: Avoid bareexcept.The bare
except:on line 369 catches all exceptions includingKeyboardInterruptandSystemExit. Use a specific exception type.try: total_size = content_range.split('/')[-1] - except: + except (IndexError, ValueError): total_size = response.headers.get('Content-Length', '0')
433-448: LetHttp404propagate instead of catching and returningNone.
get_object_or_404raisesHttp404, but the broadexcept Exceptioncatches it and returnsNone. The caller then checks forNoneand raisesHttp404again. LetHttp404propagate naturally.def _get_content_object(self, content_type, content_id): """Get the content object (Movie or Episode) by UUID""" try: if content_type == 'movie': return get_object_or_404(Movie, uuid=content_id) elif content_type == 'episode': return get_object_or_404(Episode, uuid=content_id) elif content_type == 'series': series = get_object_or_404(Series, uuid=content_id) # Return first episode for series return series.episodes.first() else: return None + except Http404: + raise except Exception as e: logger.error(f"Error getting content object: {e}") return None
373-373: Remove extraneousfprefix.This f-string has no placeholders. Per Ruff F541.
- logger.warning(f"[VOD-HEAD] No Content-Range header in 206 response") + logger.warning("[VOD-HEAD] No Content-Range header in 206 response")
336-339: Fix inconsistent indentation.Line 336 has an extra space before the comment, and line 361 has similar inconsistent indentation. This appears to be a minor formatting issue.
- # Validate URL + # Validate URL if not final_stream_url or not final_stream_url.startswith(('http://', 'https://')):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/proxy/vod_proxy/views.py(2 hunks)compare_streams.py(1 hunks)scripts/check_specific_streams.py(1 hunks)scripts/debug_episode_integrity.py(1 hunks)scripts/debug_query.py(1 hunks)scripts/inspect_episode_216433.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
scripts/debug_query.py (2)
apps/vod/models.py (1)
M3UEpisodeRelation(257-295)apps/m3u/models.py (1)
M3UAccount(13-156)
scripts/debug_episode_integrity.py (1)
apps/vod/models.py (2)
Episode(153-187)Series(76-111)
apps/proxy/vod_proxy/views.py (2)
apps/proxy/vod_proxy/multi_worker_connection_manager.py (3)
MultiWorkerVODConnectionManager(622-1370)get_instance(628-632)infer_content_type_from_url(27-77)apps/proxy/vod_proxy/connection_manager.py (1)
get_instance(284-288)
scripts/check_specific_streams.py (1)
apps/vod/models.py (1)
M3UEpisodeRelation(257-295)
🪛 Ruff (0.14.7)
apps/proxy/vod_proxy/views.py
163-163: Abstract raise to an inner function
(TRY301)
163-163: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Unpacked variable current_connections is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
231-231: Consider moving this statement to an else block
(TRY300)
233-233: Do not catch blind exception: Exception
(BLE001)
331-331: Unpacked variable current_connections is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
369-369: Do not use bare except
(E722)
373-373: f-string without any placeholders
Remove extraneous f prefix
(F541)
390-390: Do not catch blind exception: Exception
(BLE001)
391-391: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
418-418: Consider moving this statement to an else block
(TRY300)
420-420: Do not catch blind exception: Exception
(BLE001)
431-431: Use explicit conversion flag
Replace with conversion flag
(RUF010)
446-446: Do not catch blind exception: Exception
(BLE001)
447-447: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
495-495: Consider moving this statement to an else block
(TRY300)
497-497: Do not catch blind exception: Exception
(BLE001)
498-498: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
compare_streams.py
39-39: subprocess call: check for execution of untrusted input
(S603)
81-81: Consider moving this statement to an else block
(TRY300)
86-86: Do not catch blind exception: Exception
(BLE001)
129-129: f-string without any placeholders
Remove extraneous f prefix
(F541)
155-155: f-string without any placeholders
Remove extraneous f prefix
(F541)
187-187: Prefer next(iter(valid_results.values())) over single element slice
Replace with next(iter(valid_results.values()))
(RUF015)
262-262: Do not catch blind exception: Exception
(BLE001)
291-291: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
scripts/debug_episode_integrity.py (1)
2-13: Django environment setup is correct for this standalone debug scriptYou correctly adjust
sys.path, useos.environ.setdefaultforDJANGO_SETTINGS_MODULE, calldjango.setup()before importing models, and then importEpisode/Series. This is a sound pattern for an ad‑hoc diagnostic script.apps/proxy/vod_proxy/views.py (2)
159-174: Good implementation of ranked candidate selection with fallback.The refactoring to use
_get_content_objectand_get_ranked_relationsfollowed by a stream attempt loop is a solid resilience pattern. Returning 503 ("No streams available") when no relations exist is appropriate.
450-499: Well-structured ranking logic with efficient querying.The
_get_ranked_relationsmethod implements a clear priority system (preferred stream → preferred account → account priority) with proper use ofselect_relatedto avoid N+1 queries. The ordering logic is correct.
| import os | ||
| import sys | ||
| import django | ||
| from django.db.models import Count | ||
|
|
||
| # Setup Django environment | ||
| sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) | ||
| os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'dispatcharr.settings') | ||
| django.setup() | ||
|
|
||
| from apps.vod.models import M3UEpisodeRelation, M3UAccount |
There was a problem hiding this comment.
Move Django import after django.setup().
The from django.db.models import Count import on line 5 occurs before django.setup() is called on line 10. This will cause an ImproperlyConfigured exception when running the script.
import os
import sys
import django
-from django.db.models import Count
# Setup Django environment
sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'dispatcharr.settings')
django.setup()
+from django.db.models import Count
from apps.vod.models import M3UEpisodeRelation, M3UAccount🤖 Prompt for AI Agents
In scripts/debug_query.py around lines 2 to 12, the from django.db.models import
Count occurs before django.setup(), which can raise ImproperlyConfigured; move
that import (and any other Django-dependent imports) to after django.setup() is
called (or import django.db.models inside the function that runs after setup) so
all Django configuration is initialized before importing Django ORM components.
Addresses CodeRabbit review feedback - minor cleanup. The episode_keys list was being built but never used anywhere in the batch_process_episodes function. Only episode_ids is actually needed for querying existing M3UEpisodeRelation objects. Changes: - Removed episode_keys list initialization - Removed season_num and episode_num extraction in loop - Removed episode_keys.append() call - Kept episode_ids which is actually used Impact: - Reduces memory usage (no unused list) - Improves code clarity - No functional change Reviewer credit: - CodeRabbit (automated code review bot)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/vod/tasks.py (4)
377-378: Null/empty Movie/Series names handled correctly; watch dedup behavior for “nameless” itemsUsing
movie_data.get('name') or 'MovieNameNull'/series_data.get('name') or 'SeriesNameNull'is a good fix for NULL/empty provider names and avoids NOT NULL violations.One side effect is that when there’s no TMDB/IMDB ID, your dedup keys fall back to
name_{name}_{year}and will now merge all “nameless” entries for the same year under the*NameNullplaceholder. If you ever need to distinguish multiple distinct-but-nameless items (e.g. by provider/category/stream), you may want to tweak the fallback key logic to incorporate additional fields only whennameis one of these placeholders.Also applies to: 695-696
1253-1262: safe_int + dict/list episode flattening are robust; consider one extra data-shape guardThe
safe_inthelper and the explicitdictvslisthandling forepisodes_datanicely address previous crash-proneint()casts and malformed top-level payloads. Logging warnings when season keys/values can’t be parsed is also helpful for troubleshooting.One remaining edge case: in the
dictbranch you still assume eachseason_episodesvalue is iterable of mapping-like episode dicts. If a provider ever returns a nested dict instead of a list (e.g.{ "1": { "1": {...} } }),for episode_data in season_episodes:would iterate keys and then try to doepisode_data['_season_number'], raising. If you want to harden further against unexpected shapes, you could:
- Guard with
isinstance(season_episodes, list)(or convertdict.values()), and- Log and skip unrecognized season payloads instead of letting them blow up the batch.
Not required if you’re confident in the provider contract, but worth considering given the malformed-data focus of this PR.
Also applies to: 1267-1288, 1290-1291
1320-1321: Batch episode dedup logic looks sound; be aware of legacy rows with NULL season/episode numbersThe combination of:
batch_episodeskeyed by(series_id, season_number, episode_number),episodes_to_update_setto ensure each existing Episode hitsbulk_updateat most once, andsafe_intfor both season and episode numbersachieves the intended behavior: per-batch, per-(series/season/episode) dedup of Episode objects while still allowing multiple
M3UEpisodeRelationrows for differentstream_ids.One behavioral nuance:
safe_int(..., default=0)means any episode with missing or malformedepisode_numwill be treated as episode0for that season, and all such entries in the batch will collapse to a single Episode per(series, season, 0). That’s probably acceptable for badly-formed feeds and solves the unique_together issue, but:
- If your database already contains historical Episodes for this series with
season_number/episode_numberasNULL, this new logic will create new “0/0” Episodes instead of reusing the old “NULL/NULL” ones, leaving legacy rows unused but still attached to the Series.- If that’s undesirable, you might want a one-off data cleanup/migration to normalize old
NULLvalues to0(or vice versa) before or after deploying this change.Functionally the new logic is correct; this is mainly an operational/data-hygiene consideration.
Also applies to: 1324-1327, 1331-1336, 1359-1423
1457-1499: Episode bulk_create + relation remap is correct; consider ignore_conflicts for relation insertsThe episode write path is well thought out:
bulk_create(episodes_to_create, ignore_conflicts=True)plus a full re-fetch intoall_series_episodesandunsaved_episode_setcorrectly handles both new and concurrently-created Episodes.- Remapping both
relations_to_createandrelations_to_updatethroughall_series_episodesensures no relation references an in-memory, unsaved Episode.- Filtering
valid_relations = [r for r in relations_to_create if r.episode and r.episode.pk]prevents inserting relations with unresolved episodes.- The dedup stats and explicit list/set clearing are good for observability and memory behavior in long-running workers.
One small hardening opportunity:
M3UEpisodeRelation.objects.bulk_create(valid_relations)is currently called withoutignore_conflicts=True. In highly concurrent scenarios (e.g. two workers refreshing the same account/series at once), you could still hit an IntegrityError on the(m3u_account, stream_id)unique_together even though you pre-fetchedexisting_relationsearlier. Addingignore_conflicts=True(or explicitly catchingIntegrityErroraround this call) would align the behavior with your movie/series paths and make this code more resilient to rare races.Everything else in this block looks consistent with the PR’s dedup and robustness goals.
Also applies to: 1508-1516, 1524-1533, 1535-1538
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/vod/tasks.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/vod/tasks.py (2)
apps/proxy/vod_proxy/views.py (3)
get(28-247)get(659-682)get(754-1014)apps/vod/models.py (2)
M3UEpisodeRelation(257-295)Episode(153-187)
🔇 Additional comments (3)
apps/vod/tasks.py (3)
658-661: Movie relations now correctly remapped to persisted Movie instancesUpdating
relations_to_updateto map throughcreated_moviesmirrors the fix already applied torelations_to_createand closes the “unsaved related object 'movie'” hole when bulk-updating existing relations that were re-pointed at batch-created Movie instances. This is the right fix and should prevent the earlier bulk_update errors.
995-998: Series relations likewise remapped to persisted Series instancesSame pattern as movies: adjusting
relations_to_updateto go throughcreated_seriesensures all updatedM3USeriesRelationrows reference real DB Series objects beforebulk_update, eliminating unsaved-FK issues. Implementation is consistent and correct.
1219-1225: Episode refresh now deletes only relations per account, not shared Episode rowsSwitching to:
M3UEpisodeRelation.objects.filter( episode__series=series, m3u_account=account ).delete()is a solid change: it lets Episodes remain as shared canonical rows across accounts while accurately reflecting per-account deletions via relations only. Combined with series-level cleanup (and CASCADE from Series), this avoids blowing away episodes used by other accounts.
Exception Handling Improvements: - Distinguish provider/network failures from systemic errors - Provider failures (HTTP errors, timeouts, connection errors) trigger failover to next stream - Systemic errors (Redis down, DB errors, code bugs) fail immediately with full traceback - Prevents burning through all streams when infrastructure is down - Applied to both GET and HEAD methods in VOD stream failover loops Redis Client Refactoring: - Replace hardcoded redis.StrictRedis(host='localhost', port=6379) with RedisClient.get_client() - Ensures compatibility with containerized deployments (redis:6379 vs localhost:6379) - Uses centralized configuration with retry logic and health checks - Applied to views.py HEAD method and connection_manager.py Technical Details: - views.py:233-245 (GET failover): Catch RequestException/Timeout/ConnectionError for retry, raise on systemic errors - views.py:428-439 (HEAD failover): Same exception handling pattern - views.py:393-396: Use RedisClient.get_client() for content length storage - connection_manager.py:99-101: Use RedisClient.get_client() for content length retrieval Addresses code review feedback about proper error handling and centralized Redis client usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/proxy/vod_proxy/connection_manager.py (1)
99-101: Consider simplifying the None-check control flow.The current pattern raises an exception for a None Redis client, which is then caught and triggers the fallback. While functional, this could be simplified by checking for None directly and skipping the Redis operations:
try: - r = RedisClient.get_client() - if not r: - raise Exception("Redis not available") + r = RedisClient.get_client() + if r: - content_length_key = f"vod_content_length:{self.session_id}" - stored_length = r.get(content_length_key) - if stored_length: - self.content_length = stored_length - logger.info(f"[{self.session_id}] *** USING PRE-STORED CONTENT LENGTH: {self.content_length} ***") + content_length_key = f"vod_content_length:{self.session_id}" + stored_length = r.get(content_length_key) + if stored_length: + self.content_length = stored_length + logger.info(f"[{self.session_id}] *** USING PRE-STORED CONTENT LENGTH: {self.content_length} ***") + else: + # Fallback to response headers + self.content_length = response.headers.get('content-length') + logger.info(f"[{self.session_id}] *** USING RESPONSE CONTENT LENGTH: {self.content_length} ***") else: - # Fallback to response headers + logger.debug(f"[{self.session_id}] Redis client not available, using response headers") self.content_length = response.headers.get('content-length') - logger.info(f"[{self.session_id}] *** USING RESPONSE CONTENT LENGTH: {self.content_length} ***") except Exception as e: - logger.error(f"[{self.session_id}] Error checking Redis for content length: {e}") - # Fallback to response headers + logger.error(f"[{self.session_id}] Error accessing Redis for content length: {e}") self.content_length = response.headers.get('content-length')This avoids raising a generic exception for control flow and makes the intent clearer.
apps/proxy/vod_proxy/views.py (3)
176-249: Excellent failover pattern with clear provider vs systemic error handling.The multi-candidate retry loop properly distinguishes network/provider failures (which trigger failover to the next stream) from systemic errors (which abort immediately). The logging at each stage provides good observability.
Minor cleanup:
current_connectionsat line 202 is unpacked but never used. Consider using_to indicate it's intentionally ignored:- m3u_profile, current_connections = profile_result + m3u_profile, _ = profile_result
317-445: HEAD failover logic mirrors GET appropriately, but has minor issues.The HEAD request failover pattern correctly mirrors the GET method. However, there are a few cleanup opportunities:
- Line 339:
current_connectionsis unpacked but unused (same as GET).- Line 377: Bare
exceptclause should specify exception types to avoid catching unexpected errors.- Line 381: The f-string has no placeholders and doesn't need the
fprefix.Apply these diffs:
Line 339 - unused variable:
- m3u_profile, current_connections = profile_result + m3u_profile, _ = profile_resultLines 375-382 - specify exception and fix f-string:
# Content-Range: bytes 0-1/1234567890 try: total_size = content_range.split('/')[-1] - except: + except (ValueError, IndexError): total_size = response.headers.get('Content-Length', '0') logger.info(f"[VOD-HEAD] Got file size from Content-Range: {total_size}") else: - logger.warning(f"[VOD-HEAD] No Content-Range header in 206 response") + logger.warning("[VOD-HEAD] No Content-Range header in 206 response") total_size = response.headers.get('Content-Length', '0')
392-401: Previous review concern addressed, but exception handling needs refinement.The hardcoded Redis connection has been replaced with
RedisClient.get_client()as suggested in the previous review—great! However, the exception handling in this block could be improved:
- Line 396: Raising a generic
Exceptionis discouraged. Either use a custom exception or simply letRedisClient.get_client()returnNoneand handle that case.- Line 400: Catching blind
Exceptioncan mask unexpected errors.- Line 401: Use
logging.exceptioninstead oflogging.errorto include the stack trace.Based on previous review feedback and current code.
Apply this diff:
# Store the total content length in Redis for the persistent connection to use try: from core.utils import RedisClient r = RedisClient.get_client() if not r: - raise Exception("Redis not available") + logger.warning("[VOD-HEAD] Redis not available, skipping content length storage") + # Continue without Redis - not critical for streaming + else: - content_length_key = f"vod_content_length:{session_id}" - r.set(content_length_key, total_size, ex=1800) # Store for 30 minutes - logger.info(f"[VOD-HEAD] Stored total content length {total_size} for session {session_id}") + content_length_key = f"vod_content_length:{session_id}" + r.set(content_length_key, total_size, ex=1800) # Store for 30 minutes + logger.info(f"[VOD-HEAD] Stored total content length {total_size} for session {session_id}") except Exception as e: - logger.error(f"[VOD-HEAD] Failed to store content length in Redis: {e}") + logger.exception(f"[VOD-HEAD] Failed to store content length in Redis: {e}") + # Non-critical, continue without Redis storage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/proxy/vod_proxy/connection_manager.py(1 hunks)apps/proxy/vod_proxy/views.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/proxy/vod_proxy/connection_manager.py (1)
core/utils.py (2)
RedisClient(44-194)get_client(49-138)
apps/proxy/vod_proxy/views.py (2)
apps/proxy/vod_proxy/multi_worker_connection_manager.py (2)
get_instance(628-632)infer_content_type_from_url(27-77)core/utils.py (2)
RedisClient(44-194)get_client(49-138)
🪛 Ruff (0.14.7)
apps/proxy/vod_proxy/connection_manager.py
101-101: Abstract raise to an inner function
(TRY301)
101-101: Create your own exception
(TRY002)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
apps/proxy/vod_proxy/views.py
163-163: Abstract raise to an inner function
(TRY301)
163-163: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Unpacked variable current_connections is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
231-231: Consider moving this statement to an else block
(TRY300)
339-339: Unpacked variable current_connections is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
377-377: Do not use bare except
(E722)
381-381: f-string without any placeholders
Remove extraneous f prefix
(F541)
396-396: Abstract raise to an inner function
(TRY301)
396-396: Create your own exception
(TRY002)
396-396: Avoid specifying long messages outside the exception class
(TRY003)
400-400: Do not catch blind exception: Exception
(BLE001)
401-401: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
428-428: Consider moving this statement to an else block
(TRY300)
449-449: Use explicit conversion flag
Replace with conversion flag
(RUF010)
464-464: Do not catch blind exception: Exception
(BLE001)
465-465: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
513-513: Consider moving this statement to an else block
(TRY300)
515-515: Do not catch blind exception: Exception
(BLE001)
516-516: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (3)
apps/proxy/vod_proxy/views.py (3)
159-174: LGTM: Clean content retrieval and candidate selection.The refactored logic cleanly separates content object retrieval and candidate relation ranking, with appropriate 503 responses when no streams are available.
468-517: LGTM: Solid ranking logic with clear prioritization.The ranking implementation correctly prioritizes:
- Exact stream ID match
- Preferred account match
- Account priority (descending) and ID (ascending)
The use of
select_related('m3u_account')optimizes database queries, and returning an empty list on errors allows callers to handle the no-candidates case gracefully.
651-671: LGTM: Defensive URL transformation with proper error handling.The transformation logic correctly handles M3U profile-based URL rewriting with regex patterns, converting backreference syntax and returning the original URL on any errors to maintain streaming continuity.
| def _get_content_object(self, content_type, content_id): | ||
| """Get the content object (Movie or Episode) by UUID""" | ||
| try: | ||
| logger.info(f"[CONTENT-LOOKUP] Looking up {content_type} with UUID {content_id}") | ||
| if preferred_m3u_account_id: | ||
| logger.info(f"[CONTENT-LOOKUP] Preferred M3U account ID: {preferred_m3u_account_id}") | ||
| if preferred_stream_id: | ||
| logger.info(f"[CONTENT-LOOKUP] Preferred stream ID: {preferred_stream_id}") | ||
|
|
||
| if content_type == 'movie': | ||
| content_obj = get_object_or_404(Movie, uuid=content_id) | ||
| logger.info(f"[CONTENT-FOUND] Movie: {content_obj.name} (ID: {content_obj.id})") | ||
|
|
||
| # Filter by preferred stream ID first (most specific) | ||
| relations_query = content_obj.m3u_relations.filter(m3u_account__is_active=True) | ||
| if preferred_stream_id: | ||
| specific_relation = relations_query.filter(stream_id=preferred_stream_id).first() | ||
| if specific_relation: | ||
| logger.info(f"[STREAM-SELECTED] Using specific stream: {specific_relation.stream_id} from provider: {specific_relation.m3u_account.name}") | ||
| return content_obj, specific_relation | ||
| else: | ||
| logger.warning(f"[STREAM-FALLBACK] Preferred stream ID {preferred_stream_id} not found, falling back to account/priority selection") | ||
|
|
||
| # Filter by preferred M3U account if specified | ||
| if preferred_m3u_account_id: | ||
| specific_relation = relations_query.filter(m3u_account__id=preferred_m3u_account_id).first() | ||
| if specific_relation: | ||
| logger.info(f"[PROVIDER-SELECTED] Using preferred provider: {specific_relation.m3u_account.name}") | ||
| return content_obj, specific_relation | ||
| else: | ||
| logger.warning(f"[PROVIDER-FALLBACK] Preferred M3U account {preferred_m3u_account_id} not found, using highest priority") | ||
|
|
||
| # Get the highest priority active relation (fallback or default) | ||
| relation = relations_query.select_related('m3u_account').order_by('-m3u_account__priority', 'id').first() | ||
|
|
||
| if relation: | ||
| logger.info(f"[PROVIDER-SELECTED] Using provider: {relation.m3u_account.name} (priority: {relation.m3u_account.priority})") | ||
|
|
||
| return content_obj, relation | ||
|
|
||
| return get_object_or_404(Movie, uuid=content_id) | ||
| elif content_type == 'episode': | ||
| content_obj = get_object_or_404(Episode, uuid=content_id) | ||
| logger.info(f"[CONTENT-FOUND] Episode: {content_obj.name} (ID: {content_obj.id}, Series: {content_obj.series.name})") | ||
|
|
||
| # Filter by preferred stream ID first (most specific) | ||
| relations_query = content_obj.m3u_relations.filter(m3u_account__is_active=True) | ||
| if preferred_stream_id: | ||
| specific_relation = relations_query.filter(stream_id=preferred_stream_id).first() | ||
| if specific_relation: | ||
| logger.info(f"[STREAM-SELECTED] Using specific stream: {specific_relation.stream_id} from provider: {specific_relation.m3u_account.name}") | ||
| return content_obj, specific_relation | ||
| else: | ||
| logger.warning(f"[STREAM-FALLBACK] Preferred stream ID {preferred_stream_id} not found, falling back to account/priority selection") | ||
|
|
||
| # Filter by preferred M3U account if specified | ||
| if preferred_m3u_account_id: | ||
| specific_relation = relations_query.filter(m3u_account__id=preferred_m3u_account_id).first() | ||
| if specific_relation: | ||
| logger.info(f"[PROVIDER-SELECTED] Using preferred provider: {specific_relation.m3u_account.name}") | ||
| return content_obj, specific_relation | ||
| else: | ||
| logger.warning(f"[PROVIDER-FALLBACK] Preferred M3U account {preferred_m3u_account_id} not found, using highest priority") | ||
|
|
||
| # Get the highest priority active relation (fallback or default) | ||
| relation = relations_query.select_related('m3u_account').order_by('-m3u_account__priority', 'id').first() | ||
|
|
||
| if relation: | ||
| logger.info(f"[PROVIDER-SELECTED] Using provider: {relation.m3u_account.name} (priority: {relation.m3u_account.priority})") | ||
|
|
||
| return content_obj, relation | ||
|
|
||
| return get_object_or_404(Episode, uuid=content_id) | ||
| elif content_type == 'series': | ||
| # For series, get the first episode | ||
| series = get_object_or_404(Series, uuid=content_id) | ||
| logger.info(f"[CONTENT-FOUND] Series: {series.name} (ID: {series.id})") | ||
| episode = series.episodes.first() | ||
| if not episode: | ||
| logger.error(f"[CONTENT-ERROR] No episodes found for series {series.name}") | ||
| return None, None | ||
|
|
||
| logger.info(f"[CONTENT-FOUND] First episode: {episode.name} (ID: {episode.id})") | ||
|
|
||
| # Filter by preferred stream ID first (most specific) | ||
| relations_query = episode.m3u_relations.filter(m3u_account__is_active=True) | ||
| if preferred_stream_id: | ||
| specific_relation = relations_query.filter(stream_id=preferred_stream_id).first() | ||
| if specific_relation: | ||
| logger.info(f"[STREAM-SELECTED] Using specific stream: {specific_relation.stream_id} from provider: {specific_relation.m3u_account.name}") | ||
| return episode, specific_relation | ||
| else: | ||
| logger.warning(f"[STREAM-FALLBACK] Preferred stream ID {preferred_stream_id} not found, falling back to account/priority selection") | ||
|
|
||
| # Filter by preferred M3U account if specified | ||
| if preferred_m3u_account_id: | ||
| specific_relation = relations_query.filter(m3u_account__id=preferred_m3u_account_id).first() | ||
| if specific_relation: | ||
| logger.info(f"[PROVIDER-SELECTED] Using preferred provider: {specific_relation.m3u_account.name}") | ||
| return episode, specific_relation | ||
| else: | ||
| logger.warning(f"[PROVIDER-FALLBACK] Preferred M3U account {preferred_m3u_account_id} not found, using highest priority") | ||
|
|
||
| # Get the highest priority active relation (fallback or default) | ||
| relation = relations_query.select_related('m3u_account').order_by('-m3u_account__priority', 'id').first() | ||
|
|
||
| if relation: | ||
| logger.info(f"[PROVIDER-SELECTED] Using provider: {relation.m3u_account.name} (priority: {relation.m3u_account.priority})") | ||
|
|
||
| return episode, relation | ||
| # Return first episode for series | ||
| return series.episodes.first() | ||
| else: | ||
| logger.error(f"[CONTENT-ERROR] Invalid content type: {content_type}") | ||
| return None, None | ||
|
|
||
| return None | ||
| except Exception as e: | ||
| logger.error(f"Error getting content object: {e}") | ||
| return None, None | ||
| return None | ||
|
|
There was a problem hiding this comment.
Critical: Exception handling suppresses Http404, losing error details.
The broad Exception catch at line 464 will suppress the Http404 raised by get_object_or_404, converting it to None. While the caller re-raises Http404, this loses the detailed error information from Django's get_object_or_404.
Apply this diff to let Http404 propagate while catching other errors:
def _get_content_object(self, content_type, content_id):
"""Get the content object (Movie or Episode) by UUID"""
try:
if content_type == 'movie':
return get_object_or_404(Movie, uuid=content_id)
elif content_type == 'episode':
return get_object_or_404(Episode, uuid=content_id)
elif content_type == 'series':
series = get_object_or_404(Series, uuid=content_id)
# Return first episode for series
return series.episodes.first()
else:
return None
+ except Http404:
+ # Let Http404 propagate with its detailed error message
+ raise
except Exception as e:
- logger.error(f"Error getting content object: {e}")
+ logger.exception(f"Error getting content object: {e}")
return None🧰 Tools
🪛 Ruff (0.14.7)
464-464: Do not catch blind exception: Exception
(BLE001)
465-465: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In apps/proxy/vod_proxy/views.py around lines 451 to 467, the current broad
except Exception block swallows Http404 from get_object_or_404 and loses
Django's error details; change the exception handling so Http404 is allowed to
propagate (either by adding an explicit except Http404: raise) and only
catch/log other exceptions (e.g., except Exception as e: logger.error(...);
return None). Ensure Http404 is imported if needed and keep the existing return
None behavior for non-HTTP errors.
2 participants
Summary
Comprehensive fixes for VOD provider data handling issues, resolving duplicate key constraint violations, NULL value crashes, and malformed data imports.
Issues Resolved
Major Changes
1. Episode Deduplication Fix
Problem: Providers send multiple
stream_idsfor the same episode, causing duplicate key violations.Solution: Added batch-level tracking with
batch_episodesdictionary to ensure one Episode object with multiple M3UEpisodeRelation objects.Impact: Prevents constraint violations when processing duplicate streams.
2. NULL/Empty Name Handling
Problem: Database crashes when providers send NULL or empty string names.
Solution: Changed from
.get("name", "Unknown")to.get("name") or "MovieNameNull"The
oroperator handles both missing keys AND falsy values (None, ""), while.get()default only handles missing keys.Files Modified:
MovieNameNullSeriesNameNullEpisodeNameNullImpact: Prevents NOT NULL constraint violations, allows partial data import.
3. Malformed Episode Data Handling
Problem: Some providers return episodes as a list instead of dict, causing 337 series to import without episodes.
Solution: Added
isinstance()checks to handle both formats:{"1": [episodes], "2": [episodes]}[episodes](extract season from episode data)Impact: Fixed 337 series that previously had 0 episodes.
4. Bulk Update Unsaved Relations Fix
Problem: Django error "bulk_update() prohibited due to unsaved related object 'movie'"
Root Cause: Code updated
relations_to_createto use database Movie objects but forgot to do the same forrelations_to_update.Solution: Added ID mapping for both create and update relations:
Impact: Prevents crashes when new movies/series are created and existing relations need to reference them.
5. Memory Cleanup
Added explicit cleanup after batch operations to prevent memory buildup in containerized environments:
6. Stream Comparison Tool
Created auto-discovery tool to verify if duplicate streams are identical or quality variants:
Features:
Files Modified
apps/vod/tasks.py- Core deduplication, NULL handling, bulk_update fixescompare_streams.py- Stream comparison and verification toolEPISODE_DEDUP_FIX.md- User-facing documentationFIXES_DETAILED.md- Comprehensive technical documentation (573 lines)Testing
Episode Deduplication
SELECT episode_id, COUNT(*) FROM vod_m3uepisoderelation GROUP BY episode_id HAVING COUNT(*) > 1;NULL Name Handling
*NameNullplaceholdersSELECT name FROM vod_movie WHERE name LIKE '%NameNull';Malformed Data
Stream Comparison
Analyzes first 10 episodes with duplicate streams from each provider.
Documentation
All fixes include detailed technical documentation in
FIXES_DETAILED.mdcovering:Backward Compatibility
✅ No database migrations required
✅ No API changes
✅ No configuration changes
✅ Works with existing data
All changes are internal to batch processing logic.
Commits
Ready for review and testing
Summary by CodeRabbit
Bug Fixes
New Features
Performance
Tests & Docs
✏️ Tip: You can customize this high-level summary in your review settings.