Fix: VOD provider data handling and episode deduplication (#556, #569)#4
Fix: VOD provider data handling and episode deduplication (#556, #569)#4cmc0619 wants to merge 6 commits into
Conversation
…r#556, Dispatcharr#569) Fixes multiple critical issues with VOD provider data import and playback: Issue Dispatcharr#556 - Duplicate key constraint violations during VOD refresh: - Episodes from providers with multiple quality streams (same episode, different stream_ids) now create ONE Episode object with multiple M3UEpisodeRelation objects - Added batch_episodes dict to deduplicate episodes within batch by (series_id, season, episode) - Fixed PK resolution after bulk_create with ignore_conflicts=True by re-fetching all episodes - Properly map both relations_to_create AND relations_to_update to resolved Episode PKs Issue Dispatcharr#569 - MultipleObjectsReturned errors during episode playback: - Updated XC series stream lookup (apps/output/views.py) to resolve episodes via M3UEpisodeRelation - Filters by stream_id and orders by account priority, supporting deduplicated episode model NULL/empty name handling: - Changed from .get('name', 'Unknown') to .get('name') or 'MovieNameNull' - Prevents NOT NULL violations and distinguishes between missing vs explicitly empty names - Applied to Movie, Series, and Episode name fields Malformed episode data handling: - Added support for providers returning episodes as list instead of dict-by-season - Falls back to extracting season numbers from individual episode data - Logs warning when malformed structure detected Safe integer conversions: - Added safe_int() helper to handle None, empty strings, and non-numeric values - Applied to season_number and episode_number parsing to prevent ValueError crashes - Logs warnings for malformed data while continuing processing Bulk update fixes: - Extended Movie/Series/Episode relations_to_update mapping to reference saved DB objects - Prevents "bulk_update() prohibited due to unsaved related object" errors - Applies ID mapping before both bulk_create and bulk_update operations Scoped deletion: - Changed refresh_series_episodes to delete M3UEpisodeRelation instead of Episode objects - Supports cross-account episode sharing while handling provider deletions Technical Details: - apps/vod/tasks.py: batch_process_episodes, process_movie_batch, process_series_batch - apps/output/views.py: xc_series_stream lookup via M3UEpisodeRelation Impact: - Fixes 337 series that previously had 0 episodes due to malformed provider data - Prevents database crashes during VOD refresh with multiple providers - Enables proper playback of episodes from multiple provider sources
Exception Handling Improvements: - Distinguish provider/network failures from systemic errors in stream failover logic - Provider failures (RequestException, Timeout, ConnectionError, HTTPError) trigger automatic failover to next stream candidate with warning log - Systemic errors (Redis down, DB errors, code bugs) fail immediately with full traceback to prevent wasting resources trying all streams when infrastructure is down - Applied to both GET and HEAD methods in VOD stream failover loops Redis Client Centralization: - 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 from environment variables with retry logic and health checks - Applied to views.py HEAD method (content length storage) and connection_manager.py (retrieval) Technical Details: - apps/proxy/vod_proxy/views.py: Exception handling in lines 233-245 (GET), 428-439 (HEAD) - apps/proxy/vod_proxy/views.py: RedisClient usage in line 393 (HEAD content length storage) - apps/proxy/vod_proxy/connection_manager.py: RedisClient usage in line 99 (content length retrieval) Impact: - Prevents full stream iteration when infrastructure issues exist (faster failure, clearer errors) - Enables deployment in Docker/Kubernetes environments with non-localhost Redis - Provides better error visibility for debugging with targeted logging
Provides detailed documentation of all VOD provider fixes including: - Problem descriptions with actual error messages - Root cause analysis for each issue - Step-by-step failure scenarios - Complete fix explanations with code examples - Testing procedures - Impact analysis File: FIXES_DETAILED.md (573 lines) Note: This documentation file can be excluded from merge if only source code changes are desired.
Focused documentation on the episode deduplication solution: - Explains the "One Episode, Many Relations" model - Details the deduplication algorithm - Provides testing procedures and example queries File: EPISODE_DEDUP_FIX.md Note: This documentation file can be excluded from merge if only source code changes are desired.
Summary of all fixes with examples and testing steps. Useful as a pull request description or release notes. File: PR_DESCRIPTION.md Note: This documentation file can be excluded from merge if only source code changes are desired.
Prevents swallowing Django's Http404 exceptions which lose error details. Http404 now propagates properly while other exceptions are logged and return None. Addresses code review feedback about proper exception handling in helper methods.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughComprehensive refactor of the VOD task workflow introducing episode deduplication via batch tracking, improved handling for malformed/NULL provider data, bulk operation fixes with proper ID mapping, memory cleanup routines, and updated streaming logic with ranked account/stream selection and failover support across multiple proxy and output components. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant VODProxy as VOD Proxy (GET)
participant ContentLookup as Content Lookup
participant RelationRanking as Relation Ranking
participant M3UAccount as M3U Account Selection
participant StreamURL as Stream URL Builder
participant Provider as Provider Stream
participant Redis
Client->>VODProxy: GET /stream/{content_type}/{content_id}
VODProxy->>ContentLookup: _get_content_object(content_type, content_id)
ContentLookup-->>VODProxy: content_obj
VODProxy->>RelationRanking: _get_ranked_relations(content_obj)
RelationRanking-->>VODProxy: [relation₁, relation₂, relation₃...]
loop For each ranked relation (failover)
VODProxy->>M3UAccount: Select account from relation
M3UAccount-->>VODProxy: m3u_account
VODProxy->>StreamURL: _get_stream_url_from_relation(relation)
StreamURL->>M3UAccount: Get M3U profile
M3UAccount-->>StreamURL: profile
StreamURL->>StreamURL: _transform_url(raw_url, profile)
StreamURL-->>VODProxy: transformed_url
VODProxy->>Provider: Attempt stream (HEAD/GET)
alt Stream Available
Provider-->>VODProxy: 200/206 + headers
VODProxy->>Redis: Cache content_length
Redis-->>VODProxy: OK
VODProxy-->>Client: Stream response
else Provider/Network Error
Provider-->>VODProxy: Error
Note over VODProxy: Continue to next candidate
else Systemic Error
VODProxy->>VODProxy: Abort loop
end
end
alt All candidates failed
VODProxy-->>Client: 503 Service Unavailable
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
🧹 Nitpick comments (15)
apps/proxy/vod_proxy/connection_manager.py (1)
99-101: Use explicit branch instead of raising genericExceptionfor missing RedisThe
RedisClient.get_client()call and fallback behavior are fine, but usingraise Exception("Redis not available")just to hit theexceptand fall back to response headers is a bit noisy.You could simplify control flow and keep logs cleaner by branching directly:
- try: - r = RedisClient.get_client() - if not r: - raise Exception("Redis not available") + try: + r = RedisClient.get_client() + if not r: + logger.warning(f"[{self.session_id}] Redis not available, falling back to response content-length") + self.content_length = response.headers.get('content-length') + logger.info(f"[{self.session_id}] *** USING RESPONSE CONTENT LENGTH: {self.content_length} ***") + # Skip Redis lookup entirely + raise StopIteration # or `return` from the try-block context(or equivalent early‑return pattern without introducing a new exception type).
Not urgent, but it would make intent clearer and avoid using a bare
Exceptionfor normal control flow.PR_DESCRIPTION.md (1)
1-152: Add explicit languages to all fenced code blocks for markdownlint complianceThe documentation is clear and detailed. markdownlint is still flagging a few fenced code blocks without a language (MD040). For consistency with the blocks already tagged as
python/bash, consider adding a language to the remaining…fences (eventextis fine) so editors and renderers can apply proper highlighting and linters go quiet.FIXES_DETAILED.md (1)
27-531: Tag remaining fenced code blocks with a languageThis deep-dive doc is very helpful. markdownlint is still complaining about a few raw fenced blocks (error snippets, log excerpts, sample output) without a language spec (MD040). To keep CI/lint noise down and improve readability in editors, it’s worth tagging those as something like:
```text <error/output snippet>while keeping `python`/`bash` for actual code. </blockquote></details> <details> <summary>apps/vod/tasks.py (4)</summary><blockquote> `658-661`: **Unnecessary code path for `relations_to_update`.** The `relations_to_update` list contains existing relations from `existing_relations` dict (line 584), which are already fetched from the database with valid PKs. The `created_movies` dict only contains movies that were just created via `bulk_create`. Existing relations already reference existing movies (not newly created ones), so this loop will never find a match. This code is harmless but misleading. Consider adding a clarifying comment or removing if truly unreachable. --- `995-998`: **Same observation as movies - relations_to_update referencing newly created series is unlikely.** Same logic applies here: `relations_to_update` contains existing relations that already reference existing series objects. However, this is harmless defensive code. --- `1536-1538`: **Memory cleanup is good practice but `del` may not be necessary.** The explicit `clear()` calls and `del` statements help with garbage collection in low-memory environments. However, since these are local variables, they would be garbage collected when the function returns anyway. The `del` statement on line 1538 will raise `NameError` if the earlier code paths set `valid_relations` instead of these original variable names (since they're reassigned/filtered). This cleanup is defensive and acceptable for memory-constrained environments. Consider wrapping in try-except if you want to be extra safe: ```python # Explicit cleanup to help garbage collection in low-memory environments try: batch_episodes.clear() episodes_to_update_set.clear() except: pass
1253-1291: Robust handling of malformed provider data formats.Good defensive code handling both dict (normal) and list (malformed) episode data formats. The
safe_inthelper and warning logs are helpful for debugging provider issues.One minor observation: if
episodes_datais a list and episodes don't haveseason_numberorseasonfields, they'll all default to season 0. Multiple episodes without season information but sharing the same episode number will be deduplicated into a single episode (see batch_episodes tracking at lines 1324-1326). This is intentionally logged ("Reusing episode from batch"), but worth monitoring if providers commonly omit season data, as stream relations from distinct episodes could incorrectly link to the same episode object.EPISODE_DEDUP_FIX.md (1)
45-78: Line number references will become stale.The documentation references specific line numbers (e.g., "Lines 1198-1204", "Lines 1235-1299") which will drift as the code evolves. Consider using function names or code patterns instead of line numbers for more maintainable documentation.
Replace line number references with function/section descriptions:
-**1. Added Batch Tracking** (Lines 1198-1204) +**1. Added Batch Tracking** (in `batch_process_episodes()`)apps/proxy/vod_proxy/views.py (7)
202-202: Unused variablecurrent_connections.As flagged by static analysis,
current_connectionsfrom the profile result tuple is never used.- m3u_profile, current_connections = profile_result + m3u_profile, _ = profile_result
339-339: Same unused variable in HEAD handler.- m3u_profile, current_connections = profile_result + m3u_profile, _ = profile_result
376-378: Replace bareexceptwith specific exception handling.As flagged by static analysis, bare
except:catches all exceptions includingKeyboardInterruptandSystemExit.try: total_size = content_range.split('/')[-1] - except: + except (IndexError, ValueError): total_size = response.headers.get('Content-Length', '0')
381-381: Unnecessary f-string prefix.The log message has no placeholders.
- logger.warning(f"[VOD-HEAD] No Content-Range header in 206 response") + logger.warning("[VOD-HEAD] No Content-Range header in 206 response")
392-401: Good Redis integration for session state, but consider error handling granularity.The Redis client retrieval and error handling is appropriate. However, raising a generic
Exception("Redis not available")could be improved.Consider a more specific exception or graceful degradation:
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 caching") + else: + content_length_key = f"vod_content_length:{session_id}" + r.set(content_length_key, total_size, ex=1800) + 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) - 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}")
451-468: Http404 propagation is correct but could uselogging.exception.The method correctly re-raises
Http404for Django's error handling. However, the genericExceptioncatch should uselogging.exceptionfor better stack traces.except Http404: raise # Let 404s propagate to Django's error handling except Exception as e: - logger.error(f"Error getting content object: {e}") + logger.exception(f"Error getting content object: {e}") return None
517-518: Uselogging.exceptionfor better error context.As flagged by static analysis:
except Exception as e: - logger.error(f"Error getting ranked relations: {e}") + logger.exception(f"Error getting ranked relations: {e}") return []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
EPISODE_DEDUP_FIX.md(1 hunks)FIXES_DETAILED.md(1 hunks)PR_DESCRIPTION.md(1 hunks)apps/output/views.py(1 hunks)apps/proxy/vod_proxy/connection_manager.py(1 hunks)apps/proxy/vod_proxy/views.py(2 hunks)apps/vod/tasks.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/vod/tasks.py (1)
apps/vod/models.py (2)
M3UEpisodeRelation(257-295)Episode(153-187)
apps/proxy/vod_proxy/connection_manager.py (1)
core/utils.py (2)
RedisClient(44-194)get_client(49-138)
apps/output/views.py (1)
apps/vod/models.py (1)
M3UEpisodeRelation(257-295)
apps/proxy/vod_proxy/views.py (3)
apps/proxy/vod_proxy/multi_worker_connection_manager.py (4)
MultiWorkerVODConnectionManager(622-1370)get_instance(628-632)stream_content_with_session(699-1007)infer_content_type_from_url(27-77)apps/proxy/vod_proxy/connection_manager.py (2)
get_instance(285-289)stream_content_with_session(960-1245)core/utils.py (2)
RedisClient(44-194)get_client(49-138)
🪛 markdownlint-cli2 (0.18.1)
PR_DESCRIPTION.md
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)
FIXES_DETAILED.md
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)
🪛 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)
466-466: Do not catch blind exception: Exception
(BLE001)
467-467: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
515-515: Consider moving this statement to an else block
(TRY300)
517-517: Do not catch blind exception: Exception
(BLE001)
518-518: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (13)
apps/output/views.py (1)
2889-2896: Episode lookup bystream_idand priority looks correctUsing
M3UEpisodeRelation.objects.filter(stream_id=stream_id, m3u_account__is_active=True).order_by('-m3u_account__priority', 'id').first()cleanly aligns the XC episode stream lookup with the new “one Episode, many relations” model and multi‑account priority selection. The 404 fallback is also appropriate.I’d just suggest verifying end‑to‑end that the incoming XC
stream_idreally is the providerstream_id(not the internal episode or relation PK) across all call sites, since this is a behavior change at the API edge.apps/vod/tasks.py (8)
377-378: LGTM! Good defensive handling for NULL/empty names.Using
movie_data.get('name') or 'MovieNameNull'correctly handles bothNoneand empty string''cases, preventing database crashes from NULL name constraint violations.
695-696: Consistent NULL/empty name handling for Series.Good - same defensive pattern applied for series names.
1219-1225: Correctly scoped deletion to M3UEpisodeRelation only.The comment and implementation correctly preserve Episode objects for cross-account deduplication while removing only the relation entries for the current account. This is essential for the deduplication strategy.
1320-1326: Core deduplication tracking structures properly initialized.The
episodes_to_update_setandbatch_episodesdict are the key data structures enabling the deduplication fix. They're correctly initialized within the function scope (not as module-level globals, despite the AI summary mention), which is appropriate for batch-level tracking.
1331-1335: Good NULL handling for episode names and safe episode number conversion.The pattern
episode_data.get('title') or 'EpisodeNameNull'andsafe_int()usage are consistent with the movie/series handling.
1359-1423: Episode deduplication logic is correct and well-structured.The batch-first lookup pattern (
batch_episodes→existing_episodes→ create new) properly ensures:
- Only one Episode object per (series, season, episode) tuple
- Updates are queued only once via
episodes_to_update_set- All subsequent stream entries reuse the same Episode object
The
id(episode)usage for tracking is clever - it distinguishes between different in-memory Episode instances even if they represent the same database record.
1508-1516: Good filtering of relations with unresolved episodes.The validation
r.episode and r.episode.pkbefore bulk_create prevents integrity errors from relations pointing to unsaved episodes. The warning log helps with debugging.
1456-1499: No issue found with PK resolution after bulk_create.The code correctly uses
relation.episode.series_idto access the integer foreign key ID. Django's ForeignKey field always stores the ID value separately from the related object instance, regardless of whether the Episode is saved. When an Episode is instantiated withseries=series_obj(line 59 of context), Django immediately stores the integer ID in memory via the_idaccessor. Therefore,relation.episode.series_idwill correctly return the integer ID, not the Series object.EPISODE_DEDUP_FIX.md (1)
1-263: Comprehensive and well-structured documentation.The documentation clearly explains the problem, solution, verification steps, and backward compatibility. The SQL verification queries and expected log output are particularly helpful for debugging.
apps/proxy/vod_proxy/views.py (3)
159-172: Good refactoring to content-first approach with ranked relations.The separation of content fetching (
_get_content_object) from relation ranking (_get_ranked_relations) improves code organization and enables the multi-candidate failover pattern.
181-249: Well-implemented failover loop with proper error categorization.The distinction between provider/network errors (continue to next candidate) and systemic errors (fail immediately) is correct. This prevents wasting time on retries when the issue isn't provider-specific.
470-519: Well-designed ranking algorithm with clear priority order.The ranking logic (preferred stream → preferred account → priority → id) is correct and well-documented. The early return for empty relations is efficient.
One observation: the query
remaining_relations.exclude(id=specific.id)creates new querysets each time. For small result sets this is fine, but for large relation counts, consider collecting IDs to exclude in a single query.
| # Calculate actual relations created (valid_relations if it was set, otherwise relations_to_create) | ||
| relations_created_count = len(valid_relations) if 'valid_relations' in locals() else len(relations_to_create) |
There was a problem hiding this comment.
Potential NameError if valid_relations not defined.
The condition 'valid_relations' in locals() is a code smell and can fail in edge cases. If episodes_to_create is empty (falsy), the if episodes_to_create: block (line 1459) is skipped, and valid_relations is never assigned.
Consider initializing valid_relations before the conditional block:
+ valid_relations = relations_to_create # Default to all relations
+
# Create new episodes with ignore_conflicts to handle race conditions
# where another account might create the same episode simultaneously
if episodes_to_create:
# ... existing code ...
# Filter out any relations that still have unsaved episodes
valid_relations = [r for r in relations_to_create if r.episode and r.episode.pk]🤖 Prompt for AI Agents
In apps/vod/tasks.py around lines 1524-1525, the code checks for
'valid_relations' in locals() which can raise a NameError when the earlier block
that assigns valid_relations is skipped; initialize valid_relations as an empty
list (or the appropriate empty collection) before the conditional that populates
it so the later len(...) call can always safely use valid_relations, and then
replace the conditional locals() check with a direct use of valid_relations
(i.e., relations_created_count = len(valid_relations) if valid_relations else
len(relations_to_create) or simply len(valid_relations) depending on intended
semantics).
|
Warning Docstrings generation is disabled for your repository or organization. |
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
2. NULL/Empty Name Handling
.get('name', 'Unknown')to.get('name') or 'MovieNameNull'3. Malformed Episode Data
4. Bulk Update Fixes
5. VOD Proxy Improvements
Files Modified
apps/vod/tasks.py- Core deduplication, NULL handling, safe_intapps/output/views.py- Episode lookup via M3UEpisodeRelationapps/proxy/vod_proxy/views.py- Exception handling, Redis clientapps/proxy/vod_proxy/connection_manager.py- Redis clientTesting
Verify episode deduplication:
Check NULL handling:
Backward Compatibility
✅ No database migrations required
✅ No API changes
✅ No configuration changes
✅ Works with existing data
Clean atomic commits - easy to cherry-pick individual fixes
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.