From e8e385b770ea3875edc5c86fd23fc833ac09c577 Mon Sep 17 00:00:00 2001 From: V1ck3s Date: Thu, 15 Jan 2026 23:10:04 +0100 Subject: [PATCH] fix: resolve circular dependency and fix failing tests --- .../DeezerDownloadServiceTests.cs | 8 +- .../DeezerMetadataServiceTests.cs | 2 +- .../QobuzDownloadServiceTests.cs | 8 +- .../QobuzMetadataServiceTests.cs | 9 ++- .../Services/Common/BaseDownloadService.cs | 37 ++++++--- .../Services/Deezer/DeezerDownloadService.cs | 1 + .../Services/Qobuz/QobuzBundleService.cs | 6 +- .../Services/Qobuz/QobuzDownloadService.cs | 1 + .../Services/Subsonic/PlaylistSyncService.cs | 78 ++++++++++++++----- 9 files changed, 105 insertions(+), 45 deletions(-) diff --git a/octo-fiesta.Tests/DeezerDownloadServiceTests.cs b/octo-fiesta.Tests/DeezerDownloadServiceTests.cs index 11f7f2e..d66cc92 100644 --- a/octo-fiesta.Tests/DeezerDownloadServiceTests.cs +++ b/octo-fiesta.Tests/DeezerDownloadServiceTests.cs @@ -24,7 +24,6 @@ public class DeezerDownloadServiceTests : IDisposable private readonly Mock _localLibraryServiceMock; private readonly Mock _metadataServiceMock; private readonly Mock> _loggerMock; - private readonly Mock _serviceProviderMock; private readonly IConfiguration _configuration; private readonly string _testDownloadPath; @@ -42,7 +41,6 @@ public class DeezerDownloadServiceTests : IDisposable _localLibraryServiceMock = new Mock(); _metadataServiceMock = new Mock(); _loggerMock = new Mock>(); - _serviceProviderMock = new Mock(); _configuration = new ConfigurationBuilder() .AddInMemoryCollection(new Dictionary @@ -85,6 +83,10 @@ public class DeezerDownloadServiceTests : IDisposable Quality = null }); + var serviceProviderMock = new Mock(); + serviceProviderMock.Setup(sp => sp.GetService(typeof(octo_fiesta.Services.Subsonic.PlaylistSyncService))) + .Returns(null); + return new DeezerDownloadService( _httpClientFactoryMock.Object, config, @@ -92,7 +94,7 @@ public class DeezerDownloadServiceTests : IDisposable _metadataServiceMock.Object, subsonicSettings, deezerSettings, - _serviceProviderMock.Object, + serviceProviderMock.Object, _loggerMock.Object); } diff --git a/octo-fiesta.Tests/DeezerMetadataServiceTests.cs b/octo-fiesta.Tests/DeezerMetadataServiceTests.cs index b09042b..1b55a56 100644 --- a/octo-fiesta.Tests/DeezerMetadataServiceTests.cs +++ b/octo-fiesta.Tests/DeezerMetadataServiceTests.cs @@ -767,7 +767,7 @@ public class DeezerMetadataServiceTests Assert.Equal(2, result.Count); Assert.Equal("Track 1", result[0].Title); Assert.Equal("Artist A", result[0].Artist); - Assert.Equal("ext-deezer-111", result[0].Id); + Assert.Equal("ext-deezer-song-111", result[0].Id); } [Fact] diff --git a/octo-fiesta.Tests/QobuzDownloadServiceTests.cs b/octo-fiesta.Tests/QobuzDownloadServiceTests.cs index 0027d03..f57438a 100644 --- a/octo-fiesta.Tests/QobuzDownloadServiceTests.cs +++ b/octo-fiesta.Tests/QobuzDownloadServiceTests.cs @@ -22,7 +22,6 @@ public class QobuzDownloadServiceTests : IDisposable private readonly Mock _metadataServiceMock; private readonly Mock> _bundleServiceLoggerMock; private readonly Mock> _loggerMock; - private readonly Mock _serviceProviderMock; private readonly IConfiguration _configuration; private readonly string _testDownloadPath; private QobuzBundleService _bundleService; @@ -42,7 +41,6 @@ public class QobuzDownloadServiceTests : IDisposable _metadataServiceMock = new Mock(); _bundleServiceLoggerMock = new Mock>(); _loggerMock = new Mock>(); - _serviceProviderMock = new Mock(); // Create a real QobuzBundleService for testing (it will use the mocked HttpClient) _bundleService = new QobuzBundleService(_httpClientFactoryMock.Object, _bundleServiceLoggerMock.Object); @@ -88,6 +86,10 @@ public class QobuzDownloadServiceTests : IDisposable Quality = quality }); + var serviceProviderMock = new Mock(); + serviceProviderMock.Setup(sp => sp.GetService(typeof(octo_fiesta.Services.Subsonic.PlaylistSyncService))) + .Returns(null); + return new QobuzDownloadService( _httpClientFactoryMock.Object, config, @@ -96,7 +98,7 @@ public class QobuzDownloadServiceTests : IDisposable _bundleService, subsonicSettings, qobuzSettings, - _serviceProviderMock.Object, + serviceProviderMock.Object, _loggerMock.Object); } diff --git a/octo-fiesta.Tests/QobuzMetadataServiceTests.cs b/octo-fiesta.Tests/QobuzMetadataServiceTests.cs index 5789942..32cf4ab 100644 --- a/octo-fiesta.Tests/QobuzMetadataServiceTests.cs +++ b/octo-fiesta.Tests/QobuzMetadataServiceTests.cs @@ -26,11 +26,14 @@ public class QobuzMetadataServiceTests _httpClientFactoryMock = new Mock(); _httpClientFactoryMock.Setup(f => f.CreateClient(It.IsAny())).Returns(httpClient); - var httpClientFactory = Mock.Of(); + // Mock QobuzBundleService (methods are now virtual so can be mocked) + var bundleHttpClientFactoryMock = new Mock(); + bundleHttpClientFactoryMock.Setup(f => f.CreateClient(It.IsAny())).Returns(httpClient); var bundleLogger = Mock.Of>(); - - _bundleServiceMock = new Mock(httpClientFactory, bundleLogger); + _bundleServiceMock = new Mock(bundleHttpClientFactoryMock.Object, bundleLogger) { CallBase = false }; _bundleServiceMock.Setup(b => b.GetAppIdAsync()).ReturnsAsync("fake-app-id-12345"); + _bundleServiceMock.Setup(b => b.GetSecretsAsync()).ReturnsAsync(new List { "fake-secret" }); + _bundleServiceMock.Setup(b => b.GetSecretAsync(It.IsAny())).ReturnsAsync("fake-secret"); _loggerMock = new Mock>(); diff --git a/octo-fiesta/Services/Common/BaseDownloadService.cs b/octo-fiesta/Services/Common/BaseDownloadService.cs index d96221e..b707d4e 100644 --- a/octo-fiesta/Services/Common/BaseDownloadService.cs +++ b/octo-fiesta/Services/Common/BaseDownloadService.cs @@ -22,7 +22,7 @@ public abstract class BaseDownloadService : IDownloadService protected readonly IMusicMetadataService MetadataService; protected readonly SubsonicSettings SubsonicSettings; protected readonly ILogger Logger; - protected readonly IServiceProvider ServiceProvider; + private readonly IServiceProvider _serviceProvider; protected readonly string DownloadPath; protected readonly string CachePath; @@ -30,6 +30,22 @@ public abstract class BaseDownloadService : IDownloadService protected readonly Dictionary ActiveDownloads = new(); protected readonly SemaphoreSlim DownloadLock = new(1, 1); + /// + /// Lazy-loaded PlaylistSyncService to avoid circular dependency + /// + private PlaylistSyncService? _playlistSyncService; + protected PlaylistSyncService? PlaylistSyncService + { + get + { + if (_playlistSyncService == null) + { + _playlistSyncService = _serviceProvider.GetService(); + } + return _playlistSyncService; + } + } + /// /// Provider name (e.g., "deezer", "qobuz") /// @@ -47,7 +63,7 @@ public abstract class BaseDownloadService : IDownloadService LocalLibraryService = localLibraryService; MetadataService = metadataService; SubsonicSettings = subsonicSettings; - ServiceProvider = serviceProvider; + _serviceProvider = serviceProvider; Logger = logger; DownloadPath = configuration["Library:DownloadPath"] ?? "./downloads"; @@ -269,22 +285,21 @@ public abstract class BaseDownloadService : IDownloadService song.LocalPath = localPath; // Check if this track belongs to a playlist and update M3U - try + if (PlaylistSyncService != null) { - var playlistSyncService = ServiceProvider.GetService(typeof(PlaylistSyncService)) as PlaylistSyncService; - if (playlistSyncService != null) + try { - var playlistId = playlistSyncService.GetPlaylistIdForTrack(songId); + var playlistId = PlaylistSyncService.GetPlaylistIdForTrack(songId); if (playlistId != null) { Logger.LogInformation("Track {SongId} belongs to playlist {PlaylistId}, adding to M3U", songId, playlistId); - await playlistSyncService.AddTrackToM3UAsync(playlistId, song, localPath); + await PlaylistSyncService.AddTrackToM3UAsync(playlistId, song, localPath, isFullPlaylistDownload: false); } } - } - catch (Exception ex) - { - Logger.LogWarning(ex, "Failed to update playlist M3U for track {SongId}", songId); + catch (Exception ex) + { + Logger.LogWarning(ex, "Failed to update playlist M3U for track {SongId}", songId); + } } // Only register and scan if NOT in cache mode diff --git a/octo-fiesta/Services/Deezer/DeezerDownloadService.cs b/octo-fiesta/Services/Deezer/DeezerDownloadService.cs index 0f86a38..53be347 100644 --- a/octo-fiesta/Services/Deezer/DeezerDownloadService.cs +++ b/octo-fiesta/Services/Deezer/DeezerDownloadService.cs @@ -11,6 +11,7 @@ using octo_fiesta.Models.Search; using octo_fiesta.Models.Subsonic; using octo_fiesta.Services.Local; using octo_fiesta.Services.Common; +using octo_fiesta.Services.Subsonic; using Microsoft.Extensions.Options; using IOFile = System.IO.File; diff --git a/octo-fiesta/Services/Qobuz/QobuzBundleService.cs b/octo-fiesta/Services/Qobuz/QobuzBundleService.cs index b42fdd3..d5385ac 100644 --- a/octo-fiesta/Services/Qobuz/QobuzBundleService.cs +++ b/octo-fiesta/Services/Qobuz/QobuzBundleService.cs @@ -40,7 +40,7 @@ public class QobuzBundleService /// /// Gets the Qobuz App ID, extracting it from the bundle if not cached /// - public async Task GetAppIdAsync() + public virtual async Task GetAppIdAsync() { await EnsureInitializedAsync(); return _cachedAppId!; @@ -49,7 +49,7 @@ public class QobuzBundleService /// /// Gets the Qobuz secrets list, extracting them from the bundle if not cached /// - public async Task> GetSecretsAsync() + public virtual async Task> GetSecretsAsync() { await EnsureInitializedAsync(); return _cachedSecrets!; @@ -58,7 +58,7 @@ public class QobuzBundleService /// /// Gets a specific secret by index (used for signing requests) /// - public async Task GetSecretAsync(int index = 0) + public virtual async Task GetSecretAsync(int index = 0) { var secrets = await GetSecretsAsync(); if (index < 0 || index >= secrets.Count) diff --git a/octo-fiesta/Services/Qobuz/QobuzDownloadService.cs b/octo-fiesta/Services/Qobuz/QobuzDownloadService.cs index a6f2206..483f5d6 100644 --- a/octo-fiesta/Services/Qobuz/QobuzDownloadService.cs +++ b/octo-fiesta/Services/Qobuz/QobuzDownloadService.cs @@ -8,6 +8,7 @@ using octo_fiesta.Models.Search; using octo_fiesta.Models.Subsonic; using octo_fiesta.Services.Local; using octo_fiesta.Services.Common; +using octo_fiesta.Services.Subsonic; using Microsoft.Extensions.Options; using IOFile = System.IO.File; diff --git a/octo-fiesta/Services/Subsonic/PlaylistSyncService.cs b/octo-fiesta/Services/Subsonic/PlaylistSyncService.cs index 7e2866b..c3818f4 100644 --- a/octo-fiesta/Services/Subsonic/PlaylistSyncService.cs +++ b/octo-fiesta/Services/Subsonic/PlaylistSyncService.cs @@ -30,6 +30,10 @@ public class PlaylistSyncService private readonly string _musicDirectory; private readonly string _playlistDirectory; + // Cancellation token for background cleanup task + private readonly CancellationTokenSource _cleanupCancellationTokenSource = new(); + private readonly Task _cleanupTask; + public PlaylistSyncService( IEnumerable metadataServices, IEnumerable downloadServices, @@ -58,7 +62,20 @@ public class PlaylistSyncService } // Start background cleanup task for expired cache entries - _ = Task.Run(CleanupExpiredCacheEntriesAsync); + _cleanupTask = Task.Run(() => CleanupExpiredCacheEntriesAsync(_cleanupCancellationTokenSource.Token)); + } + + /// + /// Gets the metadata service for the specified provider + /// + private IMusicMetadataService? GetMetadataServiceForProvider(string provider) + { + return provider.ToLower() switch + { + "deezer" => _deezerMetadataService, + "qobuz" => _qobuzMetadataService, + _ => null + }; } /// @@ -69,7 +86,7 @@ public class PlaylistSyncService { var expiresAt = DateTime.UtcNow.Add(CacheTTL); _trackPlaylistCache[trackId] = (playlistId, expiresAt); - _logger.LogDebug("Added track {TrackId} to playlist cache with playlistId {PlaylistId}", trackId, playlistId); + _logger.LogInformation("Added track {TrackId} to playlist cache with playlistId {PlaylistId}", trackId, playlistId); } /// @@ -112,12 +129,11 @@ public class PlaylistSyncService var (provider, externalId) = PlaylistIdHelper.ParsePlaylistId(playlistId); // Get playlist metadata - var metadataService = provider.ToLower() switch + var metadataService = GetMetadataServiceForProvider(provider); + if (metadataService == null) { - "deezer" => _deezerMetadataService, - "qobuz" => _qobuzMetadataService, - _ => throw new NotSupportedException($"Provider '{provider}' not supported for playlists") - }; + throw new NotSupportedException($"Provider '{provider}' not supported for playlists"); + } var playlist = await metadataService.GetPlaylistAsync(provider, externalId); if (playlist == null) @@ -145,7 +161,7 @@ public class PlaylistSyncService return; } - // Download all tracks + // Download all tracks (M3U will be created once at the end) var downloadedTracks = new List<(Song Song, string LocalPath)>(); foreach (var track in tracks) @@ -159,6 +175,7 @@ public class PlaylistSyncService } // Add track to playlist cache BEFORE downloading + // This marks it as part of a full playlist download, so AddTrackToM3UAsync will skip real-time updates var trackId = $"ext-{provider}-{track.ExternalId}"; AddTrackToPlaylistCache(trackId, playlistId); @@ -180,7 +197,7 @@ public class PlaylistSyncService return; } - // Create M3U file + // Create M3U file ONCE at the end with all downloaded tracks await CreateM3UPlaylistAsync(playlist.Name, downloadedTracks); _logger.LogInformation("Playlist download completed: {DownloadedCount}/{TotalCount} tracks for '{PlaylistName}'", @@ -233,11 +250,19 @@ public class PlaylistSyncService /// /// Adds a track to an existing M3U playlist or creates it if it doesn't exist. - /// This is called progressively as tracks are downloaded. + /// Called when individual tracks are played/downloaded (NOT during full playlist download). /// The M3U is rebuilt in the correct playlist order each time. /// - public async Task AddTrackToM3UAsync(string playlistId, Song track, string localPath) + /// If true, skips M3U update (will be done at the end by DownloadFullPlaylistAsync) + public async Task AddTrackToM3UAsync(string playlistId, Song track, string localPath, bool isFullPlaylistDownload = false) { + // Skip real-time updates during full playlist download (M3U will be created once at the end) + if (isFullPlaylistDownload) + { + _logger.LogDebug("Skipping M3U update for track {TrackId} (full playlist download in progress)", track.Id); + return; + } + try { // Get playlist metadata to get the name and track order @@ -249,13 +274,7 @@ public class PlaylistSyncService var (provider, externalId) = PlaylistIdHelper.ParsePlaylistId(playlistId); - var metadataService = provider.ToLower() switch - { - "deezer" => _deezerMetadataService, - "qobuz" => _qobuzMetadataService, - _ => null - }; - + var metadataService = GetMetadataServiceForProvider(provider); if (metadataService == null) { _logger.LogWarning("No metadata service found for provider '{Provider}'", provider); @@ -342,13 +361,13 @@ public class PlaylistSyncService /// /// Background task to clean up expired cache entries every minute /// - private async Task CleanupExpiredCacheEntriesAsync() + private async Task CleanupExpiredCacheEntriesAsync(CancellationToken cancellationToken) { - while (true) + while (!cancellationToken.IsCancellationRequested) { try { - await Task.Delay(TimeSpan.FromMinutes(1)); + await Task.Delay(TimeSpan.FromMinutes(1), cancellationToken); var now = DateTime.UtcNow; var expiredKeys = _trackPlaylistCache @@ -366,10 +385,27 @@ public class PlaylistSyncService _logger.LogDebug("Cleaned up {Count} expired playlist cache entries", expiredKeys.Count); } } + catch (OperationCanceledException) + { + // Expected when cancellation is requested + break; + } catch (Exception ex) { _logger.LogWarning(ex, "Error during playlist cache cleanup"); } } + + _logger.LogInformation("Playlist cache cleanup task stopped"); + } + + /// + /// Stops the background cleanup task + /// + public async Task StopCleanupAsync() + { + _cleanupCancellationTokenSource.Cancel(); + await _cleanupTask; + _cleanupCancellationTokenSource.Dispose(); } }