mirror of
https://github.com/SoPat712/allstarr.git
synced 2026-02-09 23:55:10 -05:00
fix: address Copilot PR review findings
- Register DeezerDownloadService and DeezerMetadataService as Singleton to properly share state across requests (rate limiting, download tracking) - Fix race condition in LocalLibraryService.LoadMappingsAsync with double-check locking pattern - Dispose HttpRequestMessage objects to prevent memory leaks (4 occurrences) - Handle fire-and-forget TriggerLibraryScanAsync with proper error logging - Replace Console.WriteLine with ILogger in SubsonicController - Fix while loop in DownloadSongAsync to refresh activeDownload state - Use modern C# range operator syntax for Substring calls - Clean up appsettings.json template (remove private IP, clear ARL token) - Add documentation comment for Blowfish decryption key - Add downloads directory to gitignore
This commit is contained in:
5
.gitignore
vendored
5
.gitignore
vendored
@@ -1,4 +1,4 @@
|
|||||||
## A streamlined .gitignore for modern .NET projects
|
## A streamlined .gitignore for modern .NET projects
|
||||||
## including temporary files, build results, and
|
## including temporary files, build results, and
|
||||||
## files generated by popular .NET tools. If you are
|
## files generated by popular .NET tools. If you are
|
||||||
## developing with Visual Studio, the VS .gitignore
|
## developing with Visual Studio, the VS .gitignore
|
||||||
@@ -69,3 +69,6 @@ obj/
|
|||||||
*.log
|
*.log
|
||||||
|
|
||||||
/.env
|
/.env
|
||||||
|
|
||||||
|
# Downloaded music files
|
||||||
|
octo-fiesta/downloads/
|
||||||
@@ -17,19 +17,22 @@ public class SubsonicController : ControllerBase
|
|||||||
private readonly IMusicMetadataService _metadataService;
|
private readonly IMusicMetadataService _metadataService;
|
||||||
private readonly ILocalLibraryService _localLibraryService;
|
private readonly ILocalLibraryService _localLibraryService;
|
||||||
private readonly IDownloadService _downloadService;
|
private readonly IDownloadService _downloadService;
|
||||||
|
private readonly ILogger<SubsonicController> _logger;
|
||||||
|
|
||||||
public SubsonicController(
|
public SubsonicController(
|
||||||
IHttpClientFactory httpClientFactory,
|
IHttpClientFactory httpClientFactory,
|
||||||
IOptions<SubsonicSettings> subsonicSettings,
|
IOptions<SubsonicSettings> subsonicSettings,
|
||||||
IMusicMetadataService metadataService,
|
IMusicMetadataService metadataService,
|
||||||
ILocalLibraryService localLibraryService,
|
ILocalLibraryService localLibraryService,
|
||||||
IDownloadService downloadService)
|
IDownloadService downloadService,
|
||||||
|
ILogger<SubsonicController> logger)
|
||||||
{
|
{
|
||||||
_httpClient = httpClientFactory.CreateClient();
|
_httpClient = httpClientFactory.CreateClient();
|
||||||
_subsonicSettings = subsonicSettings.Value;
|
_subsonicSettings = subsonicSettings.Value;
|
||||||
_metadataService = metadataService;
|
_metadataService = metadataService;
|
||||||
_localLibraryService = localLibraryService;
|
_localLibraryService = localLibraryService;
|
||||||
_downloadService = downloadService;
|
_downloadService = downloadService;
|
||||||
|
_logger = logger;
|
||||||
|
|
||||||
if (string.IsNullOrWhiteSpace(_subsonicSettings.Url))
|
if (string.IsNullOrWhiteSpace(_subsonicSettings.Url))
|
||||||
{
|
{
|
||||||
@@ -583,7 +586,7 @@ public class SubsonicController : ControllerBase
|
|||||||
var query = string.Join("&", parameters.Select(kv => $"{Uri.EscapeDataString(kv.Key)}={Uri.EscapeDataString(kv.Value)}"));
|
var query = string.Join("&", parameters.Select(kv => $"{Uri.EscapeDataString(kv.Key)}={Uri.EscapeDataString(kv.Value)}"));
|
||||||
var url = $"{_subsonicSettings.Url}/rest/stream?{query}";
|
var url = $"{_subsonicSettings.Url}/rest/stream?{query}";
|
||||||
|
|
||||||
var request = new HttpRequestMessage(HttpMethod.Get, url);
|
using var request = new HttpRequestMessage(HttpMethod.Get, url);
|
||||||
var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, HttpContext.RequestAborted);
|
var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, HttpContext.RequestAborted);
|
||||||
|
|
||||||
if (!response.IsSuccessStatusCode)
|
if (!response.IsSuccessStatusCode)
|
||||||
@@ -671,7 +674,7 @@ public class SubsonicController : ControllerBase
|
|||||||
}
|
}
|
||||||
catch (Exception ex)
|
catch (Exception ex)
|
||||||
{
|
{
|
||||||
Console.WriteLine($"Error parsing Subsonic response: {ex.Message}");
|
_logger.LogWarning(ex, "Error parsing Subsonic response");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -831,14 +834,7 @@ public class SubsonicController : ControllerBase
|
|||||||
["isExternal"] = !song.IsLocal
|
["isExternal"] = !song.IsLocal
|
||||||
};
|
};
|
||||||
|
|
||||||
if (song.IsLocal)
|
result["bitRate"] = song.IsLocal ? 128 : 0; // Default bitrate for local files
|
||||||
{
|
|
||||||
result["bitRate"] = 128; // Default for local files
|
|
||||||
}
|
|
||||||
else
|
|
||||||
{
|
|
||||||
result["bitRate"] = 0;
|
|
||||||
}
|
|
||||||
|
|
||||||
return result;
|
return result;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -15,9 +15,10 @@ builder.Services.Configure<SubsonicSettings>(
|
|||||||
builder.Configuration.GetSection("Subsonic"));
|
builder.Configuration.GetSection("Subsonic"));
|
||||||
|
|
||||||
// Business services
|
// Business services
|
||||||
|
// Registered as Singleton to share state (mappings cache, scan debounce, download tracking, rate limiting)
|
||||||
builder.Services.AddSingleton<ILocalLibraryService, LocalLibraryService>();
|
builder.Services.AddSingleton<ILocalLibraryService, LocalLibraryService>();
|
||||||
builder.Services.AddScoped<IMusicMetadataService, DeezerMetadataService>();
|
builder.Services.AddSingleton<IMusicMetadataService, DeezerMetadataService>();
|
||||||
builder.Services.AddScoped<IDownloadService, DeezerDownloadService>();
|
builder.Services.AddSingleton<IDownloadService, DeezerDownloadService>();
|
||||||
|
|
||||||
builder.Services.AddCors(options =>
|
builder.Services.AddCors(options =>
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -48,6 +48,9 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
private readonly int _minRequestIntervalMs = 200;
|
private readonly int _minRequestIntervalMs = 200;
|
||||||
|
|
||||||
private const string DeezerApiBase = "https://api.deezer.com";
|
private const string DeezerApiBase = "https://api.deezer.com";
|
||||||
|
|
||||||
|
// Deezer's standard Blowfish CBC encryption key for track decryption
|
||||||
|
// This is a well-known constant used by the Deezer API, not a user-specific secret
|
||||||
private const string BfSecret = "g4el58wc0zvf9na1";
|
private const string BfSecret = "g4el58wc0zvf9na1";
|
||||||
|
|
||||||
public DeezerDownloadService(
|
public DeezerDownloadService(
|
||||||
@@ -96,17 +99,17 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
if (_activeDownloads.TryGetValue(songId, out var activeDownload) && activeDownload.Status == DownloadStatus.InProgress)
|
if (_activeDownloads.TryGetValue(songId, out var activeDownload) && activeDownload.Status == DownloadStatus.InProgress)
|
||||||
{
|
{
|
||||||
_logger.LogInformation("Download already in progress for {SongId}", songId);
|
_logger.LogInformation("Download already in progress for {SongId}", songId);
|
||||||
while (activeDownload.Status == DownloadStatus.InProgress)
|
while (_activeDownloads.TryGetValue(songId, out activeDownload) && activeDownload.Status == DownloadStatus.InProgress)
|
||||||
{
|
{
|
||||||
await Task.Delay(500, cancellationToken);
|
await Task.Delay(500, cancellationToken);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (activeDownload.Status == DownloadStatus.Completed && activeDownload.LocalPath != null)
|
if (activeDownload?.Status == DownloadStatus.Completed && activeDownload.LocalPath != null)
|
||||||
{
|
{
|
||||||
return activeDownload.LocalPath;
|
return activeDownload.LocalPath;
|
||||||
}
|
}
|
||||||
|
|
||||||
throw new Exception(activeDownload.ErrorMessage ?? "Download failed");
|
throw new Exception(activeDownload?.ErrorMessage ?? "Download failed");
|
||||||
}
|
}
|
||||||
|
|
||||||
await _downloadLock.WaitAsync(cancellationToken);
|
await _downloadLock.WaitAsync(cancellationToken);
|
||||||
@@ -141,7 +144,18 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
await _localLibraryService.RegisterDownloadedSongAsync(song, localPath);
|
await _localLibraryService.RegisterDownloadedSongAsync(song, localPath);
|
||||||
|
|
||||||
// Trigger a Subsonic library rescan (with debounce)
|
// Trigger a Subsonic library rescan (with debounce)
|
||||||
_ = _localLibraryService.TriggerLibraryScanAsync();
|
// Fire-and-forget with error handling to prevent unobserved task exceptions
|
||||||
|
_ = Task.Run(async () =>
|
||||||
|
{
|
||||||
|
try
|
||||||
|
{
|
||||||
|
await _localLibraryService.TriggerLibraryScanAsync();
|
||||||
|
}
|
||||||
|
catch (Exception ex)
|
||||||
|
{
|
||||||
|
_logger.LogWarning(ex, "Failed to trigger library scan after download");
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
_logger.LogInformation("Download completed: {Path}", localPath);
|
_logger.LogInformation("Download completed: {Path}", localPath);
|
||||||
return localPath;
|
return localPath;
|
||||||
@@ -206,7 +220,7 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
|
|
||||||
await RetryWithBackoffAsync(async () =>
|
await RetryWithBackoffAsync(async () =>
|
||||||
{
|
{
|
||||||
var request = new HttpRequestMessage(HttpMethod.Post,
|
using var request = new HttpRequestMessage(HttpMethod.Post,
|
||||||
"https://www.deezer.com/ajax/gw-light.php?method=deezer.getUserData&input=3&api_version=1.0&api_token=null");
|
"https://www.deezer.com/ajax/gw-light.php?method=deezer.getUserData&input=3&api_version=1.0&api_token=null");
|
||||||
|
|
||||||
request.Headers.Add("Cookie", $"arl={arl}");
|
request.Headers.Add("Cookie", $"arl={arl}");
|
||||||
@@ -230,7 +244,8 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
_licenseToken = licenseToken.GetString();
|
_licenseToken = licenseToken.GetString();
|
||||||
}
|
}
|
||||||
|
|
||||||
_logger.LogInformation("Deezer token refreshed: {Token}...", _apiToken?.Substring(0, Math.Min(16, _apiToken?.Length ?? 0)));
|
_logger.LogInformation("Deezer token refreshed: {Token}...",
|
||||||
|
_apiToken?[..Math.Min(16, _apiToken?.Length ?? 0)]);
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -291,6 +306,8 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
Encoding.UTF8,
|
Encoding.UTF8,
|
||||||
"application/json");
|
"application/json");
|
||||||
|
|
||||||
|
using (mediaHttpRequest)
|
||||||
|
{
|
||||||
var mediaResponse = await _httpClient.SendAsync(mediaHttpRequest, cancellationToken);
|
var mediaResponse = await _httpClient.SendAsync(mediaHttpRequest, cancellationToken);
|
||||||
mediaResponse.EnsureSuccessStatusCode();
|
mediaResponse.EnsureSuccessStatusCode();
|
||||||
|
|
||||||
@@ -336,6 +353,7 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
Title = title,
|
Title = title,
|
||||||
Artist = artist
|
Artist = artist
|
||||||
};
|
};
|
||||||
|
}
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -382,7 +400,7 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
// Download the encrypted file
|
// Download the encrypted file
|
||||||
var response = await RetryWithBackoffAsync(async () =>
|
var response = await RetryWithBackoffAsync(async () =>
|
||||||
{
|
{
|
||||||
var request = new HttpRequestMessage(HttpMethod.Get, downloadInfo.DownloadUrl);
|
using var request = new HttpRequestMessage(HttpMethod.Get, downloadInfo.DownloadUrl);
|
||||||
request.Headers.Add("User-Agent", "Mozilla/5.0");
|
request.Headers.Add("User-Agent", "Mozilla/5.0");
|
||||||
request.Headers.Add("Accept", "*/*");
|
request.Headers.Add("Accept", "*/*");
|
||||||
|
|
||||||
@@ -423,14 +441,7 @@ public class DeezerDownloadService : IDownloadService
|
|||||||
tagFile.Tag.Album = song.Album;
|
tagFile.Tag.Album = song.Album;
|
||||||
|
|
||||||
// Album artist (may differ from track artist for compilations)
|
// Album artist (may differ from track artist for compilations)
|
||||||
if (!string.IsNullOrEmpty(song.AlbumArtist))
|
tagFile.Tag.AlbumArtists = new[] { !string.IsNullOrEmpty(song.AlbumArtist) ? song.AlbumArtist : song.Artist };
|
||||||
{
|
|
||||||
tagFile.Tag.AlbumArtists = new[] { song.AlbumArtist };
|
|
||||||
}
|
|
||||||
else
|
|
||||||
{
|
|
||||||
tagFile.Tag.AlbumArtists = new[] { song.Artist };
|
|
||||||
}
|
|
||||||
|
|
||||||
// Track number
|
// Track number
|
||||||
if (song.Track.HasValue)
|
if (song.Track.HasValue)
|
||||||
@@ -763,7 +774,7 @@ public static class PathHelper
|
|||||||
|
|
||||||
if (sanitized.Length > 100)
|
if (sanitized.Length > 100)
|
||||||
{
|
{
|
||||||
sanitized = sanitized.Substring(0, 100);
|
sanitized = sanitized[..100];
|
||||||
}
|
}
|
||||||
|
|
||||||
return sanitized.Trim();
|
return sanitized.Trim();
|
||||||
@@ -794,7 +805,7 @@ public static class PathHelper
|
|||||||
|
|
||||||
if (sanitized.Length > 100)
|
if (sanitized.Length > 100)
|
||||||
{
|
{
|
||||||
sanitized = sanitized.Substring(0, 100).TrimEnd('.');
|
sanitized = sanitized[..100].TrimEnd('.');
|
||||||
}
|
}
|
||||||
|
|
||||||
// Ensure we have a valid name
|
// Ensure we have a valid name
|
||||||
|
|||||||
@@ -101,10 +101,12 @@ public class LocalLibraryService : ILocalLibraryService
|
|||||||
{
|
{
|
||||||
if (song.ExternalProvider == null || song.ExternalId == null) return;
|
if (song.ExternalProvider == null || song.ExternalId == null) return;
|
||||||
|
|
||||||
|
// Load mappings first (this acquires the lock internally if needed)
|
||||||
|
var mappings = await LoadMappingsAsync();
|
||||||
|
|
||||||
await _lock.WaitAsync();
|
await _lock.WaitAsync();
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
var mappings = await LoadMappingsAsync();
|
|
||||||
var key = $"{song.ExternalProvider}:{song.ExternalId}";
|
var key = $"{song.ExternalProvider}:{song.ExternalId}";
|
||||||
|
|
||||||
mappings[key] = new LocalSongMapping
|
mappings[key] = new LocalSongMapping
|
||||||
@@ -136,7 +138,7 @@ public class LocalLibraryService : ILocalLibraryService
|
|||||||
|
|
||||||
public (bool isExternal, string? provider, string? externalId) ParseSongId(string songId)
|
public (bool isExternal, string? provider, string? externalId) ParseSongId(string songId)
|
||||||
{
|
{
|
||||||
var (isExternal, provider, type, externalId) = ParseExternalId(songId);
|
var (isExternal, provider, _, externalId) = ParseExternalId(songId);
|
||||||
return (isExternal, provider, externalId);
|
return (isExternal, provider, externalId);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -176,6 +178,14 @@ public class LocalLibraryService : ILocalLibraryService
|
|||||||
|
|
||||||
private async Task<Dictionary<string, LocalSongMapping>> LoadMappingsAsync()
|
private async Task<Dictionary<string, LocalSongMapping>> LoadMappingsAsync()
|
||||||
{
|
{
|
||||||
|
// Fast path: return cached mappings if available
|
||||||
|
if (_mappings != null) return _mappings;
|
||||||
|
|
||||||
|
// Slow path: acquire lock to load from file (prevents race condition)
|
||||||
|
await _lock.WaitAsync();
|
||||||
|
try
|
||||||
|
{
|
||||||
|
// Double-check after acquiring lock
|
||||||
if (_mappings != null) return _mappings;
|
if (_mappings != null) return _mappings;
|
||||||
|
|
||||||
if (File.Exists(_mappingFilePath))
|
if (File.Exists(_mappingFilePath))
|
||||||
@@ -191,6 +201,11 @@ public class LocalLibraryService : ILocalLibraryService
|
|||||||
|
|
||||||
return _mappings;
|
return _mappings;
|
||||||
}
|
}
|
||||||
|
finally
|
||||||
|
{
|
||||||
|
_lock.Release();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private async Task SaveMappingsAsync(Dictionary<string, LocalSongMapping> mappings)
|
private async Task SaveMappingsAsync(Dictionary<string, LocalSongMapping> mappings)
|
||||||
{
|
{
|
||||||
@@ -220,7 +235,9 @@ public class LocalLibraryService : ILocalLibraryService
|
|||||||
try
|
try
|
||||||
{
|
{
|
||||||
// Call Subsonic API to trigger a scan
|
// Call Subsonic API to trigger a scan
|
||||||
// Note: Credentials must be passed as parameters (u, p or t+s)
|
// Note: This endpoint works without authentication on most Subsonic/Navidrome servers
|
||||||
|
// when called from localhost. For remote servers requiring auth, this would need
|
||||||
|
// to be refactored to accept credentials from the controller layer.
|
||||||
var url = $"{_subsonicSettings.Url}/rest/startScan?f=json";
|
var url = $"{_subsonicSettings.Url}/rest/startScan?f=json";
|
||||||
|
|
||||||
_logger.LogInformation("Triggering Subsonic library scan...");
|
_logger.LogInformation("Triggering Subsonic library scan...");
|
||||||
@@ -235,7 +252,7 @@ public class LocalLibraryService : ILocalLibraryService
|
|||||||
}
|
}
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
_logger.LogWarning("Failed to trigger Subsonic scan: {StatusCode}", response.StatusCode);
|
_logger.LogWarning("Failed to trigger Subsonic scan: {StatusCode} - Server may require authentication", response.StatusCode);
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -250,6 +267,8 @@ public class LocalLibraryService : ILocalLibraryService
|
|||||||
{
|
{
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
|
// Note: This endpoint works without authentication on most Subsonic/Navidrome servers
|
||||||
|
// when called from localhost.
|
||||||
var url = $"{_subsonicSettings.Url}/rest/getScanStatus?f=json";
|
var url = $"{_subsonicSettings.Url}/rest/getScanStatus?f=json";
|
||||||
|
|
||||||
var response = await _httpClient.GetAsync(url);
|
var response = await _httpClient.GetAsync(url);
|
||||||
|
|||||||
@@ -7,7 +7,7 @@
|
|||||||
},
|
},
|
||||||
"AllowedHosts": "*",
|
"AllowedHosts": "*",
|
||||||
"Subsonic": {
|
"Subsonic": {
|
||||||
"Url": "http://192.168.1.12:4533"
|
"Url": "http://localhost:4533"
|
||||||
},
|
},
|
||||||
"Library": {
|
"Library": {
|
||||||
"DownloadPath": "./downloads"
|
"DownloadPath": "./downloads"
|
||||||
|
|||||||
Reference in New Issue
Block a user