feat(song): add SongExternalLink CRUD with details endpoint - #66
Merged
Conversation
📝 WalkthroughWalkthroughAdds song external-link data contracts, Prisma persistence, transactional service operations with outbox events, authenticated CRUD routes, song-scoped lyric handling, and backend runtime updates. ChangesSong external links
Backend runtime updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant songExternalLinkHandler
participant SongService
participant SongRepository
participant Prisma
participant OutboxQueue
Client->>songExternalLinkHandler: Submit external-link CRUD request
songExternalLinkHandler->>SongService: Execute validated operation
SongService->>SongRepository: Check song or link
SongService->>Prisma: Commit mutation and outbox entry
Prisma-->>SongService: Return committed result
SongService->>OutboxQueue: Enqueue outbox entry
SongService-->>songExternalLinkHandler: Return operation result
songExternalLinkHandler-->>Client: Return HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/db/prisma/models/core/externalLink.prisma (1)
1-15: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider adding an index on the
songIdforeign key.Fetching a song's details will likely retrieve its associated external links, making
songIda frequent query filter. Depending on your underlying database (like PostgreSQL), Prisma does not automatically create indexes for relation scalars. Adding an explicit index onsongIdprevents potential full table scans as the table grows.⚡ Proposed performance optimization
deletedAt DateTime? `@map`("deleted_at") @@map("song_external_link") + @@index([songId]) @@schema("core") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/prisma/models/core/externalLink.prisma` around lines 1 - 15, Add an explicit Prisma index for the songId relation scalar in the SongExternalLink model, preserving the existing field mapping and schema definitions so queries filtering external links by song remain indexed.apps/backend/src/handlers/catalog/song/externalLink/create.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer named exports for Zod imports.
Although importing Zod as a default export (
import z from "zod") functions correctly ifesModuleInteropis enabled, Zod's official documentation and standard usage advocate for the named export. Transitioning to the named export ensures robust ESM compatibility and averts potential module resolution edge cases.
apps/backend/src/handlers/catalog/song/externalLink/create.ts#L10-L10: Change toimport { z } from "zod";.apps/backend/src/handlers/catalog/song/externalLink/update.ts#L10-L10: Change toimport { z } from "zod";.apps/backend/src/handlers/catalog/song/externalLink/delete.ts#L5-L5: Change toimport { z } from "zod";.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/catalog/song/externalLink/create.ts` at line 10, Replace the default Zod import with the named z export in apps/backend/src/handlers/catalog/song/externalLink/create.ts:10, apps/backend/src/handlers/catalog/song/externalLink/update.ts:10, and apps/backend/src/handlers/catalog/song/externalLink/delete.ts:5; no other changes are needed.packages/core/src/modules/catalog/song/repository.ts (1)
220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOmit
songIdto exactly match the DTO shape.
SongExternalLinkResponseDtospecifically omitssongId. While the current code is functionally safe, consider addingsongId: trueto theomitblocks here (and equivalently ingetExternalLinkByIdandupdateExternalLink) so the returned payload strictly matches the DTO interface without excess properties.♻️ Proposed refactor
omit: { deletedAt: true, + songId: true, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/modules/catalog/song/repository.ts` around lines 220 - 222, Update the external-link repository query omit blocks to include songId alongside deletedAt, including the corresponding blocks in getExternalLinkById and updateExternalLink, so returned payloads exactly match SongExternalLinkResponseDto.packages/core/src/modules/catalog/song/dto.ts (2)
82-82: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider stricter URL validation.
Similarly, consider using
.url()here for consistency if the URLs should be strictly validated.♻️ Proposed refactor
- url: z.string().min(1).optional(), + url: z.string().url().optional(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/modules/catalog/song/dto.ts` at line 82, Update the URL schema field in the song DTO to use Zod’s strict URL validation while preserving its optional behavior, replacing the current non-empty string validation on the url field.
75-75: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider stricter URL validation.
If all external links are expected to be valid standard URLs, consider replacing
.min(1)with.url()to ensure strict validation.♻️ Proposed refactor
- url: z.string().min(1), + url: z.string().url(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/modules/catalog/song/dto.ts` at line 75, Update the url field validation in the song DTO schema to use Zod’s strict URL validation instead of only requiring a non-empty string. Preserve the existing field and schema structure while ensuring external links must be valid standard URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/modules/catalog/song/service.ts`:
- Around line 233-236: The external-link lookup must enforce ownership by song.
In packages/core/src/modules/catalog/song/service.ts lines 233-236 and 260-263,
pass id to getExternalLinkById; update
packages/core/src/modules/catalog/song/repository.ts lines 227-236 to accept
songId and query by id, songId, and deletedAt: null; update
packages/core/src/modules/catalog/song/repository.interface.ts lines 46-49 to
match the new signature. Do not modify the noted lyric methods unless separately
requested.
---
Nitpick comments:
In `@apps/backend/src/handlers/catalog/song/externalLink/create.ts`:
- Line 10: Replace the default Zod import with the named z export in
apps/backend/src/handlers/catalog/song/externalLink/create.ts:10,
apps/backend/src/handlers/catalog/song/externalLink/update.ts:10, and
apps/backend/src/handlers/catalog/song/externalLink/delete.ts:5; no other
changes are needed.
In `@packages/core/src/modules/catalog/song/dto.ts`:
- Line 82: Update the URL schema field in the song DTO to use Zod’s strict URL
validation while preserving its optional behavior, replacing the current
non-empty string validation on the url field.
- Line 75: Update the url field validation in the song DTO schema to use Zod’s
strict URL validation instead of only requiring a non-empty string. Preserve the
existing field and schema structure while ensuring external links must be valid
standard URLs.
In `@packages/core/src/modules/catalog/song/repository.ts`:
- Around line 220-222: Update the external-link repository query omit blocks to
include songId alongside deletedAt, including the corresponding blocks in
getExternalLinkById and updateExternalLink, so returned payloads exactly match
SongExternalLinkResponseDto.
In `@packages/db/prisma/models/core/externalLink.prisma`:
- Around line 1-15: Add an explicit Prisma index for the songId relation scalar
in the SongExternalLink model, preserving the existing field mapping and schema
definitions so queries filtering external links by song remain indexed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a7b8e5e2-27b0-46d6-bc96-15b6c0a027e2
📒 Files selected for processing (11)
apps/backend/src/handlers/catalog/song/externalLink/create.tsapps/backend/src/handlers/catalog/song/externalLink/delete.tsapps/backend/src/handlers/catalog/song/externalLink/index.tsapps/backend/src/handlers/catalog/song/externalLink/update.tsapps/backend/src/handlers/catalog/song/index.tspackages/core/src/modules/catalog/song/dto.tspackages/core/src/modules/catalog/song/repository.interface.tspackages/core/src/modules/catalog/song/repository.tspackages/core/src/modules/catalog/song/service.tspackages/db/prisma/models/core/externalLink.prismapackages/db/prisma/models/core/song.prisma
alikia2x
force-pushed
the
feat/yuhua-song-external-link
branch
from
July 15, 2026 09:02
120352a to
6b84973
Compare
…tests - Refactor i18n setup from @fanee/runtime-node to @fanee/core + @fanee/node - Update i18n API calls (t -> getT) in error response handler - Add unit tests for SongService external link CRUD (create, update, delete) - Pin bullmq and ioredis dependency versions - Update bun.lock with latest dependency resolutions
alikia2x
force-pushed
the
feat/yuhua-song-external-link
branch
from
July 15, 2026 09:05
6b84973 to
1fe2b5b
Compare
Add songId parameter to getLyricById, updateLyric, softDeleteLyric, getExternalLinkById, updateExternalLink, and softDeleteExternalLink to prevent IDOR (CWE-639) where an attacker could modify/delete lyrics or external links belonging to a different song by supplying an arbitrary child ID. All repository where clauses now include the parent songId, and the service layer passes it through consistently.
alikia2x
approved these changes
Jul 15, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/modules/catalog/song/repository.ts (1)
225-227: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOmit
songIdfrom standalone external-link responses.
SongExternalLinkResponseDtoand the test fixture excludesongId, but these Prisma results retain it at runtime.
packages/core/src/modules/catalog/song/repository.ts#L225-L227: addsongId: trueto the create omission.packages/core/src/modules/catalog/song/repository.ts#L238-L238: omitsongIdfrom the fetched link.packages/core/src/modules/catalog/song/repository.ts#L255-L255: omitsongIdfrom the updated link.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/modules/catalog/song/repository.ts` around lines 225 - 227, Omit songId from all standalone external-link responses by updating the create omission near lines 225-227, the fetched-link omission near line 238, and the updated-link omission near line 255 in packages/core/src/modules/catalog/song/repository.ts. Apply the omission consistently in each repository operation so results match SongExternalLinkResponseDto and its fixture.
🧹 Nitpick comments (1)
packages/core/tests/unit/SongService.test.ts (1)
418-432: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winIDOR (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External
Assert the ownership-scoped lookup arguments.
These tests verify the write call but not the security-critical lookup that prevents cross-song link mutation.
packages/core/tests/unit/SongService.test.ts#L418-L432: assertgetExternalLinkById(1, 1).packages/core/tests/unit/SongService.test.ts#L455-L464: assertgetExternalLinkById(1, 1).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/unit/SongService.test.ts` around lines 418 - 432, Assert that both affected tests in packages/core/tests/unit/SongService.test.ts (418-432 and 455-464) verify mockRepository.getExternalLinkById was called with the ownership-scoped arguments (1, 1), while preserving their existing updateExternalLink result and write-call assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/modules/catalog/song/repository.ts`:
- Around line 193-208: Restrict the lyric mutations in updateLyric and
softDeleteLyric to active rows by adding deletedAt: null to each where predicate
alongside the existing id and songId filters. Apply this change at
packages/core/src/modules/catalog/song/repository.ts lines 193-208 and 251-266;
both sites require the same predicate update.
---
Outside diff comments:
In `@packages/core/src/modules/catalog/song/repository.ts`:
- Around line 225-227: Omit songId from all standalone external-link responses
by updating the create omission near lines 225-227, the fetched-link omission
near line 238, and the updated-link omission near line 255 in
packages/core/src/modules/catalog/song/repository.ts. Apply the omission
consistently in each repository operation so results match
SongExternalLinkResponseDto and its fixture.
---
Nitpick comments:
In `@packages/core/tests/unit/SongService.test.ts`:
- Around line 418-432: Assert that both affected tests in
packages/core/tests/unit/SongService.test.ts (418-432 and 455-464) verify
mockRepository.getExternalLinkById was called with the ownership-scoped
arguments (1, 1), while preserving their existing updateExternalLink result and
write-call assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 82bf5c8b-785d-4e36-9dcb-9047f643441d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
apps/backend/package.jsonapps/backend/src/common/error/getErrorResponse.tsapps/backend/src/common/i18n.tsapps/backend/src/middlewares/auth.tspackages/core/package.jsonpackages/core/src/modules/catalog/song/dto.tspackages/core/src/modules/catalog/song/repository.interface.tspackages/core/src/modules/catalog/song/repository.tspackages/core/src/modules/catalog/song/service.tspackages/core/src/outbox/queue.tspackages/core/tests/integration/SongRepository.test.tspackages/core/tests/unit/SongSearchService.test.tspackages/core/tests/unit/SongService.test.ts
💤 Files with no reviewable changes (1)
- apps/backend/src/middlewares/auth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/modules/catalog/song/dto.ts
Comment on lines
193
to
+208
| return this.query("db.song.updateLyric", () => | ||
| client.lyrics.update({ | ||
| where: { id: lyricId }, | ||
| where: { id: lyricId, songId }, | ||
| data: input, | ||
| omit: { deletedAt: true, songId: true }, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| async softDeleteLyric(lyricId: number, tx?: TxClient) { | ||
| async softDeleteLyric(songId: SongId, lyricId: number, tx?: TxClient) { | ||
| const client = tx ?? this.prisma; | ||
|
|
||
| await this.query("db.song.softDeleteLyric", () => | ||
| client.lyrics.update({ | ||
| where: { id: lyricId }, | ||
| where: { id: lyricId, songId }, | ||
| data: { deletedAt: new Date() }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant file structure and inspect the cited repository file.
git ls-files 'packages/core/src/modules/catalog/song/*' 'packages/core/src/**/repository.ts' | sed -n '1,200p'
printf '\n--- repository outline ---\n'
ast-grep outline packages/core/src/modules/catalog/song/repository.ts --view expanded || true
printf '\n--- relevant snippets ---\n'
sed -n '150,320p' packages/core/src/modules/catalog/song/repository.ts
printf '\n--- search for deletedAt predicates in song repository and nearby files ---\n'
rg -n "deletedAt:\s*null|deletedAt" packages/core/src/modules/catalog/song packages/core/src/modules/catalog -g '!**/dist/**' | sed -n '1,220p'Repository: project-cvsa/cvsa
Length of output: 13233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- files mentioning lyrics / songExternalLink models ---\n'
rg -n "model (Lyric|SongExternalLink)|lyrics.update|songExternalLink.update|deletedAt" packages/core -g '*.prisma' -g '*.ts' | sed -n '1,240p'
printf '\n--- prisma schema candidates ---\n'
fd -a -e prisma packages/core | sed -n '1,80p'
printf '\n--- repository file around cited ranges ---\n'
sed -n '180,285p' packages/core/src/modules/catalog/song/repository.tsRepository: project-cvsa/cvsa
Length of output: 9159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- song service methods using lyric/external link mutations ---\n'
rg -n "updateLyric|softDeleteLyric|updateExternalLink|softDeleteExternalLink|getLyricById|getExternalLinkById" \
packages/core/src/modules/catalog/song -g '*.ts' | sed -n '1,220p'
printf '\n--- song integration tests around lyric/external-link mutations ---\n'
sed -n '150,260p' packages/core/tests/integration/SongRepository.test.ts
printf '\n--- song service outline ---\n'
ast-grep outline packages/core/src/modules/catalog/song/service.ts --view expanded || trueRepository: project-cvsa/cvsa
Length of output: 7609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- song service methods around lyric/external-link update/delete ---\n'
sed -n '130,285p' packages/core/src/modules/catalog/song/service.ts
printf '\n--- repository interface around relevant methods ---\n'
sed -n '30,75p' packages/core/src/modules/catalog/song/repository.interface.tsRepository: project-cvsa/cvsa
Length of output: 5351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,285p' packages/core/src/modules/catalog/song/service.tsRepository: project-cvsa/cvsa
Length of output: 4144
Restrict these mutations to active rows. update/softDelete still match tombstoned lyrics and external links, so a stale or concurrent request can update a deleted row or emit a duplicate delete event. Add deletedAt: null to both predicates.
packages/core/src/modules/catalog/song/repository.ts#L185-L208packages/core/src/modules/catalog/song/repository.ts#L243-L266
📍 Affects 1 file
packages/core/src/modules/catalog/song/repository.ts#L193-L208(this comment)packages/core/src/modules/catalog/song/repository.ts#L251-L266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/modules/catalog/song/repository.ts` around lines 193 - 208,
Restrict the lyric mutations in updateLyric and softDeleteLyric to active rows
by adding deletedAt: null to each where predicate alongside the existing id and
songId filters. Apply this change at
packages/core/src/modules/catalog/song/repository.ts lines 193-208 and 251-266;
both sites require the same predicate update.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ExternalLinktoSongExternalLinkand updated song relations and table mapping.