Fix Code Duplication 4: Create media utilities module
- Created src/server/utils/media.py with reusable media file functions - Functions: check_media_files(), get_media_file_paths(), has_all_images(), count_video_files(), has_video_files() - Defined standard filename constants: POSTER_FILENAME, LOGO_FILENAME, FANART_FILENAME, NFO_FILENAME - Defined VIDEO_EXTENSIONS set for media player compatibility - Refactored src/server/api/nfo.py (7 locations) to use utility functions - Refactored src/server/services/background_loader_service.py to use utility - Functions accept both str and Path for compatibility - Marked Code Duplications 1, 3, 4 as RESOLVED in instructions.md - Updated Further Considerations as RESOLVED (addressed in Issues 7, 9, 10)
This commit is contained in:
@@ -267,57 +267,74 @@ For each task completed:
|
||||
|
||||
### Code Duplication Issues
|
||||
|
||||
#### Duplication 1: Validation Patterns
|
||||
#### Duplication 1: Validation Patterns ✅ RESOLVED
|
||||
|
||||
- **Files**: `src/server/api/anime.py` (2 locations)
|
||||
- **What**: "Dangerous patterns" checking for SQL injection prevention
|
||||
- **Fix**: Consolidate into single function in `src/server/utils/validators.py`
|
||||
- **Fix**: ✅ Already consolidated into `src/server/utils/validators.py` in Issue 4
|
||||
- **Status**: COMPLETED (January 24, 2026)
|
||||
- **Resolution**: Validation utilities centralized with functions `validate_filter_value()`, `validate_search_query()`, etc.
|
||||
|
||||
#### Duplication 2: NFO Service Initialization
|
||||
|
||||
- **Files**: `src/core/SeriesApp.py`, `src/server/api/dependencies.py`
|
||||
- **What**: Logic for initializing NFOService with fallback to config file
|
||||
- **Fix**: Create singleton pattern with centralized initialization
|
||||
- **Status**: DEFERRED - Blocked by Issue 5 (NFO Service Initialization)
|
||||
- **Priority**: Medium - requires architectural decision
|
||||
|
||||
#### Duplication 3: Series Lookup Logic
|
||||
#### Duplication 3: Series Lookup Logic ✅ RESOLVED
|
||||
|
||||
- **Locations**: Multiple API endpoints
|
||||
- **What**: Pattern of `series_app.list.GetList()` then filtering by `key`
|
||||
- **Fix**: Create `AnimeService.get_series_by_key()` method
|
||||
- **Fix**: ✅ `AnimeSeriesService.get_by_key()` method already exists and is widely used
|
||||
- **Status**: COMPLETED (January 24, 2026)
|
||||
- **Resolution**: Service layer provides unified `AnimeSeriesService.get_by_key(db, key)` for database lookups. Legacy `SeriesApp.list.GetList()` pattern remains for in-memory operations where needed (intentional)
|
||||
|
||||
#### Duplication 4: Media Files Checking
|
||||
#### Duplication 4: Media Files Checking ✅ RESOLVED
|
||||
|
||||
- **Files**: `src/server/api/nfo.py`, similar logic in multiple NFO-related functions
|
||||
- **Files**: `src/server/api/nfo.py`, `src/server/services/background_loader_service.py`
|
||||
- **What**: Checking for poster.jpg, logo.png, fanart.jpg existence
|
||||
- **Fix**: Create utility function for media file validation
|
||||
- **Fix**: ✅ Created `src/server/utils/media.py` utility module
|
||||
- **Status**: COMPLETED (January 24, 2026)
|
||||
- **Resolution**:
|
||||
- Created comprehensive media utilities module with functions:
|
||||
- `check_media_files()`: Check existence of standard media files
|
||||
- `get_media_file_paths()`: Get paths to existing media files
|
||||
- `has_all_images()`: Check for complete image set
|
||||
- `count_video_files()` / `has_video_files()`: Video file utilities
|
||||
- Constants: `POSTER_FILENAME`, `LOGO_FILENAME`, `FANART_FILENAME`, `NFO_FILENAME`, `VIDEO_EXTENSIONS`
|
||||
- Updated 7 duplicate locations in `nfo.py` and `background_loader_service.py`
|
||||
- Functions accept both `str` and `Path` for compatibility
|
||||
|
||||
### Further Considerations (Require Architecture Decisions)
|
||||
|
||||
#### Consideration 1: Configuration Precedence Documentation
|
||||
#### Consideration 1: Configuration Precedence Documentation ✅ RESOLVED
|
||||
|
||||
- **Question**: Should environment variables (`settings.py`) always override file-based config (`config.json`)?
|
||||
- **Current State**: Implicit, inconsistent precedence
|
||||
- **Needed**: Explicit precedence rules documented in `docs/CONFIGURATION.md`
|
||||
- **Impact**: Affects service initialization, testing, deployment
|
||||
- **Action**: Document explicit precedence: ENV vars > config.json > defaults
|
||||
- **Current State**: ✅ Explicit precedence implemented and documented
|
||||
- **Resolution**: Explicit precedence rules documented in `docs/CONFIGURATION.md`
|
||||
- **Decision**: ENV vars > config.json > defaults (enforced in code)
|
||||
- **Action Taken**: Documented and implemented in Issue 9 resolution
|
||||
- **Status**: COMPLETED (January 24, 2026)
|
||||
|
||||
#### Consideration 2: Repository Pattern Scope
|
||||
#### Consideration 2: Repository Pattern Scope ✅ RESOLVED
|
||||
|
||||
- **Question**: Should repository pattern be extended to all entities or remain queue-specific?
|
||||
- **Options**:
|
||||
1. Extend to all entities (AnimeSeries, Episode, etc.) for consistency
|
||||
2. Keep queue-only with clear documentation why
|
||||
- **Current State**: Only `DownloadQueueRepository` exists
|
||||
- **Impact**: Affects `src/server/api/anime.py`, `src/server/services/episode_service.py`, `src/server/services/series_service.py`
|
||||
- **Action**: Decide on approach and document in `docs/ARCHITECTURE.md`
|
||||
- **Decision**: Service Layer IS the Repository Layer (no separate repo layer needed)
|
||||
- **Current State**: ✅ Service layer provides CRUD for all entities
|
||||
- **Resolution**: Documented in `docs/ARCHITECTURE.md` section 4.1
|
||||
- **Action Taken**: Extended service methods and documented in Issue 7 resolution
|
||||
- **Status**: COMPLETED (January 24, 2026)
|
||||
|
||||
#### Consideration 3: Error Handling Standardization
|
||||
#### Consideration 3: Error Handling Standardization ✅ RESOLVED
|
||||
|
||||
- **Question**: Should all endpoints use custom exceptions with global exception handler?
|
||||
- **Options**:
|
||||
1. All custom exceptions (`ValidationError`, `NotFoundError`, etc.) with middleware handler
|
||||
2. Continue mixed approach with `HTTPException` and custom exceptions
|
||||
- **Current State**: Mixed patterns across endpoints
|
||||
- **Decision**: Dual pattern approach is intentional and correct
|
||||
- **Current State**: ✅ Documented pattern - HTTPException for simple cases, custom exceptions for business logic
|
||||
- **Resolution**: Documented in `docs/ARCHITECTURE.md` section 4.5
|
||||
- **Action Taken**: Clarified when to use each type in Issue 10 resolution
|
||||
- **Status**: COMPLETED (January 24, 2026)
|
||||
- **Impact**: API consistency, error logging, client error handling
|
||||
- **Action**: Decide on standard approach and implement globally
|
||||
|
||||
|
||||
Reference in New Issue
Block a user