Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix tedious connect deprecation #12182

Merged
merged 3 commits into from Apr 28, 2020
Merged

Fix tedious connect deprecation #12182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a4f7bd
fix(mssql): call connect if state is INITIALIZED
UziTech Apr 27, 2020
cdf315e
test: add state to connection stub
UziTech Apr 27, 2020
84b6c5c
test: create unit test for INITIALIZED
UziTech Apr 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/dialects/mssql/connection-manager.js
View file
Open in desktop
Expand Up @@ -61,6 +61,9 @@ class ConnectionManager extends AbstractConnectionManager {
try {
return await new Promise((resolve, reject) => {
const connection = new this.lib.Connection(connectionConfig);
if (connection.state === connection.STATE.INITIALIZED) {
connection.connect();
}
Comment on lines +64 to +66
Copy link

Choose a reason for hiding this comment

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

@UziTech I don't think you should be checking against state here. Just calling .connect should be just fine. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless the user is using an old version of tedious where calling connect would throw an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this Code leads to an unhandled exception if used with msnodesqlv8 instead of tedious.

connection.state is undefined as well as connection.STATE, so it will throw a cannot read property INITIALIZED of undefined Error.

Could you please check if connection.STATE is defined before accessing INITIALIZED? Checking if .connect exists would not be suitable because msnodesqlv8 does not require the .connect() call at all, this is done in the Constructor already.

Copy link

Choose a reason for hiding this comment

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

@killerfurbel This code is not supposed to work with msnodesqlv8 in the first place. It has a completely different API than tedious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? See https://github.com/TimelordUK/node-sqlserver-v8#use-with-sequelize ... and why does sequelize support the dialectModule property in the constructor, if tedious is the only choice?
I agree, tedious is built in, therefore it is possibly the first choice for most users. But it is possible to use others and this code breaks it... I don't think it has a completely different API - it has a specific sequelize connector (https://github.com/TimelordUK/node-sqlserver-v8/blob/master/lib/sequelize/connection.js).

Copy link

Choose a reason for hiding this comment

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

I think the main idea behind dialectModule is so you can e.g. use e.g. a forked version of tedious or any other connector used by sequelize.

The way that msnodesqlv8 build compatibility for sequelize "works", but I'm not sure that is a robust approach. Shouldn't there be a separate dialect for msnodesqlv8 if that is supposed to be a fully supported option?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sequelize is not designed to work with msnodesqlv8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this code breaks it... I don't think it has a completely different API

If it wants to keep working with sequelize it could add a STATE property.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created an issue in the msnodesqlv8 repo: TimelordUK/node-sqlserver-v8#159

However, since you backported this behavior also to v5, I would strongly vote for a check within sequelize or revert the commit in the v5 branch. I think "removing" a depreciation warning by implementing a breaking change is not reasonable and should only happen in a major release (--> 6.x).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could create a PR to check for STATE. That would probably be the best chance of getting it changed.

connection.queue = new AsyncQueue();
connection.lib = this.lib;

Expand Down
28 changes: 28 additions & 0 deletions test/unit/dialects/mssql/connection-manager.test.js
View file
Open in desktop
Expand Up @@ -39,6 +39,8 @@ if (dialect === 'mssql') {

it('connectionManager._connect() does not delete `domain` from config.dialectOptions', function() {
this.connectionStub.returns({
STATE: {},
state: '',
once(event, cb) {
if (event === 'connect') {
setTimeout(() => {
Expand All @@ -58,6 +60,8 @@ if (dialect === 'mssql') {

it('connectionManager._connect() should reject if end was called and connect was not', function() {
this.connectionStub.returns({
STATE: {},
state: '',
once(event, cb) {
if (event === 'end') {
setTimeout(() => {
Expand All @@ -75,5 +79,29 @@ if (dialect === 'mssql') {
expect(err.parent.message).to.equal('Connection was closed by remote server');
});
});

it('connectionManager._connect() should call connect if state is initialized', function() {
const connectStub = sinon.stub();
const INITIALIZED = { name: 'INITIALIZED' };
this.connectionStub.returns({
STATE: { INITIALIZED },
state: INITIALIZED,
connect: connectStub,
once(event, cb) {
if (event === 'connect') {
setTimeout(() => {
cb();
}, 500);
}
},
removeListener: () => {},
on: () => {}
});

return this.instance.dialect.connectionManager._connect(this.config)
.then(() => {
expect(connectStub.called).to.equal(true);
});
});
});
}