Skip to content

Conversation

@HappyZombies
Copy link
Contributor

This PR intends to address #3764 by introducing a state property on the connection object, aligning behavior with the original mysql package.

The new property reflects the current connection status; it only includes the following three states: disconnected, connected, or protocol_error.

The state flow includes:

Default: 'disconnected'

Updated to 'connected' upon successful handshake.

Updated to 'disconnected' after .end() or .close() is called (note that failures can eventually call this method).

Updated to 'protocol_error' on fatal or unexpected protocol errors.

Tests include:

Connects successfully and sets state to connected.

Closes cleanly and sets state to disconnected.

Incorrect credentials result in state protocol_error.

Let me know what you think, and tell me if any changes should be made -- along if we need more tests as well :)

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.51%. Comparing base (52bb6e4) to head (bcb70ab).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
lib/base/connection.js 88.88% 1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@ ## master #3885 +/- ## ========================================== + Coverage 89.78% 90.51% +0.73%  ========================================== Files 86 86 Lines 13607 13614 +7 Branches 1607 1635 +28 ========================================== + Hits 12217 12323 +106  + Misses 1390 1291 -99 
Flag Coverage Δ
compression-0 89.62% <88.88%> (+0.73%) ⬆️
compression-1 90.49% <88.88%> (+0.73%) ⬆️
static-parser-0 88.09% <88.88%> (+0.73%) ⬆️
static-parser-1 88.85% <88.88%> (+0.73%) ⬆️
tls-0 89.89% <77.77%> (+0.70%) ⬆️
tls-1 90.17% <88.88%> (+0.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Copy link
Collaborator

@wellwelwel wellwelwel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tip would be to use the sleep method instead of setting the setTimeout multiple times with a Promise, for example:

import { sleep } from 'poku'; // ... await sleep(20); // ...
Comment on lines +82 to +89
const raw = connection.connection;
// use changeUser to induce auth error (assuming user 'nope' doesn't exist)
let gotErr = null;
assert.equal(raw.state, "connected");
raw.on('error', (e) => { gotErr = e; });
try {
await connection.changeUser({ user: 'nope', password: 'bad' });
} catch (err) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const raw = connection.connection;
// use changeUser to induce auth error (assuming user 'nope' doesn't exist)
let gotErr = null;
assert.equal(raw.state, "connected");
raw.on('error', (e) => { gotErr = e; });
try {
await connection.changeUser({ user: 'nope', password: 'bad' });
} catch (err) {
const raw = connection.connection;
let gotErr = null;
assert.equal(raw.state, "connected");
raw.on('error', (e) => { gotErr = e; });
try {
await connection.changeUser({ user: 'NON_EXISTENT_USER', password: 'BAD_PASSWORD' });
} catch (err) {
assert.equal(badConnection.connection.state, 'protocol_error');
} finally {
try {
await bad.end();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await bad.end();
await badConnection.end();
});

await it('state is protocol_error if creds are wrong.', async () => {
const badConnection = common.createConnection({ password: "WR0NG", user: 'wrong' }).promise();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const badConnection = common.createConnection({ password: "WR0NG", user: 'wrong' }).promise();
const badConnection = common.createConnection({ password: "WR0NG_PASSWORD", user: 'WRONG_USER' }).promise();
@wellwelwel wellwelwel added mysqljs/mysql incompatibilities Previously: feligxe-mysql-incompatibilities and removed needs documentation labels Oct 24, 2025
@HappyZombies
Copy link
Contributor Author

I just realized I forgot to add the "authorized" state, so I will be adding that and also fixing the unit tests as per your recommendation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mysqljs/mysql incompatibilities Previously: feligxe-mysql-incompatibilities

2 participants