From bd855073088e819b22d54e2463c04cebbacd5a92 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 19 Jun 2026 15:44:41 -0400 Subject: [PATCH] Block duplicate-release uploads by (title, artist): pre-flight check + server 409 backstop, with within-batch Cut attach via releaseId --- DeepDrftAPI/Controllers/TrackController.cs | 44 +++- DeepDrftAPI/DeepDrftAPI.csproj | 6 + DeepDrftAPI/Services/UnifiedTrackService.cs | 118 +++++++-- .../Components/Pages/Tracks/BatchEdit.razor | 8 + .../Components/Pages/Tracks/BatchUpload.razor | 46 +++- DeepDrftManager/Services/CmsTrackService.cs | 51 ++++ DeepDrftManager/Services/ICmsTrackService.cs | 15 ++ DeepDrftTests/DeepDrftTests.csproj | 3 + .../UploadDuplicateDetectionTests.cs | 245 ++++++++++++++++++ 9 files changed, 505 insertions(+), 31 deletions(-) create mode 100644 DeepDrftTests/UploadDuplicateDetectionTests.cs diff --git a/DeepDrftAPI/Controllers/TrackController.cs b/DeepDrftAPI/Controllers/TrackController.cs index eade8f9..41360ae 100644 --- a/DeepDrftAPI/Controllers/TrackController.cs +++ b/DeepDrftAPI/Controllers/TrackController.cs @@ -96,6 +96,37 @@ public class TrackController : ControllerBase return Ok(result.Value); } + // GET api/track/release/exists?title=...&artist=... ([ApiKeyAuthorize]) + // Upload-form pre-flight: does a release with this exact (title, artist) already exist? Returns the + // matching ReleaseDto (so the caller can name it in the block message) or 404 when none exists. Uses + // the same GetReleaseByTitleAndArtist read the upload create-path duplicate guard uses, so the + // pre-flight and the server backstop agree on the match by construction (exact ordinal comparison, + // soft-deleted rows excluded). "release/exists" is a literal 2-segment route declared before the + // parameterized "{trackId}" route and distinct from "release/{id:long}" (different segment shape). + [ApiKeyAuthorize] + [HttpGet("release/exists")] + public async Task ReleaseExists( + [FromQuery] string? title, + [FromQuery] string? artist, + CancellationToken ct = default) + { + if (string.IsNullOrWhiteSpace(title) || string.IsNullOrWhiteSpace(artist)) + return BadRequest("title and artist are both required"); + + var result = await _sqlTrackService.GetReleaseByTitleAndArtist(title, artist, ct); + if (!result.Success) + { + var error = result.Messages.FirstOrDefault()?.Message ?? "Unknown error"; + _logger.LogError("ReleaseExists failed for ({Title}, {Artist}): {Error}", title, artist, error); + return StatusCode(500, "Failed to check release"); + } + + if (result.Value is null) + return NotFound(); + + return Ok(result.Value); + } + // GET api/track/genres (unauthenticated) // Distinct non-null genres with track counts. Public browse data, same posture as GET // api/track/page. Literal segment, declared before the parameterized "{trackId}" route. @@ -220,6 +251,7 @@ public class TrackController : ControllerBase [FromForm] string? releaseType, [FromForm] string? medium, [FromForm] int? trackNumber, + [FromForm] long? releaseId, CancellationToken cancellationToken) { _logger.LogInformation("UploadTrack called: trackName={TrackName}, artist={Artist}, fileName={FileName}, size={Size}", @@ -315,6 +347,7 @@ public class TrackController : ControllerBase parsedReleaseType, parsedMedium, resolvedTrackNumber, + releaseId, cancellationToken); if (!result.Success || result.Value is null) @@ -322,14 +355,19 @@ public class TrackController : ControllerBase var error = result.Messages.FirstOrDefault()?.Message ?? "Failed to process and store audio"; _logger.LogWarning("UploadTrack: UnifiedTrackService failed for {TrackName}: {Error}", trackName, error); - // A cardinality rejection is a well-formed request that violates a domain rule, so it - // is 409 Conflict — distinct from the 500 used for processing failure. The marker is - // stripped so the client sees only the human-readable detail. + // A cardinality or duplicate-release rejection is a well-formed request that violates a + // domain rule, so it is 409 Conflict — distinct from the 500 used for processing failure. + // The marker is stripped so the client sees only the human-readable detail. if (error.StartsWith(UnifiedTrackService.CardinalityViolationMarker, StringComparison.Ordinal)) { return Conflict(error[UnifiedTrackService.CardinalityViolationMarker.Length..]); } + if (error.StartsWith(UnifiedTrackService.DuplicateReleaseMarker, StringComparison.Ordinal)) + { + return Conflict(error[UnifiedTrackService.DuplicateReleaseMarker.Length..]); + } + return StatusCode(500, error); } diff --git a/DeepDrftAPI/DeepDrftAPI.csproj b/DeepDrftAPI/DeepDrftAPI.csproj index df7b2df..fe133a5 100644 --- a/DeepDrftAPI/DeepDrftAPI.csproj +++ b/DeepDrftAPI/DeepDrftAPI.csproj @@ -18,6 +18,12 @@ + + + + + diff --git a/DeepDrftAPI/Services/UnifiedTrackService.cs b/DeepDrftAPI/Services/UnifiedTrackService.cs index 208c7dd..1ed985d 100644 --- a/DeepDrftAPI/Services/UnifiedTrackService.cs +++ b/DeepDrftAPI/Services/UnifiedTrackService.cs @@ -25,6 +25,16 @@ public class UnifiedTrackService /// follows the marker and is what the CMS surfaces to the admin. /// internal const string CardinalityViolationMarker = "CARDINALITY_VIOLATION: "; + + /// + /// Stable marker prefixed onto a duplicate-release rejection so the controller can map it to 409 + /// Conflict, the same way is mapped. Fires when an upload + /// with no explicit releaseId would create a release whose (title, artist) already exists in the + /// catalogue — the upload form is a create-new tool, never an edit/append path. The human-readable + /// detail follows the marker and is what the CMS surfaces to the admin. + /// + internal const string DuplicateReleaseMarker = "DUPLICATE_RELEASE: "; + private readonly TrackContentService _contentTrackContentService; private readonly ITrackService _sqlTrackService; private readonly FileDb _fileDatabase; @@ -64,33 +74,66 @@ public class UnifiedTrackService ReleaseType releaseType, ReleaseMedium medium, int trackNumber, + long? releaseId, CancellationToken ct) { - // Cardinality pre-check — BEFORE the vault write so a rejected over-limit add never orphans - // audio in the tracks vault. This is a READ-only peek (no release is created for an upload we - // may reject); the real FindOrCreateRelease still runs below for the accepted path. Only the - // find path can violate: a release that does not yet exist has zero tracks and admits its - // first. The guard is the general form `(liveCount + 1) > Max`, not Session/Mix-hardcoded, so - // a future bounded medium is covered by the same line. + // Resolve which release this track lands on BEFORE the vault write, so a rejected upload never + // orphans audio. Two paths: + // - releaseId is null → CREATE path: this is the first row of a submit. (title, artist) must + // NOT already exist — the upload form creates new releases only. A pre-existing match is a + // duplicate and is blocked (409). + // - releaseId is set → ATTACH path: rows 2..N of a within-batch multi-track Cut, attaching + // to the release row 1 just created. No (title, artist) lookup — the release id is + // authoritative — so the within-batch build is never mistaken for a pre-existing duplicate. + // Both paths run the cardinality guard `(liveCount + 1) > Max` (not Session/Mix-hardcoded, so a + // future bounded medium is covered by the same line). + ResolvedRelease? resolved = null; if (!string.IsNullOrWhiteSpace(album)) { - var peek = await _sqlTrackService.GetReleaseByTitleAndArtist(album, artist, ct); - if (!peek.Success) + if (releaseId is { } attachId) { - var error = peek.Messages.FirstOrDefault()?.Message ?? "Unknown error"; - _logger.LogError("UploadAsync: release peek failed for ({Album}, {Artist}): {Error}", album, artist, error); - return ResultContainer.CreateFailResult($"Could not verify the release: {error}"); - } + var attachPeek = await _sqlTrackService.GetReleaseByTitleAndArtist(album, artist, ct); + if (!attachPeek.Success) + { + var error = attachPeek.Messages.FirstOrDefault()?.Message ?? "Unknown error"; + _logger.LogError("UploadAsync: release peek failed for ({Album}, {Artist}): {Error}", album, artist, error); + return ResultContainer.CreateFailResult($"Could not verify the release: {error}"); + } - if (peek.Value is { } existing) - { - var cardinality = MediumRules.CardinalityOf(existing.Medium); - if (existing.TrackCount + 1 > cardinality.Max) + // The attach target must be the same release the natural key resolves to — a guard against + // a stale/forged releaseId pointing at a different (title, artist) than this row carries. + if (attachPeek.Value is not { } target || target.Id != attachId) { return ResultContainer.CreateFailResult( - $"{CardinalityViolationMarker}A {existing.Medium} release holds a single track; " + - $"'{existing.Title}' already has one — edit the existing track or choose a different release."); + $"{DuplicateReleaseMarker}The release this track should attach to could not be found. " + + "Start the upload again."); } + + var cardinalityCheck = CheckCardinality(target); + if (cardinalityCheck is { } violation) + return ResultContainer.CreateFailResult(violation); + + resolved = new ResolvedRelease(target.Id); + } + else + { + var peek = await _sqlTrackService.GetReleaseByTitleAndArtist(album, artist, ct); + if (!peek.Success) + { + var error = peek.Messages.FirstOrDefault()?.Message ?? "Unknown error"; + _logger.LogError("UploadAsync: release peek failed for ({Album}, {Artist}): {Error}", album, artist, error); + return ResultContainer.CreateFailResult($"Could not verify the release: {error}"); + } + + // CREATE path: a pre-existing (title, artist) is a duplicate. Block it — the form never + // edits or appends to an existing release. + if (peek.Value is { } existing) + { + return ResultContainer.CreateFailResult( + $"{DuplicateReleaseMarker}A release titled '{existing.Title}' by {existing.Artist} already " + + "exists. The upload form creates new releases only — use the edit tools to change an existing one."); + } + // resolved stays null → FindOrCreateRelease below creates the release. } } @@ -109,9 +152,12 @@ public class UnifiedTrackService // shared release (created on first sighting); an upload without one stays a loose track with // a null ReleaseId. Release-cardinal metadata (artist/genre/description/releaseDate/type/uploader) // rides on the release, not the track. - long? releaseId = null; - if (!string.IsNullOrWhiteSpace(album)) + long? resolvedReleaseId = resolved?.Id; + if (!string.IsNullOrWhiteSpace(album) && resolvedReleaseId is null) { + // CREATE path only: the duplicate guard above proved no (title, artist) match exists, so this + // mints the release. (The attach path already resolved the id from the pre-check above and + // skips FindOrCreateRelease entirely, so a within-batch row never re-runs the natural-key find.) var releaseData = new ReleaseDto { Title = album, @@ -124,11 +170,9 @@ public class UnifiedTrackService CreatedByUserId = createdByUserId, }; - // Medium (like every other field in releaseData) applies only when this upload CREATES the - // release. FindOrCreateRelease returns an existing (title, artist) row untouched — the first - // upload's medium is authoritative. Do NOT "fix" this to overwrite the stored medium on a - // subsequent track add: medium is a release-level property, changed only via the edit path - // (PUT api/track/meta), never silently flipped by adding a track to an existing release. + // FindOrCreateRelease's find branch still backstops a concurrent insert of the same + // (title, artist) between the duplicate peek and this call — it returns the winning row + // rather than throwing. Medium and every other field apply only on the create it performs. var releaseResult = await _sqlTrackService.FindOrCreateRelease(album, artist, releaseData, ct); if (!releaseResult.Success || releaseResult.Value is null) { @@ -139,11 +183,11 @@ public class UnifiedTrackService return ResultContainer.CreateFailResult($"Track was uploaded but could not be saved: {error}"); } - releaseId = releaseResult.Value.Id; + resolvedReleaseId = releaseResult.Value.Id; } var trackDto = TrackConverter.Convert(unpersisted); - trackDto.ReleaseId = releaseId; + trackDto.ReleaseId = resolvedReleaseId; trackDto.Release = null; // FK already resolved; Create must not re-resolve a detached graph. var saveResult = await _sqlTrackService.Create(trackDto); @@ -166,6 +210,26 @@ public class UnifiedTrackService return saveResult; } + // The release a track resolved onto before the vault write. A null Id is the create path (mint + // below); a non-null Id is the attach path (a within-batch multi-track Cut row 2..N). + private readonly record struct ResolvedRelease(long Id); + + // The cardinality guard shared by the attach path and (historically) the create path: a release + // already at its medium's Max rejects a further track. Returns the marker-prefixed rejection + // message, or null when the add is within limits. The create path never trips this (a brand-new + // release has zero tracks and admits its first), so only the attach path calls it today. + private static string? CheckCardinality(ReleaseDto release) + { + var cardinality = MediumRules.CardinalityOf(release.Medium); + if (release.TrackCount + 1 > cardinality.Max) + { + return $"{CardinalityViolationMarker}A {release.Medium} release holds a single track; " + + $"'{release.Title}' already has one — edit the existing track or choose a different release."; + } + + return null; + } + /// /// Replace an existing track's audio in place: look up the SQL row, swap only the vault bytes /// keyed by its EntryKey, regenerate both waveform datums from the new audio, then write the diff --git a/DeepDrftManager/Components/Pages/Tracks/BatchEdit.razor b/DeepDrftManager/Components/Pages/Tracks/BatchEdit.razor index fc4b7a8..6764a8c 100644 --- a/DeepDrftManager/Components/Pages/Tracks/BatchEdit.razor +++ b/DeepDrftManager/Components/Pages/Tracks/BatchEdit.razor @@ -146,6 +146,9 @@ private string _releaseDate = string.Empty; private ReleaseType _releaseType = ReleaseType.Single; private ReleaseMedium _medium = ReleaseMedium.Cut; + // The id of the release being edited. New tracks added in this session attach to it via the upload + // service's releaseId (ATTACH) path, so they are not rejected as a pre-existing-(title,artist) duplicate. + private long? _releaseId; // The medium selector drives ReleaseType visibility and is persisted on save: every UpdateAsync / // UploadTrackAsync call below passes _medium, and PUT api/track/meta resets ReleaseType to its @@ -214,6 +217,10 @@ } var release = tracks[0].Release; + // The release being edited already exists, so any new track added here ATTACHES to it (the upload + // service's releaseId path) rather than taking the CREATE path, which would reject it as a + // duplicate (title, artist). Fall back to the track's own ReleaseId if the nav is not populated. + _releaseId = release?.Id ?? tracks[0].ReleaseId; _albumName = albumName; _artist = release?.Artist ?? string.Empty; _genre = release?.Genre ?? string.Empty; @@ -592,6 +599,7 @@ _releaseType, trackNumber, _medium, + _releaseId, progress); if (!uploadResult.Success || uploadResult.Value is null) diff --git a/DeepDrftManager/Components/Pages/Tracks/BatchUpload.razor b/DeepDrftManager/Components/Pages/Tracks/BatchUpload.razor index b3d9092..524d089 100644 --- a/DeepDrftManager/Components/Pages/Tracks/BatchUpload.razor +++ b/DeepDrftManager/Components/Pages/Tracks/BatchUpload.razor @@ -298,6 +298,29 @@ return; } + // Pre-flight duplicate guard (primary block): the upload form creates new releases only, so a + // (title, artist) that already exists in the catalogue is refused BEFORE any bytes transfer — + // the admin is not surprised at the end of a long upload. The server backstops this on the + // create path, but checking here keeps the failure fast and visible. The values passed match + // exactly what the upload sends (untrimmed _albumName/_artist) so the pre-flight and the server + // agree on the match. A check failure (API unreachable) blocks rather than proceeding blind. + var duplicateCheck = await CmsTrackService.GetExistingReleaseAsync(_albumName, _artist); + if (!duplicateCheck.Success) + { + var checkError = duplicateCheck.Messages.FirstOrDefault()?.Message ?? "Unknown error"; + _errorMessage = $"Could not verify the release name: {checkError}"; + Snackbar.Add(_errorMessage, Severity.Error); + return; + } + + if (duplicateCheck.Value is { } existing) + { + _errorMessage = $"A release titled '{existing.Title}' by {existing.Artist} already exists. " + + "The upload form creates new releases only — use the edit tools to change an existing one."; + Snackbar.Add(_errorMessage, Severity.Error); + return; + } + // For single-track media (Session/Mix) the track name is derived from the Release Name — // no separate Track Name input is shown. Sync here so the stored name always matches. if (MediumRules.CardinalityOf(_medium).IsSingleTrack && _tracks.Count > 0) @@ -327,6 +350,11 @@ } int succeeded = 0, failed = 0; + // Within-batch attach: row 1 creates the release (no releaseId → CREATE path); once it + // succeeds we carry its ReleaseId into rows 2..N so they ATTACH to the just-created release + // rather than tripping the server's pre-existing-duplicate block. Only a multi-track Cut + // reaches row 2 (single-track media collapse to one row). + long? batchReleaseId = null; for (int i = 0; i < _tracks.Count; i++) { var row = _tracks[i]; @@ -375,6 +403,7 @@ _releaseType, trackNumber, _medium, + batchReleaseId, progress); if (!result.Success || result.Value is null) @@ -387,6 +416,15 @@ } else { + // Capture the release id created by the first successful row so subsequent rows + // attach to it (the within-batch multi-track Cut path). Only set once — later + // rows must not overwrite it. A null ReleaseId here (loose track) leaves it null, + // which is correct: a release-less upload has no within-batch release to attach to. + if (batchReleaseId is null && result.Value.ReleaseId is { } createdReleaseId) + { + batchReleaseId = createdReleaseId; + } + // The upload endpoint does not accept an imagePath, so link the cover art with // a follow-up metadata update — same two-step pattern BatchEdit uses. if (_imagePath is { } imgPath) @@ -487,7 +525,13 @@ } else { - Snackbar.Add($"Uploaded {succeeded} track(s); {failed} failed — review errors below.", Severity.Warning); + // Surface the actual reason, not just counts — a server rejection (duplicate, cardinality) + // relays a human-readable message via row.ErrorMessage. Show the first failure's reason so + // the admin sees WHY without scanning the rows; the per-row errors remain as detail. + var firstError = _tracks.FirstOrDefault(t => t.Status == BatchRowStatus.Failed)?.ErrorMessage; + var reason = string.IsNullOrWhiteSpace(firstError) ? "review errors below" : firstError; + _errorMessage = succeeded == 0 ? reason : $"{succeeded} uploaded; {failed} failed: {reason}"; + Snackbar.Add($"Uploaded {succeeded} track(s); {failed} failed — {reason}", Severity.Warning); // Stay on page so the admin can see the failed rows. } } diff --git a/DeepDrftManager/Services/CmsTrackService.cs b/DeepDrftManager/Services/CmsTrackService.cs index 80f54c3..663312e 100644 --- a/DeepDrftManager/Services/CmsTrackService.cs +++ b/DeepDrftManager/Services/CmsTrackService.cs @@ -68,6 +68,7 @@ public class CmsTrackService : ICmsTrackService ReleaseType releaseType, int trackNumber, ReleaseMedium medium = ReleaseMedium.Cut, + long? releaseId = null, IProgress? progress = null, CancellationToken ct = default) { @@ -91,6 +92,9 @@ public class CmsTrackService : ICmsTrackService // The upload endpoint binds "medium" to the created release's ReleaseMedium (defaulting to Cut // for an unrecognised value). Authoritative only when this upload creates the release. multipart.Add(new StringContent(medium.ToString()), "medium"); + // releaseId present → ATTACH (rows 2..N of a within-batch Cut); absent → CREATE (server rejects a + // pre-existing (title, artist) as a duplicate). Only sent when set so the form omits it on row 1. + if (releaseId is { } rid) multipart.Add(new StringContent(rid.ToString()), "releaseId"); var send = await phase.SendAsync(UploadPath, multipart, $"upload of {trackName}"); if (send.Response is not { } response) @@ -474,6 +478,53 @@ public class CmsTrackService : ICmsTrackService } } + public async Task> GetExistingReleaseAsync( + string title, string artist, CancellationToken ct = default) + { + var client = _httpClientFactory.CreateClient(ContentCmsClientName); + var query = $"api/track/release/exists?title={Uri.EscapeDataString(title)}&artist={Uri.EscapeDataString(artist)}"; + + HttpResponseMessage response; + try + { + response = await client.GetAsync(query, ct); + } + catch (Exception ex) + { + _logger.LogError(ex, "Content API call failed for release existence check ({Title}, {Artist})", title, artist); + return ResultContainer.CreateFailResult("Content API is unreachable."); + } + + using (response) + { + // 404 is the not-found (null) case, not a failure — no release matches this (title, artist). + if (response.StatusCode == HttpStatusCode.NotFound) + { + return ResultContainer.CreatePassResult(null); + } + + if (!response.IsSuccessStatusCode) + { + _logger.LogError("Content API release existence check failed for ({Title}, {Artist}): {Status}", + title, artist, (int)response.StatusCode); + return ResultContainer.CreateFailResult("Failed to check for an existing release."); + } + + ReleaseDto? release; + try + { + release = await response.Content.ReadFromJsonAsync(ct); + } + catch (Exception ex) + { + _logger.LogError(ex, "Failed to deserialize ReleaseDto from release existence check"); + return ResultContainer.CreateFailResult("Content API returned an unexpected response."); + } + + return ResultContainer.CreatePassResult(release); + } + } + private static readonly HashSet KnownImageMimeTypes = new(StringComparer.OrdinalIgnoreCase) { "image/jpeg", "image/png", "image/gif", "image/webp", "image/svg+xml", "image/bmp" diff --git a/DeepDrftManager/Services/ICmsTrackService.cs b/DeepDrftManager/Services/ICmsTrackService.cs index 6c49d48..3796c51 100644 --- a/DeepDrftManager/Services/ICmsTrackService.cs +++ b/DeepDrftManager/Services/ICmsTrackService.cs @@ -25,6 +25,10 @@ public interface ICmsTrackService /// sets Content-Length and is the denominator for , which reports cumulative /// bytes pushed to the wire. Each progress tick also resets the idle/heartbeat upload timeout, so a /// stalled connection aborts without a fixed total-duration cap. + /// distinguishes the two rows of a within-batch multi-track Cut: null on + /// the first row (CREATE — the server rejects a pre-existing (title, artist) as a duplicate) and the + /// id returned by that first row on rows 2..N (ATTACH — the server skips the duplicate check and adds + /// the track to the release the batch just created). /// Task> UploadTrackAsync( Stream wavStream, @@ -42,9 +46,20 @@ public interface ICmsTrackService ReleaseType releaseType, int trackNumber, ReleaseMedium medium = ReleaseMedium.Cut, + long? releaseId = null, IProgress? progress = null, CancellationToken ct = default); + /// + /// Upload-form pre-flight: returns the existing release whose exact (title, artist) matches, or null + /// when none exists. Backs the duplicate block the form runs BEFORE transferring bytes, so the admin + /// is not surprised at the end of a long upload. A 404 from the API is the not-found (null) case, not + /// a failure. The match semantics are the API's GetReleaseByTitleAndArtist — the same read the + /// server backstop uses — so the pre-flight and the backstop agree. + /// + Task> GetExistingReleaseAsync( + string title, string artist, CancellationToken ct = default); + /// /// Delete a track via the Content API, which removes the SQL row then the vault entry. /// Maps a 404 to a "Track not found." failure. diff --git a/DeepDrftTests/DeepDrftTests.csproj b/DeepDrftTests/DeepDrftTests.csproj index b8478d1..b160638 100644 --- a/DeepDrftTests/DeepDrftTests.csproj +++ b/DeepDrftTests/DeepDrftTests.csproj @@ -28,6 +28,9 @@ + +