feat(engine): add CRUD endpoints for SVS engine#49
Conversation
Add full CRUD endpoints for managing SVS engines in the catalog: - GET /v2/engine/:id/details - retrieve engine details - POST /v2/engine - create new engine (auth required) - PATCH /v2/engine/:id - update engine (auth required) - DELETE /v2/engine/:id - soft delete engine (auth required) Also includes minor fixes to song service for missing search sync calls on lyrics operations. Closes #48
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request adds complete CRUD functionality for SVS engines, including four HTTP endpoints (GET details, POST create, PATCH update, DELETE soft-delete), corresponding data access layers with repository and service implementations, DTOs and Zod validation schemas, dependency injection wiring, and comprehensive E2E tests covering all operations and error cases. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as Handler<br/>(engineCreateHandler)
participant Service as Service<br/>(engineService)
participant Repo as Repository<br/>(engineRepository)
participant DB as Database<br/>(Prisma)
Client->>Handler: POST /engine<br/>(authenticated)
activate Handler
Handler->>Handler: validate request body<br/>via CreateEngineRequestSchema
Handler->>Service: traceTask("engineService.create")<br/>engineService.create(body)
deactivate Handler
activate Service
Service->>Service: traceTask("db create engine")
Service->>Repo: repository.create(input)
deactivate Service
activate Repo
Repo->>DB: prisma.svsEngine.create()
activate DB
DB-->>Repo: created engine record
deactivate DB
Repo->>Repo: transformPrismaResult()
Repo-->>Service: EngineResponseDto
deactivate Repo
Service-->>Handler: EngineResponseDto
Handler->>Handler: status(201, engine)
Handler-->>Client: HTTP 201<br/>EngineResponseSchema
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 4
🧹 Nitpick comments (9)
packages/core/src/modules/catalog/engine/dto.ts (2)
18-20:EngineDetailsResponseSchemais a pure alias ofEngineResponseSchema.If details and summary are intended to diverge later, keeping the alias is fine; otherwise consider exporting only one schema and re-using it at call sites to avoid the impression that these shapes differ. Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/engine/dto.ts` around lines 18 - 20, EngineDetailsResponseSchema is currently just a direct alias of EngineResponseSchema; either remove the alias and export only EngineResponseSchema (updating call sites to use EngineResponseSchema), or make the intent explicit by keeping EngineDetailsResponseSchema but adding a clarifying comment above it stating it intentionally mirrors EngineResponseSchema for potential future divergence—refer to the symbols EngineResponseSchema and EngineDetailsResponseSchema when applying the change.
12-16:UpdateEngineRequestSchemaallows an empty payload.All fields are optional, so
PATCH /engine/:idwith{}validates successfully and produces a no-op DB write (and a 200 response). Consider requiring at least one field.♻️ Proposed refinement
-export const UpdateEngineRequestSchema = z.object({ - name: z.string().min(1).max(255).optional(), - description: z.string().optional(), - localizedDescriptions: z.record(z.string(), z.string()).optional(), -}); +export const UpdateEngineRequestSchema = z + .object({ + name: z.string().min(1).max(255).optional(), + description: z.string().optional(), + localizedDescriptions: z.record(z.string(), z.string()).optional(), + }) + .refine((v) => Object.keys(v).length > 0, { + error: "At least one field must be provided", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/engine/dto.ts` around lines 12 - 16, UpdateEngineRequestSchema currently allows an empty object because all fields are optional; change the schema so PATCH /engine/:id rejects an empty payload by requiring at least one of the fields be present. Modify UpdateEngineRequestSchema (the z.object with name, description, localizedDescriptions) to add a refinement that checks the parsed object has at least one key (e.g., .refine(obj => Object.keys(obj).length > 0, { message: "At least one field must be provided" }) or equivalent) so an empty {} will fail validation.packages/core/src/modules/catalog/engine/index.ts (1)
1-5: Consider not re-exportingrepository.interfaceandrepositoryfrom the public barrel.Exposing the concrete
EngineRepositoryand theIEngineRepositoryabstract class through the top-level@cvsa/coresurface leaks data-access internals that consumers shouldn't need — onlyengineService, DTOs, and schemas are needed by handlers. Limiting the surface keeps future refactors safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/engine/index.ts` around lines 1 - 5, The barrel file currently re-exports concrete and interface repository types (repository, repository.interface, e.g., EngineRepository and IEngineRepository), leaking data-access internals; remove those two exports from packages/core/src/modules/catalog/engine/index.ts so only service, DTOs and container/schema types remain exported (keep service export such as engineService and DTO/schema exports), ensuring handlers consume the service and DTOs only and the repository types stay internal to the module.apps/backend/src/handlers/catalog/engine/details.ts (1)
20-24:400response is declared but unreachable from this handler.Zod
paramsvalidation failures in Elysia produce a422(or framework-defined validation status), and the service only throws404. The400: ErrorResponseSchemaentry is dead documentation — consider removing it or confirming your error mapper converts validation errors to 400.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/handlers/catalog/engine/details.ts` around lines 20 - 24, The responses object declares a 400 ErrorResponseSchema but that status is never produced by this handler; either remove the `400: ErrorResponseSchema` entry from the response map (leaving `200: EngineDetailsResponseSchema` and `404: ErrorResponseSchema`) or, if you intend to document framework validation failures, replace/add the correct framework status (e.g. `422: ErrorResponseSchema`) and ensure your validation-to-status mapper maps Zod param errors to that status; locate the response object in the handler where `EngineDetailsResponseSchema` and `ErrorResponseSchema` are referenced and update accordingly.packages/core/src/modules/catalog/engine/service.ts (2)
41-49: Minor:await+returninsidetraceTaskis redundant fordelete.
softDeletereturnsvoid, so the innerreturn awaitcan be simplified to justawait. Purely stylistic.♻️ Proposed tweak
- await traceTask("db delete engine", async () => { - return await this.repository.softDelete(id); - }); + await traceTask("db delete engine", () => this.repository.softDelete(id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/engine/service.ts` around lines 41 - 49, The delete method contains a redundant "return await" inside the traceTask callback; update the traceTask call in the delete function to simply await this.repository.softDelete(id) (remove the inner return await) while keeping the existing null check that throws AppError("error.engine.notfound", "NOT_FOUND", 404) and the traceTask("db delete engine", ...) wrapper around the repository.softDelete call.
31-49: TOCTOU between existence check and mutation inupdate/delete.
updateanddeletedogetByIdthenupdate/softDeleteas separate DB round trips. Under concurrent requests, an engine could be soft-deleted between the check and the mutation, causing the follow-uprepository.updateto silently mutate a soft-deleted row (see the matching repository comment) orsoftDeleteto re-stampdeletedAt.Low risk for typical CRUD admin usage, but if you want consistency, wrap both steps in a Prisma interactive transaction, or move the existence check into the mutation itself by using
updateManywith{ id, deletedAt: null }and throwingNOT_FOUNDwhencount === 0. This also saves one round trip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/engine/service.ts` around lines 31 - 49, The update and delete methods perform a separate getById check before calling repository.update or repository.softDelete, creating a TOCTOU race where the row may be soft-deleted between calls; fix by performing the existence check and mutation atomically: either wrap the get+mutate in a Prisma interactive transaction inside the service (use traceTask("db update engine") / traceTask("db delete engine") to keep tracing) or replace the two-step pattern with a conditional mutation (e.g., repository.updateMany / updateCount style that filters on { id, deletedAt: null } and throws the same AppError("error.engine.notfound","NOT_FOUND",404) when affected count === 0); update the update(id: EngineId, input: UpdateEngineRequestDto) and delete(id: EngineId) paths to use one of these atomic approaches and remove the separate getById race.apps/backend/tests/engine.test.ts (2)
95-122: Missing assertion ondescriptionin the localized-descriptions test.The
toMatchObjectcheck validatesnameandlocalizedDescriptionsbut dropsdescription, weakening the test relative to the earlier case. Worth addingdescription: "A singing voice synthesis software"for symmetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/tests/engine.test.ts` around lines 95 - 122, The test "should create an engine with localized descriptions" is missing an assertion for the plain description; update the expect(data).toMatchObject in that test to include description: "A singing voice synthesis software" so the response is validated against payload.description; locate the test block and the payload variable and add the description property to the toMatchObject check for api.v2.engine.post's response.
22-28:getAuthTokensilently returnsundefinedon signup failure.If the signup call fails (e.g., validation or DB error),
signup.data?.data.tokenresolves toundefinedand the subsequent authenticated requests will sendBearer undefined, yielding confusing 401s instead of a clear test failure. Consider asserting the signup response shape before returning the token.🧪 Proposed fix
async function getAuthToken() { const signup = await api.v2.user.post({ username: `${Math.random()}`, password: "password123", }); - return signup.data?.data.token; + const token = signup.data?.data.token; + if (!token) { + throw new Error(`Signup failed: ${JSON.stringify(signup.error?.value)}`); + } + return token; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/tests/engine.test.ts` around lines 22 - 28, The helper getAuthToken currently returns signup.data?.data.token which can be undefined on signup failure; update getAuthToken to validate the signup response from api.v2.user.post (inspect the signup variable and the nested data.data.token) and throw a descriptive error or fail the test if the token is missing or response shape is unexpected; ensure any thrown error includes the signup response (or status/message) so tests fail fast with clear diagnostics rather than sending "Bearer undefined".packages/core/src/modules/catalog/engine/repository.ts (1)
45-63:updateandsoftDeletedo not filter bydeletedAt: null.Unlike
getById/getDetailsById, theupdateandsoftDeletequeries usewhere: { id }without adeletedAt: nullguard. Today the service layer pre-checks existence viagetById(which does filter), so misuse is prevented. However, if these repository methods are ever called directly (bulk ops, future services, tx flows), they will happily mutate already soft-deleted rows — including re-stampingdeletedAton a subsequentsoftDeletecall.Consider tightening the
whereclause at the repository level for defense-in-depth:♻️ Proposed refinement
async update(id: EngineId, input: UpdateEngineRequestDto, tx?: TxClient) { const client = tx ?? this.prisma; return transformPrismaResult( - await client.svsEngine.update({ - where: { id }, + await client.svsEngine.update({ + where: { id, deletedAt: null }, data: input, omit: { deletedAt: true }, }) ); } async softDelete(id: EngineId, tx?: TxClient) { const client = tx ?? this.prisma; - await client.svsEngine.update({ - where: { id }, + await client.svsEngine.update({ + where: { id, deletedAt: null }, data: { deletedAt: new Date() }, }); }Note: Prisma
updatewith a compoundwhererequires the filter fields to be part of a unique index; if not, useupdateMany(which accepts arbitrary filters) and check the returnedcount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/engine/repository.ts` around lines 45 - 63, The update and softDelete methods currently use where: { id } and can affect soft-deleted rows; modify both EngineRepository.update and EngineRepository.softDelete to ensure they only target non-deleted records by adding a deletedAt: null guard in the where clause; if Prisma prevents a compound where for update because id is the only unique field, switch to client.svsEngine.updateMany({ where: { id, deletedAt: null }, data: ... }) for both operations and verify the returned count is 1 (or >0) and surface an error if nothing was updated so callers know the record did not exist or was already deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/handlers/catalog/engine/create.ts`:
- Around line 1-30: Run the repository formatter and commit the changes to fix
the CI lint/format failure: locate the engineCreateHandler export (the Elysia
instance named "engineCreateHandler" in this file) and run the project's
formatting command (e.g., bun run format or prettier --write .) to rewrite the
file with the correct formatting, then stage and commit the formatted file so
the Test and Lint check passes.
In `@packages/core/src/modules/catalog/engine/repository.interface.ts`:
- Around line 1-21: The file defining IEngineRepository (symbols:
IEngineRepository, getById, getDetailsById, create, update, softDelete) is
failing CI due to formatting; run the repository formatter (e.g.,
prettier/format script configured in the repo) on
packages/core/src/modules/catalog/engine/repository.interface.ts (or run the
project-wide format command) and commit the formatted file so the interface and
its method signatures conform to the project's lint/format rules.
In `@packages/core/src/modules/catalog/song/service.ts`:
- Around line 134-142: The DB update promise stored in result is not awaited
before triggering the search sync; change the flow so traceTask("db update
lyric", async () => this.repository.updateLyric(lyricId, input)) is awaited
(await the returned Promise/result) before calling traceTask("sync search
index", ...) so that this.search.sync(id) (SongSearchService.sync) runs only
after this.repository.updateLyric completes; keep the error handling for
this.search.sync as-is.
- Around line 113-117: The traced callback passed to traceTask currently calls
this.search.sync(id).catch(...) inside the async function, causing traceTask to
resolve before sync finishes; move the await into the traced callback and remove
the internal .catch so traceTask can observe failures (i.e., replace
this.search.sync(id).catch(...) with await this.search.sync(id) inside the
traceTask callback) and perform the catch outside the traceTask call (so any
errors are handled after traceTask resolves) while keeping the existing logging
(appLogger.warn/Bun.inspect) in that outer catch; apply the same change for all
occurrences using traceTask and this.search.sync (e.g., the blocks around lines
referencing id and traceTask).
---
Nitpick comments:
In `@apps/backend/src/handlers/catalog/engine/details.ts`:
- Around line 20-24: The responses object declares a 400 ErrorResponseSchema but
that status is never produced by this handler; either remove the `400:
ErrorResponseSchema` entry from the response map (leaving `200:
EngineDetailsResponseSchema` and `404: ErrorResponseSchema`) or, if you intend
to document framework validation failures, replace/add the correct framework
status (e.g. `422: ErrorResponseSchema`) and ensure your validation-to-status
mapper maps Zod param errors to that status; locate the response object in the
handler where `EngineDetailsResponseSchema` and `ErrorResponseSchema` are
referenced and update accordingly.
In `@apps/backend/tests/engine.test.ts`:
- Around line 95-122: The test "should create an engine with localized
descriptions" is missing an assertion for the plain description; update the
expect(data).toMatchObject in that test to include description: "A singing voice
synthesis software" so the response is validated against payload.description;
locate the test block and the payload variable and add the description property
to the toMatchObject check for api.v2.engine.post's response.
- Around line 22-28: The helper getAuthToken currently returns
signup.data?.data.token which can be undefined on signup failure; update
getAuthToken to validate the signup response from api.v2.user.post (inspect the
signup variable and the nested data.data.token) and throw a descriptive error or
fail the test if the token is missing or response shape is unexpected; ensure
any thrown error includes the signup response (or status/message) so tests fail
fast with clear diagnostics rather than sending "Bearer undefined".
In `@packages/core/src/modules/catalog/engine/dto.ts`:
- Around line 18-20: EngineDetailsResponseSchema is currently just a direct
alias of EngineResponseSchema; either remove the alias and export only
EngineResponseSchema (updating call sites to use EngineResponseSchema), or make
the intent explicit by keeping EngineDetailsResponseSchema but adding a
clarifying comment above it stating it intentionally mirrors
EngineResponseSchema for potential future divergence—refer to the symbols
EngineResponseSchema and EngineDetailsResponseSchema when applying the change.
- Around line 12-16: UpdateEngineRequestSchema currently allows an empty object
because all fields are optional; change the schema so PATCH /engine/:id rejects
an empty payload by requiring at least one of the fields be present. Modify
UpdateEngineRequestSchema (the z.object with name, description,
localizedDescriptions) to add a refinement that checks the parsed object has at
least one key (e.g., .refine(obj => Object.keys(obj).length > 0, { message: "At
least one field must be provided" }) or equivalent) so an empty {} will fail
validation.
In `@packages/core/src/modules/catalog/engine/index.ts`:
- Around line 1-5: The barrel file currently re-exports concrete and interface
repository types (repository, repository.interface, e.g., EngineRepository and
IEngineRepository), leaking data-access internals; remove those two exports from
packages/core/src/modules/catalog/engine/index.ts so only service, DTOs and
container/schema types remain exported (keep service export such as
engineService and DTO/schema exports), ensuring handlers consume the service and
DTOs only and the repository types stay internal to the module.
In `@packages/core/src/modules/catalog/engine/repository.ts`:
- Around line 45-63: The update and softDelete methods currently use where: { id
} and can affect soft-deleted rows; modify both EngineRepository.update and
EngineRepository.softDelete to ensure they only target non-deleted records by
adding a deletedAt: null guard in the where clause; if Prisma prevents a
compound where for update because id is the only unique field, switch to
client.svsEngine.updateMany({ where: { id, deletedAt: null }, data: ... }) for
both operations and verify the returned count is 1 (or >0) and surface an error
if nothing was updated so callers know the record did not exist or was already
deleted.
In `@packages/core/src/modules/catalog/engine/service.ts`:
- Around line 41-49: The delete method contains a redundant "return await"
inside the traceTask callback; update the traceTask call in the delete function
to simply await this.repository.softDelete(id) (remove the inner return await)
while keeping the existing null check that throws
AppError("error.engine.notfound", "NOT_FOUND", 404) and the traceTask("db delete
engine", ...) wrapper around the repository.softDelete call.
- Around line 31-49: The update and delete methods perform a separate getById
check before calling repository.update or repository.softDelete, creating a
TOCTOU race where the row may be soft-deleted between calls; fix by performing
the existence check and mutation atomically: either wrap the get+mutate in a
Prisma interactive transaction inside the service (use traceTask("db update
engine") / traceTask("db delete engine") to keep tracing) or replace the
two-step pattern with a conditional mutation (e.g., repository.updateMany /
updateCount style that filters on { id, deletedAt: null } and throws the same
AppError("error.engine.notfound","NOT_FOUND",404) when affected count === 0);
update the update(id: EngineId, input: UpdateEngineRequestDto) and delete(id:
EngineId) paths to use one of these atomic approaches and remove the separate
getById race.
🪄 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: 87b7ec56-626d-4037-a36a-f65a8b9c30a0
📒 Files selected for processing (17)
apps/backend/src/handlers/catalog/engine/create.tsapps/backend/src/handlers/catalog/engine/delete.tsapps/backend/src/handlers/catalog/engine/details.tsapps/backend/src/handlers/catalog/engine/index.tsapps/backend/src/handlers/catalog/engine/update.tsapps/backend/src/handlers/index.tsapps/backend/src/index.tsapps/backend/tests/engine.test.tspackages/core/src/modules/catalog/engine/container.tspackages/core/src/modules/catalog/engine/dto.tspackages/core/src/modules/catalog/engine/index.tspackages/core/src/modules/catalog/engine/repository.interface.tspackages/core/src/modules/catalog/engine/repository.tspackages/core/src/modules/catalog/engine/service.tspackages/core/src/modules/catalog/index.tspackages/core/src/modules/catalog/song/service.tspackages/core/tests/integration/SongSearchService.test.ts
💤 Files with no reviewable changes (1)
- packages/core/tests/integration/SongSearchService.test.ts
| await traceTask("sync search index", async () => { | ||
| this.search.sync(id).catch((e) => { | ||
| appLogger.warn(Bun.inspect(e)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Await search.sync inside traceTask and catch outside it.
These blocks currently fire-and-forget the sync, so traceTask ends before indexing finishes and cannot mark sync failures as errors. Catch the rejection after traceTask so the endpoint can still succeed while preserving tracing accuracy.
🔎 Proposed fix for each sync block
await traceTask("sync search index", async () => {
- this.search.sync(id).catch((e) => {
- appLogger.warn(Bun.inspect(e));
- });
+ await this.search.sync(id);
+ }).catch((e) => {
+ appLogger.warn(Bun.inspect(e));
});Verification:
#!/bin/bash
# Description: Find sync blocks that hide `search.sync` from `traceTask`.
# Expectation after the fix: no matches for `.catch(...)` inside the traced callback.
ast-grep --pattern $'await traceTask("sync search index", async () => {
$CALL.catch($$$);
});'
# Confirm traceTask only marks spans as ERROR when the callback throws/rejects.
sed -n '1,18p' packages/observability/src/tracer.tsAlso applies to: 137-141, 157-161
🤖 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 113 - 117,
The traced callback passed to traceTask currently calls
this.search.sync(id).catch(...) inside the async function, causing traceTask to
resolve before sync finishes; move the await into the traced callback and remove
the internal .catch so traceTask can observe failures (i.e., replace
this.search.sync(id).catch(...) with await this.search.sync(id) inside the
traceTask callback) and perform the catch outside the traceTask call (so any
errors are handled after traceTask resolves) while keeping the existing logging
(appLogger.warn/Bun.inspect) in that outer catch; apply the same change for all
occurrences using traceTask and this.search.sync (e.g., the blocks around lines
referencing id and traceTask).
Changes
Add full CRUD endpoints for managing SVS engines in the catalog:
Also includes minor fixes to song service for missing search sync calls on lyrics operations.
Related