fix: address review findings from extended metadata PR #79

Merged

joezombie merged 4 commits from fix/extended-metadata-review-fixes into main

2026-02-17 22:49:39 +00:00

joezombie commented

2026-02-15 23:44:38 +00:00

(Migrated from github.com)

Summary

Addresses all issues identified during the comprehensive review of PR #78 (extended episode metadata).

Critical fixes (original)

Perpetual backfill loop: Added sentinel defaults for Artist ("") and Bitrate (0) after ReadFileMetadata, matching the existing DurationSeconds ??= 0 pattern. Files without these ID3 tags are no longer re-parsed every sync cycle.
Silent exception swallowing: GetArtwork catch blocks now capture exception variables and log at Warning level with feed/file context
Missing exception handlers: Added IOException and UnauthorizedAccessException catches to GetArtwork, matching the pattern in ReadFileMetadata

Critical fixes (post-review)

NullReferenceException on picture.Data: Guard picture.Data before dereferencing picture.Data.Data; null/empty art data returns NotFound cleanly instead of 500
IOException/UnauthorizedAccessException returning 404: Now returns StatusCode(500) at LogError — these are server infrastructure failures, not "not found" conditions
Unhandled catch-all gap: Added catch (Exception ex) fallback in GetArtwork that logs at LogError and returns 500, preventing uncontextualized errors

Important fixes (original)

GetArtwork logging: Added Debug/Warning logging throughout the endpoint, matching existing GetMedia patterns
12 new controller tests: Input validation, path traversal, not-found, corrupt file handling for GetArtwork
RSS feed priority test: Verifies ThumbnailUrl takes precedence over embedded art
Backfill skip test: Verifies episodes with populated metadata are excluded from backfill
Stronger assertion: itunes:author test now verifies item-level element content

Important fixes (post-review)

Sentinel logging: Each sentinel write now emits a LogDebug entry (artist, bitrate, duration), making silent backfill failures discoverable in logs
HasEmbeddedArt stale state: Reset HasEmbeddedArt = false in CorruptFileException and UnsupportedFormatException catch blocks so unreadable files don't generate broken artwork URLs in the RSS feed
Bare catch(Exception) in GetEpisodesAsync: Narrowed to IOException/UnauthorizedAccessException so programming bugs propagate naturally instead of silently dropping episodes from the feed
Path.GetFullPath guard: Wrapped Path.GetFullPath calls in GetMediaFilePathAsync in try/catch (ArgumentException) to handle invalid feed.Directory database values

Minor fixes (original + post-review)

Comment explaining HasEmbeddedArt direct assignment — revised to remove misleading implication that ??= was a valid alternative (it's not for bool)
Sentinel comment now explains why Title needs no sentinel (filename fallback in ReadFileMetadata)
Log message for null media path is now neutral ("not found or not permitted") rather than implying file-not-found only
Symmetric LogDebug added for ThumbnailUrl artwork path to match existing embedded-art log
GetArtwork_WithCorruptFile test renamed from _WithFileWithoutEmbeddedArt to accurately reflect the tested exception

Test plan

dotnet build — 0 warnings, 0 errors
dotnet test — all 197 tests pass (7 new tests added: success-path artwork, whitespace input, sentinel boundary, negative itunes:author)

🤖 Generated with Claude Code

## Summary Addresses all issues identified during the comprehensive review of PR #78 (extended episode metadata). ### Critical fixes (original) - **Perpetual backfill loop**: Added sentinel defaults for `Artist` (`""`) and `Bitrate` (`0`) after `ReadFileMetadata`, matching the existing `DurationSeconds ??= 0` pattern. Files without these ID3 tags are no longer re-parsed every sync cycle. - **Silent exception swallowing**: `GetArtwork` catch blocks now capture exception variables and log at Warning level with feed/file context - **Missing exception handlers**: Added `IOException` and `UnauthorizedAccessException` catches to `GetArtwork`, matching the pattern in `ReadFileMetadata` ### Critical fixes (post-review) - **NullReferenceException on picture.Data**: Guard `picture.Data` before dereferencing `picture.Data.Data`; null/empty art data returns NotFound cleanly instead of 500 - **IOException/UnauthorizedAccessException returning 404**: Now returns `StatusCode(500)` at `LogError` — these are server infrastructure failures, not "not found" conditions - **Unhandled catch-all gap**: Added `catch (Exception ex)` fallback in `GetArtwork` that logs at `LogError` and returns 500, preventing uncontextualized errors ### Important fixes (original) - **GetArtwork logging**: Added Debug/Warning logging throughout the endpoint, matching existing `GetMedia` patterns - **12 new controller tests**: Input validation, path traversal, not-found, corrupt file handling for `GetArtwork` - **RSS feed priority test**: Verifies `ThumbnailUrl` takes precedence over embedded art - **Backfill skip test**: Verifies episodes with populated metadata are excluded from backfill - **Stronger assertion**: `itunes:author` test now verifies item-level element content ### Important fixes (post-review) - **Sentinel logging**: Each sentinel write now emits a `LogDebug` entry (artist, bitrate, duration), making silent backfill failures discoverable in logs - **HasEmbeddedArt stale state**: Reset `HasEmbeddedArt = false` in `CorruptFileException` and `UnsupportedFormatException` catch blocks so unreadable files don't generate broken artwork URLs in the RSS feed - **Bare catch(Exception) in GetEpisodesAsync**: Narrowed to `IOException`/`UnauthorizedAccessException` so programming bugs propagate naturally instead of silently dropping episodes from the feed - **Path.GetFullPath guard**: Wrapped `Path.GetFullPath` calls in `GetMediaFilePathAsync` in `try/catch (ArgumentException)` to handle invalid `feed.Directory` database values ### Minor fixes (original + post-review) - Comment explaining `HasEmbeddedArt` direct assignment — revised to remove misleading implication that `??=` was a valid alternative (it's not for `bool`) - Sentinel comment now explains why `Title` needs no sentinel (filename fallback in `ReadFileMetadata`) - Log message for null media path is now neutral ("not found or not permitted") rather than implying file-not-found only - Symmetric `LogDebug` added for `ThumbnailUrl` artwork path to match existing embedded-art log - `GetArtwork_WithCorruptFile` test renamed from `_WithFileWithoutEmbeddedArt` to accurately reflect the tested exception ## Test plan - [x] `dotnet build` — 0 warnings, 0 errors - [x] `dotnet test` — all 197 tests pass (7 new tests added: success-path artwork, whitespace input, sentinel boundary, negative itunes:author) 🤖 Generated with [Claude Code](https://claude.com/claude-code)