feat(song): add song lyrics CRUD endpoints#46
Conversation
Implement full CRUD operations for song lyrics: - GET /v2/song/:id/lyrics - List all lyrics for a song - GET /v2/song/:id/lyric/:lyricId - Get specific lyric - POST /v2/song/:id/lyric - Create lyric (auth required) - PATCH /v2/song/:id/lyric/:lyricId - Update lyric (auth required) - DELETE /v2/song/:id/lyric/:lyricId - Delete lyric (auth required) Includes repository methods, service layer, DTOs, E2E tests, and i18n translations.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds full song-lyrics CRUD: new backend Elysia handlers (list/get/create/update/delete), DTOs/schemas and repository/service methods for lyrics, response-schema adjustments for songs, localization keys, and unit + E2E tests covering new endpoints and soft-delete behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as Elysia Handler
participant Service as SongService
participant Repo as SongRepository
participant DB as Database
Client->>Handler: POST /v2/song/:id/lyric (auth, body)
Handler->>Service: createLyric(songId, body)
Service->>Repo: createLyrics(songId, dto)
Repo->>DB: INSERT lyric row (deletedAt null)
DB-->>Repo: created record
Repo-->>Service: SongLyricsResponseDto (omit deletedAt/songId)
Service-->>Handler: created lyric DTO
Handler-->>Client: 201 Created + body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
packages/core/src/modules/catalog/song/dto.ts (1)
26-40: Consider extracting base lyric fields to reduce duplication.
SongLyricsCreateRequestSchemaandSongLyricsUpdateRequestSchemahave identical field definitions. While this is acceptable for separate create/update DTOs, you could DRY this up if both schemas will always remain identical.♻️ Optional: Extract common fields
+const BaseLyricsFieldsSchema = z.object({ + language: z.string().optional(), + isTranslated: z.boolean().optional(), + plainText: z.string().optional(), + ttml: z.string().optional(), + lrc: z.string().optional(), +}); + -export const SongLyricsCreateRequestSchema = z.object({ - language: z.string().optional(), - isTranslated: z.boolean().optional(), - plainText: z.string().optional(), - ttml: z.string().optional(), - lrc: z.string().optional(), -}); - -export const SongLyricsUpdateRequestSchema = z.object({ - language: z.string().optional(), - isTranslated: z.boolean().optional(), - plainText: z.string().optional(), - ttml: z.string().optional(), - lrc: z.string().optional(), -}); +export const SongLyricsCreateRequestSchema = BaseLyricsFieldsSchema; +export const SongLyricsUpdateRequestSchema = BaseLyricsFieldsSchema;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/song/dto.ts` around lines 26 - 40, The two schemas SongLyricsCreateRequestSchema and SongLyricsUpdateRequestSchema are identical; extract the common object into a single SongLyricsBaseSchema (or SongLyricsFieldsSchema) that contains language, isTranslated, plainText, ttml, and lrc, then reuse it for both exports (e.g., export const SongLyricsCreateRequestSchema = SongLyricsBaseSchema and export const SongLyricsUpdateRequestSchema = SongLyricsBaseSchema or .partial()/.extend() if you later need variation). Update the export names accordingly so callers keep the same symbols.apps/backend/tests/songLyrics.test.ts (1)
120-165: Consider adding a test for cross-song lyric access.The tests verify 404 for nonexistent songs and lyrics, but don't cover the case where a valid
lyricIdbelongs to a different song. Once the ownership verification fix inservice.tsis applied, add a test to ensure accessingsong A's lyric viasong B's endpoint returns 404.📝 Suggested test case
test("should return 404 when lyric belongs to different song", async () => { const songA = await prisma.song.create({ data: { name: "Song A", type: "ORIGINAL", lyrics: { create: [{ language: "en", plainText: "Lyrics for A" }], }, }, }); const songB = await prisma.song.create({ data: { name: "Song B", type: "ORIGINAL" }, }); const lyricA = await prisma.lyrics.findFirst({ where: { songId: songA.id }, }); expect(lyricA).not.toBeNull(); if (!lyricA) return; // Try to access songA's lyric via songB's endpoint const { error, status } = await api.v2 .song({ id: songB.id }) .lyric({ lyricId: lyricA.id }) .get(); expect(status).toBe(404); expect(error?.value).toMatchObject({ code: "NOT_FOUND" }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/tests/songLyrics.test.ts` around lines 120 - 165, Add a test that ensures a lyric cannot be fetched through a different song's endpoint: create songA with a lyric (use prisma.song.create and prisma.lyrics.findFirst to get the lyric id), create songB, then call api.v2.song({ id: songB.id }).lyric({ lyricId: <lyricFromA>.id }).get() and assert the response is 404 with error.code "NOT_FOUND"; place this alongside the existing tests in songLyrics.test.ts so it verifies cross-song ownership enforcement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 16: Update the typo in AGENTS.md by replacing the incorrect package path
string "pakcages/observability" with the correct "packages/observability" so it
matches the repository's package naming convention and other package references.
In `@apps/backend/src/handlers/catalog/song/lyrics/create.ts`:
- Around line 34-36: The inline params zod schema inside the create.ts handler
(the params object with id: z.coerce.number().int().positive()) must be moved
into the core DTOs and reused: add and export a named schema (e.g.,
SongParamsSchema) from the module-level DTO file, import that schema into the
create.ts handler and replace the inline params with the imported
SongParamsSchema, and update any associated type uses (z.infer or typings) to
reference the exported schema to keep schemas centralized and consistent.
In `@apps/backend/src/handlers/catalog/song/lyrics/delete.ts`:
- Line 24: The handler currently uses inline Zod schemas (z.null() response and
the inline params object) — extract these into shared DTO exports in
packages/core/src/modules/catalog/song/dto.ts (e.g., export const
DeleteLyricsResponseSchema = z.null(); export const DeleteLyricsParamsSchema =
z.object({...}) with the same shape), import those schemas into
apps/backend/src/handlers/catalog/song/lyrics/delete.ts, and replace the inline
z.null() and inline params object with the imported DeleteLyricsResponseSchema
and DeleteLyricsParamsSchema so the handler references the centralized DTOs.
In `@apps/backend/src/handlers/catalog/song/lyrics/get.ts`:
- Around line 24-27: Replace the inline Zod schema in the handler's params with
the shared schema exported from the core DTO; specifically, in
apps/backend/src/handlers/catalog/song/lyrics/get.ts remove the inline
z.object(...) used in the params and import SongLyricPathParamsSchema from
packages/core/src/modules/catalog/song/dto.ts, then use
SongLyricPathParamsSchema in the params field (also add/adjust the import
statement). Ensure the symbol name SongLyricPathParamsSchema is used exactly to
match the exported DTO.
In `@apps/backend/src/handlers/catalog/song/lyrics/list.ts`:
- Around line 24-26: Replace the inline params zod schema (params: z.object({
id: z.coerce.number().int().positive() })) with the shared schema exported from
`@cvsa/core`: create/export a SongParamsDto (or songParamsSchema) in the core song
DTO file (packages/core/modules/song/dto.ts) that defines the id
coercion/validation, then import that symbol from '@cvsa/core' into the handler
and use it in place of the inline params; ensure the handler's params reference
name matches the exported DTO name and remove the inline z.object block.
In `@apps/backend/src/handlers/catalog/song/lyrics/update.ts`:
- Around line 34-37: Replace the inline params z.object({ id:
z.coerce.number().int().positive(), lyricId: z.coerce.number().int().positive()
}) in the update handler with an imported shared schema from `@cvsa/core` (e.g.,
export a SongLyricParamsSchema or songLyricParamsDto from the core module DTO
and import it into this handler). If the shared DTO does not exist yet, add it
to packages/core/modules/<module>/dto.ts as a zod schema that validates id and
lyricId, export it, then update the handler to use the imported schema
(referencing the existing params, id and lyricId symbols) instead of defining
the schema inline.
In `@packages/core/src/modules/catalog/song/dto.ts`:
- Line 96: The SongResponseDto type uses the incorrect capitalized Zod helper;
change the usage of z.Infer to the correct lowercase z.infer so SongResponseDto
= Serialized<z.infer<typeof SongResponseSchema>> (keep Serialized and
SongResponseSchema as-is) to match the other schema-derived types and coding
guidelines.
In `@packages/core/src/modules/catalog/song/repository.interface.ts`:
- Around line 16-23: The implementation methods getById, create, and update are
returning Prisma-serialized Song objects (which include deletedAt) while the
interface expects SongResponseDto without deletedAt; update the method
signatures to explicitly annotate their return types to match the interface
(async getById(...): Promise<SongResponseDto | null>, async create(...):
Promise<SongResponseDto>, async update(...): Promise<SongResponseDto>) and
ensure the returned value conforms by either using transformPrismaResult(...)
and then removing deletedAt or by changing the Prisma queries to omit({
deletedAt: true }) (mirroring the lyrics methods), so the runtime result matches
the declared SongResponseDto type.
In `@packages/core/src/modules/catalog/song/service.ts`:
- Around line 88-100: The service methods getLyric, updateLyric, and deleteLyric
currently fetch a lyric by lyricId only (via repository.getLyricById) and do not
verify it belongs to the provided song id, allowing cross-song access; fix by
enforcing ownership: either change the repository signature to
getLyricById(lyricId, songId) and update calls in
getLyric/updateLyric/deleteLyric to pass the song id, or after fetching the
lyric in those service methods check lyric.songId === id and throw
AppError("error.lyric.notfound", "NOT_FOUND", 404) if it doesn't match; update
all three methods to perform this ownership check using the
repository.getLyricById symbol or the lyric.songId check.
---
Nitpick comments:
In `@apps/backend/tests/songLyrics.test.ts`:
- Around line 120-165: Add a test that ensures a lyric cannot be fetched through
a different song's endpoint: create songA with a lyric (use prisma.song.create
and prisma.lyrics.findFirst to get the lyric id), create songB, then call
api.v2.song({ id: songB.id }).lyric({ lyricId: <lyricFromA>.id }).get() and
assert the response is 404 with error.code "NOT_FOUND"; place this alongside the
existing tests in songLyrics.test.ts so it verifies cross-song ownership
enforcement.
In `@packages/core/src/modules/catalog/song/dto.ts`:
- Around line 26-40: The two schemas SongLyricsCreateRequestSchema and
SongLyricsUpdateRequestSchema are identical; extract the common object into a
single SongLyricsBaseSchema (or SongLyricsFieldsSchema) that contains language,
isTranslated, plainText, ttml, and lrc, then reuse it for both exports (e.g.,
export const SongLyricsCreateRequestSchema = SongLyricsBaseSchema and export
const SongLyricsUpdateRequestSchema = SongLyricsBaseSchema or
.partial()/.extend() if you later need variation). Update the export names
accordingly so callers keep the same symbols.
🪄 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: ec91718d-a761-4c10-9e3c-607c0cf4cc2a
📒 Files selected for processing (21)
AGENTS.mdapps/backend/src/errorHandler.tsapps/backend/src/handlers/catalog/song/create.tsapps/backend/src/handlers/catalog/song/index.tsapps/backend/src/handlers/catalog/song/lyrics/create.tsapps/backend/src/handlers/catalog/song/lyrics/delete.tsapps/backend/src/handlers/catalog/song/lyrics/get.tsapps/backend/src/handlers/catalog/song/lyrics/index.tsapps/backend/src/handlers/catalog/song/lyrics/list.tsapps/backend/src/handlers/catalog/song/lyrics/update.tsapps/backend/src/handlers/catalog/song/update.tsapps/backend/tests/auth.test.tsapps/backend/tests/songLyrics.test.tslocale/modules/backend/messages/en.jsonlocale/modules/backend/messages/zh.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/tests/unit/SongSearchService.test.tspackages/core/tests/unit/SongService.test.ts
💤 Files with no reviewable changes (1)
- apps/backend/src/errorHandler.ts
| params: z.object({ | ||
| id: z.coerce.number().int().positive(), | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use a shared params schema from core DTOs.
The inline params schema at Line 34-36 should be defined in packages/core/src/modules/catalog/song/dto.ts and imported here.
As per coding guidelines: apps/backend/src/handlers/**/*.ts: Define all Zod schemas in packages/core/modules/<module>/dto.ts first; do not define them inline inside handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/handlers/catalog/song/lyrics/create.ts` around lines 34 -
36, The inline params zod schema inside the create.ts handler (the params object
with id: z.coerce.number().int().positive()) must be moved into the core DTOs
and reused: add and export a named schema (e.g., SongParamsSchema) from the
module-level DTO file, import that schema into the create.ts handler and replace
the inline params with the imported SongParamsSchema, and update any associated
type uses (z.infer or typings) to reference the exported schema to keep schemas
centralized and consistent.
| description: "Delete a specific lyrics item for a song. Requires authentication.", | ||
| }, | ||
| response: { | ||
| 204: z.null(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid inline Zod schemas in this handler config.
z.null() at Line 24 and the inline params object at Line 29-32 should be moved to shared schemas in packages/core/src/modules/catalog/song/dto.ts and imported here.
As per coding guidelines: apps/backend/src/handlers/**/*.ts: Define all Zod schemas in packages/core/modules/<module>/dto.ts first; do not define them inline inside handlers.
Also applies to: 29-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/handlers/catalog/song/lyrics/delete.ts` at line 24, The
handler currently uses inline Zod schemas (z.null() response and the inline
params object) — extract these into shared DTO exports in
packages/core/src/modules/catalog/song/dto.ts (e.g., export const
DeleteLyricsResponseSchema = z.null(); export const DeleteLyricsParamsSchema =
z.object({...}) with the same shape), import those schemas into
apps/backend/src/handlers/catalog/song/lyrics/delete.ts, and replace the inline
z.null() and inline params object with the imported DeleteLyricsResponseSchema
and DeleteLyricsParamsSchema so the handler references the centralized DTOs.
| params: z.object({ | ||
| id: z.coerce.number().int().positive(), | ||
| lyricId: z.coerce.number().int().positive(), | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move route param schema to core DTO module.
Inline z.object(...) at Line 24-27 should be replaced by a shared schema exported from packages/core/src/modules/catalog/song/dto.ts.
♻️ Proposed refactor
-import z from "zod";
+import { SongLyricPathParamsSchema } from "@cvsa/core";
...
- params: z.object({
- id: z.coerce.number().int().positive(),
- lyricId: z.coerce.number().int().positive(),
- }),
+ params: SongLyricPathParamsSchema,// packages/core/src/modules/catalog/song/dto.ts
export const SongLyricPathParamsSchema = z.object({
id: z.coerce.number().int().positive(),
lyricId: z.coerce.number().int().positive(),
});As per coding guidelines: apps/backend/src/handlers/**/*.ts: Define all Zod schemas in packages/core/modules/<module>/dto.ts first; do not define them inline inside handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/handlers/catalog/song/lyrics/get.ts` around lines 24 - 27,
Replace the inline Zod schema in the handler's params with the shared schema
exported from the core DTO; specifically, in
apps/backend/src/handlers/catalog/song/lyrics/get.ts remove the inline
z.object(...) used in the params and import SongLyricPathParamsSchema from
packages/core/src/modules/catalog/song/dto.ts, then use
SongLyricPathParamsSchema in the params field (also add/adjust the import
statement). Ensure the symbol name SongLyricPathParamsSchema is used exactly to
match the exported DTO.
| params: z.object({ | ||
| id: z.coerce.number().int().positive(), | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract params schema into shared song DTOs.
The inline params schema at Line 24-26 should be imported from @cvsa/core instead of being declared inside the handler.
As per coding guidelines: apps/backend/src/handlers/**/*.ts: Define all Zod schemas in packages/core/modules/<module>/dto.ts first; do not define them inline inside handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/handlers/catalog/song/lyrics/list.ts` around lines 24 - 26,
Replace the inline params zod schema (params: z.object({ id:
z.coerce.number().int().positive() })) with the shared schema exported from
`@cvsa/core`: create/export a SongParamsDto (or songParamsSchema) in the core song
DTO file (packages/core/modules/song/dto.ts) that defines the id
coercion/validation, then import that symbol from '@cvsa/core' into the handler
and use it in place of the inline params; ensure the handler's params reference
name matches the exported DTO name and remove the inline z.object block.
| params: z.object({ | ||
| id: z.coerce.number().int().positive(), | ||
| lyricId: z.coerce.number().int().positive(), | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract id/lyricId params schema to core DTO module.
Please replace the inline params schema at Line 34-37 with an imported shared schema from @cvsa/core.
As per coding guidelines: apps/backend/src/handlers/**/*.ts: Define all Zod schemas in packages/core/modules/<module>/dto.ts first; do not define them inline inside handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/handlers/catalog/song/lyrics/update.ts` around lines 34 -
37, Replace the inline params z.object({ id: z.coerce.number().int().positive(),
lyricId: z.coerce.number().int().positive() }) in the update handler with an
imported shared schema from `@cvsa/core` (e.g., export a SongLyricParamsSchema or
songLyricParamsDto from the core module DTO and import it into this handler). If
the shared DTO does not exist yet, add it to
packages/core/modules/<module>/dto.ts as a zod schema that validates id and
lyricId, export it, then update the handler to use the imported schema
(referencing the existing params, id and lyricId symbols) instead of defining
the schema inline.
| async getLyric(id: SongId, lyricId: number): Promise<SongLyricsResponseDto> { | ||
| const existing = await this.repository.getById(id); | ||
| if (existing === null) { | ||
| throw new AppError("error.song.notfound", "NOT_FOUND", 404); | ||
| } | ||
| const lyric = await traceTask("db get lyric", async () => { | ||
| return await this.repository.getLyricById(lyricId); | ||
| }); | ||
| if (lyric === null) { | ||
| throw new AppError("error.lyric.notfound", "NOT_FOUND", 404); | ||
| } | ||
| return lyric; | ||
| } |
There was a problem hiding this comment.
Missing lyric ownership verification allows cross-song access.
getLyric, updateLyric, and deleteLyric accept id: SongId but only verify the song exists—they don't verify the lyric actually belongs to that song. The repository's getLyricById method doesn't filter by songId.
This allows a user to access or modify lyrics from a different song if they know the lyricId:
GET /v2/song/123/lyric/456returns lyric 456 even if it belongs to song 789.
🔐 Proposed fix: Add songId verification to repository or service
Option 1: Update repository to include songId filtering
In repository.ts, modify getLyricById to accept and verify songId:
- async getLyricById(lyricId: number, tx?: TxClient) {
+ async getLyricById(songId: SongId, lyricId: number, tx?: TxClient) {
const client = tx ?? this.prisma;
return transformPrismaResult(
await client.lyrics.findFirst({
- where: { id: lyricId, deletedAt: null },
+ where: { id: lyricId, songId: songId, deletedAt: null },
omit: { deletedAt: true, songId: true },
})
);
}Then update service calls:
async getLyric(id: SongId, lyricId: number): Promise<SongLyricsResponseDto> {
const existing = await this.repository.getById(id);
if (existing === null) {
throw new AppError("error.song.notfound", "NOT_FOUND", 404);
}
const lyric = await traceTask("db get lyric", async () => {
- return await this.repository.getLyricById(lyricId);
+ return await this.repository.getLyricById(id, lyricId);
});
if (lyric === null) {
throw new AppError("error.lyric.notfound", "NOT_FOUND", 404);
}
return lyric;
}Also applies to: 115-131, 133-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/modules/catalog/song/service.ts` around lines 88 - 100, The
service methods getLyric, updateLyric, and deleteLyric currently fetch a lyric
by lyricId only (via repository.getLyricById) and do not verify it belongs to
the provided song id, allowing cross-song access; fix by enforcing ownership:
either change the repository signature to getLyricById(lyricId, songId) and
update calls in getLyric/updateLyric/deleteLyric to pass the song id, or after
fetching the lyric in those service methods check lyric.songId === id and throw
AppError("error.lyric.notfound", "NOT_FOUND", 404) if it doesn't match; update
all three methods to perform this ownership check using the
repository.getLyricById symbol or the lyric.songId check.
a74f39f to
b944c58
Compare
Song Lyrics CRUD Endpoints Implementation
Backend Handlers
apps/backend/src/handlers/catalog/song/lyrics/:create.ts: Authenticated POST/song/:id/lyricendpoint for creating lyricsget.ts: GET/song/:id/lyric/:lyricIdendpoint for retrieving specific lyricslist.ts: GET/song/:id/lyricsendpoint for listing all lyrics of a songupdate.ts: Authenticated PATCH/song/:id/lyric/:lyricIdendpoint for updating lyricsdelete.ts: Authenticated DELETE/song/:id/lyric/:lyricIdendpoint for soft-deleting lyricsindex.ts: Composite handler that registers all lyric sub-handlersCore Layer - Data Transfer Objects & Schemas
nullish()tooptional()SongDetailsResponseSchemato omitdeletedAtfrom lyrics and exclude soft-deleted recordsTesting
apps/backend/tests/auth.test.tsby removing unusedbeforeEachimportpackages/core/tests/unit/SongService.test.tsmock repository with new lyric methodspackages/core/tests/unit/SongSearchService.test.tsfixture to removedeletedAtfield and add lyric propertiesDocumentation
AGENTS.mdto document new backend packages:packages/embedding,packages/logger,packages/observability, andpackages/envRelated