feat: use ssl config for mysql connections, update build process DMD-1605#293
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughCI workflows and release tooling updated (Node.js → 22, Actions v3 → v4, Corepack/Yarn v4); Yarn node-modules linker and packageManager added; build script adjusted; MySQL pool creation now always includes an Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Here's the code health analysis summary for commits Analysis Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@__tests__/execute-query.spec.js`:
- Around line 35-41: The object literal assigned to dbConfig has its keys in the
wrong order for the sort-keys rule; reorder the properties in the dbConfig
object so that ssl appears before user (i.e., database, host, password, ssl,
user) to satisfy the linter failing in the test file where dbConfig is defined.
In `@__tests__/execute-transaction.spec.js`:
- Around line 42-49: The object literal assigned to dbConfig in the test
violates sort-keys; reorder its properties so "ssl" appears before "user" (e.g.,
database, host, password, ssl, user) to satisfy the ESLint sort-keys rule in the
test case that checks SSL passthrough.
In @.github/workflows/publish.yml:
- Around line 21-23: The workflow's npm-publish job declares a non-existent
dependency "build-and-publish"; update the npm-publish job's needs field so it
depends on the actual job name "build-and-test" (i.e., change needs:
build-and-publish to needs: build-and-test) to ensure the correct job ordering
for the npm-publish job.
In `@src/execute-query.ts`:
- Line 18: Update the test setup in beforeEach to initialize mockPool with
default jest.fn() implementations for query and execute so tests calling
executeQuery or executeQueryWithParams don't hit "pool.query is not a function"
/ "pool.execute is not a function"; specifically set mockPool = { query:
jest.fn().mockResolvedValue([[], null]), execute:
jest.fn().mockResolvedValue([[], null]) } and keep
getPool.mockResolvedValue(mockPool) so individual tests can still override these
mocks as needed.
🧹 Nitpick comments (5)
src/execute-query.ts (1)
11-19: ESLint sort-keys:sslshould come beforeuser.The project enforces alphabetical key ordering. Move
sslbeforeuserto satisfy the linter.♻️ Proposed fix
const pool = await getPool({ charset: dbConfig.charset, connectTimeout: dbConfig.connectTimeout, database: dbConfig.database, host: dbConfig.host, password: dbConfig.password, + ssl: dbConfig.ssl !== undefined ? dbConfig.ssl : {}, user: dbConfig.user, - ssl: dbConfig.ssl !== undefined ? dbConfig.ssl : {}, });.github/workflows/pull_request.yml (1)
12-13: Update GitHub Actions to v4.
actions/checkoutandactions/setup-nodeare at v3 but v4 is available with security fixes and Node 20+ improvements. Consider updating for consistency with the Node 22 upgrade.♻️ Proposed fix
- - uses: actions/checkout@v3 - - uses: actions/setup-node@v3 + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4src/execute-transaction.ts (1)
11-17: ESLint sort-keys:sslshould come beforeuser.Same linting issue as in
execute-query.ts. Also note this pool config omitscharsetandconnectTimeoutwhich are included inexecute-query.tsandstream-query.ts- verify if this inconsistency is intentional.♻️ Proposed fix for key ordering
const pool = await getPool({ + charset: dbConfig.charset, + connectTimeout: dbConfig.connectTimeout, database: dbConfig.database, host: dbConfig.host, password: dbConfig.password, + ssl: dbConfig.ssl !== undefined ? dbConfig.ssl : {}, user: dbConfig.user, - ssl: dbConfig.ssl !== undefined ? dbConfig.ssl : {}, });src/stream-query.ts (2)
11-19: ESLint sort-keys:sslshould come beforeuser.Same linting issue as other files.
♻️ Proposed fix
const pool = createPool({ charset: dbConfig.charset, connectTimeout: dbConfig.connectTimeout, database: dbConfig.database, host: dbConfig.host, password: dbConfig.password, + ssl: dbConfig.ssl !== undefined ? dbConfig.ssl : {}, user: dbConfig.user, - ssl: dbConfig.ssl !== undefined ? dbConfig.ssl : {}, });
10-30: UsegetPool()to reuse pooled connections and prevent connection leaks.The
streamQuery()function creates a new pool for every call viacreatePool()but never closes it, causing a connection leak. UnlikeexecuteQuery()andexecuteTransaction()which cache pools viagetPool(), this function duplicates pool creation. Consider refactoring to usegetPool()instead, which maintains a shared pool cache keyed onhost:databaseand prevents connection exhaustion.
| const dbConfig = { | ||
| database: chance.word(), | ||
| host: chance.word(), | ||
| password: chance.word(), | ||
| user: chance.word(), | ||
| ssl: { ca: chance.string() }, | ||
| }; |
There was a problem hiding this comment.
Fix key order to satisfy sort-keys lint.
The linter expects ssl before user in this object.
🔧 Suggested reorder
- user: chance.word(),
- ssl: { ca: chance.string() },
+ ssl: { ca: chance.string() },
+ user: chance.word(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dbConfig = { | |
| database: chance.word(), | |
| host: chance.word(), | |
| password: chance.word(), | |
| user: chance.word(), | |
| ssl: { ca: chance.string() }, | |
| }; | |
| const dbConfig = { | |
| database: chance.word(), | |
| host: chance.word(), | |
| password: chance.word(), | |
| ssl: { ca: chance.string() }, | |
| user: chance.word(), | |
| }; |
🧰 Tools
🪛 ESLint
[error] 40-40: Expected object keys to be in ascending order. 'ssl' should be before 'user'.
(sort-keys)
🤖 Prompt for AI Agents
In `@__tests__/execute-query.spec.js` around lines 35 - 41, The object literal
assigned to dbConfig has its keys in the wrong order for the sort-keys rule;
reorder the properties in the dbConfig object so that ssl appears before user
(i.e., database, host, password, ssl, user) to satisfy the linter failing in the
test file where dbConfig is defined.
| it('should pass through ssl config when provided', async () => { | ||
| const dbConfig = { | ||
| database: chance.word(), | ||
| host: chance.word(), | ||
| password: chance.word(), | ||
| user: chance.word(), | ||
| ssl: { ca: chance.string() }, | ||
| }; |
There was a problem hiding this comment.
Fix key order to satisfy sort-keys lint.
ESLint’s sort-keys rule expects ssl before user here.
🔧 Suggested reorder
- user: chance.word(),
- ssl: { ca: chance.string() },
+ ssl: { ca: chance.string() },
+ user: chance.word(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should pass through ssl config when provided', async () => { | |
| const dbConfig = { | |
| database: chance.word(), | |
| host: chance.word(), | |
| password: chance.word(), | |
| user: chance.word(), | |
| ssl: { ca: chance.string() }, | |
| }; | |
| it('should pass through ssl config when provided', async () => { | |
| const dbConfig = { | |
| database: chance.word(), | |
| host: chance.word(), | |
| password: chance.word(), | |
| ssl: { ca: chance.string() }, | |
| user: chance.word(), | |
| }; |
🧰 Tools
🪛 ESLint
[error] 48-48: Expected object keys to be in ascending order. 'ssl' should be before 'user'.
(sort-keys)
🤖 Prompt for AI Agents
In `@__tests__/execute-transaction.spec.js` around lines 42 - 49, The object
literal assigned to dbConfig in the test violates sort-keys; reorder its
properties so "ssl" appears before "user" (e.g., database, host, password, ssl,
user) to satisfy the ESLint sort-keys rule in the test case that checks SSL
passthrough.
| npm-publish: | ||
| needs: build-and-test | ||
| needs: build-and-publish | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Fix job dependency: build-and-publish doesn’t exist.
This will fail the workflow; npm-publish should depend on build-and-test.
✅ Suggested fix
- npm-publish:
- needs: build-and-publish
+ npm-publish:
+ needs: build-and-test📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| npm-publish: | |
| needs: build-and-test | |
| needs: build-and-publish | |
| runs-on: ubuntu-latest | |
| npm-publish: | |
| needs: build-and-test | |
| runs-on: ubuntu-latest |
🧰 Tools
🪛 actionlint (1.7.10)
[error] 21-21: job "npm-publish" needs job "build-and-publish" which does not exist in this workflow
(job-needs)
🤖 Prompt for AI Agents
In @.github/workflows/publish.yml around lines 21 - 23, The workflow's
npm-publish job declares a non-existent dependency "build-and-publish"; update
the npm-publish job's needs field so it depends on the actual job name
"build-and-test" (i.e., change needs: build-and-publish to needs:
build-and-test) to ensure the correct job ordering for the npm-publish job.
| host: dbConfig.host, | ||
| password: dbConfig.password, | ||
| user: dbConfig.user, | ||
| ssl: dbConfig.ssl !== undefined ? dbConfig.ssl : {}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find test files and check getPool mocks
rg -n -A 10 'getPool' --glob '*spec*' --glob '*test*'Repository: TractorZoom/serverless-mysql-utils
Length of output: 8850
🏁 Script executed:
# Also check the execute-query.ts file to see how getPool is used
cat -n src/execute-query.ts | head -40Repository: TractorZoom/serverless-mysql-utils
Length of output: 1504
🏁 Script executed:
# Find all test files to understand the structure
fd -e spec.ts -e test.tsRepository: TractorZoom/serverless-mysql-utils
Length of output: 108
🏁 Script executed:
# Let's see the full execute-query.spec.js file, especially the beforeEach and how mockPool is configured
cat -n __tests__/execute-query.spec.js | head -70Repository: TractorZoom/serverless-mysql-utils
Length of output: 2933
🏁 Script executed:
# Also check if there are methods added to mockPool later in the file
cat -n __tests__/execute-query.spec.js | sed -n '48,90p'Repository: TractorZoom/serverless-mysql-utils
Length of output: 1855
🏁 Script executed:
# Let's check the executeQueryWithParams test section to see if execute method is mocked
cat -n __tests__/execute-query.spec.js | sed -n '88,150p'Repository: TractorZoom/serverless-mysql-utils
Length of output: 2769
🏁 Script executed:
# Also look for any setup or teardown that might be relevant
cat -n __tests__/execute-query.spec.js | tail -100Repository: TractorZoom/serverless-mysql-utils
Length of output: 4297
Add pool method mocks to the test beforeEach setup.
The test file initializes mockPool as an empty object and doesn't set up query and execute methods in beforeEach. Tests that call executeQuery or executeQueryWithParams without first setting up these methods (e.g., lines 17-32 and 89-104 in __tests__/execute-query.spec.js) will fail with "pool.query is not a function" or "pool.execute is not a function". Update beforeEach to include default mocks for these methods:
beforeEach(() => {
mockPool = {
query: jest.fn().mockResolvedValue([[], null]),
execute: jest.fn().mockResolvedValue([[], null]),
};
getPool.mockResolvedValue(mockPool);
});This ensures all tests have functional pool methods available, and individual tests can override them as needed.
🧰 Tools
🪛 ESLint
[error] 18-18: Expected object keys to be in ascending order. 'ssl' should be before 'user'.
(sort-keys)
🤖 Prompt for AI Agents
In `@src/execute-query.ts` at line 18, Update the test setup in beforeEach to
initialize mockPool with default jest.fn() implementations for query and execute
so tests calling executeQuery or executeQueryWithParams don't hit "pool.query is
not a function" / "pool.execute is not a function"; specifically set mockPool =
{ query: jest.fn().mockResolvedValue([[], null]), execute:
jest.fn().mockResolvedValue([[], null]) } and keep
getPool.mockResolvedValue(mockPool) so individual tests can still override these
mocks as needed.
2 participants
Summary by CodeRabbit
Chores
Bug Fixes
Tests