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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants