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.

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

Fix tedious connect deprecation #12182

Merged
merged 3 commits into from Apr 28, 2020
Merged

Conversation

UziTech
Copy link
Contributor

@UziTech UziTech commented Apr 27, 2020

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Call connect after creating tedious connection if required.

Fixes #12181

@UziTech
Copy link
Contributor Author

@UziTech UziTech commented Apr 27, 2020

I'm not sure how to test this since it would require installing different versions of tedious.

Edit: I just created a unit test to tell if connect is called when state is initialized. I'm not sure if that is a great test.

@UziTech UziTech changed the title fix(mssql): call connect if state is INITIALIZED Fix tedious connect deprecation Apr 27, 2020
@codecov
Copy link

@codecov codecov bot commented Apr 27, 2020

Codecov Report

Merging #12182 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12182   +/-   ##
=======================================
  Coverage   96.33%   96.34%           
=======================================
  Files          95       95           
  Lines        9179     9181    +2     
=======================================
+ Hits         8843     8845    +2     
  Misses        336      336           
Impacted Files Coverage Δ
lib/dialects/mssql/connection-manager.js 87.17% <100.00%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1e451e...84b6c5c. Read the comment docs.

@sushantdhiman sushantdhiman merged commit 5dcc0d5 into sequelize:master Apr 28, 2020
4 checks passed
@RodrigoHamuy
Copy link

@RodrigoHamuy RodrigoHamuy commented May 18, 2020

Thanks for this fix. When can we expect it to be on v5? At the moment it has been added only to v6. Thanks.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented May 18, 2020

If someone opens a PR for v5, it will get auto released on merge

@UziTech
Copy link
Contributor Author

@UziTech UziTech commented May 19, 2020

this fix is available in v5.21.10

@RodrigoHamuy
Copy link

@RodrigoHamuy RodrigoHamuy commented May 19, 2020

Thanks!

if (connection.state === connection.STATE.INITIALIZED) {
connection.connect();
}

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

@UziTech UziTech May 30, 2020

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

@killerfurbel killerfurbel Jun 4, 2020

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.

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

@killerfurbel killerfurbel Jun 4, 2020

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).

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

@sushantdhiman sushantdhiman Jun 5, 2020

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

@UziTech UziTech Jun 5, 2020

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

@killerfurbel killerfurbel Jun 5, 2020

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

@UziTech UziTech Jun 5, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants