-
Notifications
You must be signed in to change notification settings - Fork 1
fix(seed): ensure PrismaClient connections are properly awaited in seed scripts (#86) #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,7 @@ | |
| "jest": { | ||
| "preset": "ts-jest", | ||
| "testEnvironment": "node", | ||
| "detectOpenHandles": true, | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| "moduleNameMapper": { | ||
| "@app/custom-validators": "<rootDir>/src/validators" | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,5 @@ | ||
| import { PrismaClient } from "@prisma/client"; | ||
|
|
||
| export async function customSeed() { | ||
| const client = new PrismaClient(); | ||
|
|
||
| client.$disconnect(); | ||
| export async function customSeed(client: PrismaClient) { | ||
| // TODO: add seeding logic here using the provided client instance | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,14 @@ import * as dotenv from "dotenv"; | |
| import { PrismaClient } from "@prisma/client"; | ||
| import { customSeed } from "./customSeed"; | ||
|
|
||
| // Single PrismaClient instance for the whole script | ||
| const client = new PrismaClient(); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| // Extra safety: ensure disconnect on process exit | ||
| process.on("beforeExit", async () => { | ||
| await client.$disconnect(); | ||
| }); | ||
|
|
||
| if (require.main === module) { | ||
| dotenv.config(); | ||
|
|
||
|
|
@@ -10,16 +18,24 @@ if (require.main === module) { | |
| if (!BCRYPT_SALT) { | ||
| throw new Error("BCRYPT_SALT environment variable must be defined"); | ||
| } | ||
|
|
||
| // Execute the seed when run directly | ||
| seed().catch((err) => { | ||
| console.error("Failed to seed database", err); | ||
| process.exit(1); | ||
| }); | ||
| } | ||
|
|
||
| async function seed() { | ||
| console.info("Seeding database..."); | ||
|
|
||
| const client = new PrismaClient(); | ||
| void client.$disconnect(); | ||
| try { | ||
| console.info("Seeding database with custom seed..."); | ||
| await customSeed(client); | ||
|
|
||
| console.info("Seeding database with custom seed..."); | ||
| customSeed(); | ||
|
|
||
| console.info("Seeded database successfully"); | ||
| console.info("Seeded database successfully"); | ||
| } finally { | ||
| // Always disconnect, even if seeding throws | ||
| await client.$disconnect(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { resolve } from 'path'; | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test assumes the |
||
| import { spawn } from 'child_process'; | ||
|
|
||
| /** | ||
| * Integration test to ensure the seed script exits cleanly | ||
| * without leaving open handles (detectOpenHandles enabled in Jest config). | ||
| */ | ||
|
|
||
| describe('Database seed script', () => { | ||
| it('should run to completion with exit code 0', async () => { | ||
| // Path to the seed script (TypeScript). | ||
| const scriptPath = resolve(__dirname, '../scripts/seed.ts'); | ||
|
|
||
| // Spawn the seed script via ts-node. | ||
| // Using stdio: 'inherit' helps observe any console output during CI failures. | ||
| const child = spawn( | ||
| process.execPath, // Node.js binary executing ts-node register and script | ||
| [ | ||
| // Register ts-node transpiler | ||
| '-r', | ||
| 'ts-node/register', | ||
| scriptPath, | ||
| ], | ||
| { stdio: 'inherit' }, | ||
| ); | ||
|
|
||
| const exitCode: number = await new Promise((resolvePromise, rejectPromise) => { | ||
| child.on('error', rejectPromise); | ||
| child.on('exit', resolvePromise); | ||
| }); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| }, 60_000); // generous timeout for DB operations | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # Prisma Seed Scripts Audit – Database Connection Handling | ||
|
|
||
| Date: 2025-10-31 | ||
|
|
||
| This audit searches the repository for potential mis-management of `PrismaClient` connections in seed / script files. We looked specifically for: | ||
|
|
||
| * `new PrismaClient(` – new client instantiation | ||
| * `$disconnect()` – connection teardown | ||
|
|
||
| --- | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The audit still flags issues that this PR already fixed (unawaited |
||
|
|
||
| ## Findings | ||
|
|
||
| | # | File Path | Line(s) | Code Snippet | Requires Change? | Recommended Action | | ||
| |---|-----------|---------|--------------|------------------|--------------------| | ||
| | 1 | `apps/hotel-management-service-server/scripts/seed.ts` | 18-19 | ```ts | ||
| const client = new PrismaClient(); | ||
| void client.$disconnect(); | ||
| ``` | **Yes** | • Move `$disconnect()` into a `finally` block and `await` it.<br/>• Prefer passing a shared `PrismaClient` instance to `customSeed()` instead of creating separate instances.<br/>• Await `customSeed()` if it becomes async. | | ||
| | 2 | `apps/hotel-management-service-server/scripts/customSeed.ts` | 4-6 | ```ts | ||
| const client = new PrismaClient(); | ||
|
|
||
| client.$disconnect(); | ||
| ``` | **Yes** | • Accept a `PrismaClient` parameter rather than creating a new one.<br/>• Ensure the caller handles cleanup in `finally` with `await client.$disconnect()`. | | ||
|
|
||
| No other occurrences of `PrismaClient` instantiation or `$disconnect()` were found in the repository at the time of this audit. | ||
|
|
||
| --- | ||
|
|
||
| ## Summary | ||
| Both seed scripts create their own `PrismaClient` instance and call `$disconnect()` without awaiting the returned promise. This risks the Node.js process exiting before the connection is cleanly closed, possibly causing connection leaks. | ||
|
|
||
| **Refactor Plan (to be implemented in subsequent phases):** | ||
|
|
||
| 1. Instantiate a single `PrismaClient` in `seed.ts`. | ||
| 2. Wrap all seeding logic in `try { … } finally { await client.$disconnect(); }`. | ||
| 3. Pass the shared client into `customSeed()` (update its signature accordingly). | ||
| 4. Use `process.on('beforeExit')` as a secondary safeguard if desired. | ||
|
|
||
| This document will serve as reference for Phase 2 refactoring work. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeding instructions should mention that the
BCRYPT_SALTenvironment variable must be set (or sourced from a copied.envfile). Without itnpm run seedthrows at runtime, so readers following the guide are likely to hit an avoidable error.