Skip to content

allow options to be a JSON object#12404

Merged
sushantdhiman merged 8 commits intosequelize:masterfrom
yorek:master
Jun 23, 2020
Merged

allow options to be a JSON object#12404
sushantdhiman merged 8 commits intosequelize:masterfrom
yorek:master

Conversation

@yorek
Copy link
Copy Markdown

@yorek yorek commented Jun 22, 2020 •
edited
Loading

This commit allows complex object to be passed as option in the URI connection string. For example it's now possibile to do something like:

const sequelize = new Sequelize('mssql://user:[email protected]/database?options={"encrypt":true}&anotherOption=1');

so that it will be possible to correctly connect to an Azure SQL database, as encryption is required

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 22, 2020 •
edited
Loading

Codecov Report

Merging #12404 into master will increase coverage by 6.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12404      +/-   ##
==========================================
+ Coverage   90.28%   96.44%   +6.15%     
==========================================
  Files          92       95       +3     
  Lines        8968     9114     +146     
==========================================
+ Hits         8097     8790     +693     
+ Misses        871      324     -547
Impacted Files Coverage Δ
lib/sequelize.js 95.87% <100.00%> (+0.69%) ⬆️
lib/dialects/mssql/query-interface.js 100.00% <0.00%> (ø)
lib/dialects/mssql/connection-manager.js 87.17% <0.00%> (ø)
lib/dialects/mssql/index.js 100.00% <0.00%> (ø)
lib/model.js 96.62% <0.00%> (+0.06%) ⬆️
lib/dialects/abstract/query-generator.js 97.39% <0.00%> (+1.26%) ⬆️
...dialects/abstract/query-generator/helpers/quote.js 100.00% <0.00%> (+6.66%) ⬆️
lib/dialects/mssql/data-types.js 100.00% <0.00%> (+69.87%) ⬆️
lib/dialects/mssql/async-queue.js 100.00% <0.00%> (+76.47%) ⬆️
... and 2 more

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 e33d2bd...f6da4e9. Read the comment docs.

@sushantdhiman
Copy link
Copy Markdown
Contributor

sushantdhiman commented Jun 23, 2020

Need some unit tests

sequelize/test/unit/configuration.test.js

Line 58 in c6e4192

describe('Instantiation with a URL string', () => {

sushantdhiman
sushantdhiman reviewed Jun 23, 2020
View reviewed changes
Comment thread lib/sequelize.js Outdated
sushantdhiman
sushantdhiman reviewed Jun 23, 2020
View reviewed changes
Comment thread lib/sequelize.js Outdated
@yorek
Copy link
Copy Markdown
Author

yorek commented Jun 23, 2020

Need some unit tests

sequelize/test/unit/configuration.test.js

Line 58 in c6e4192

describe('Instantiation with a URL string', () => {

Added test but for some reason it fails when run against a specific postgres test configuration, with a weird "leak" error, while it works perfectly on all others. As I'm really new to Node, any help is more than welcome.

sushantdhiman
sushantdhiman reviewed Jun 23, 2020
View reviewed changes
Comment thread test/integration/configuration.test.js Outdated
sushantdhiman
sushantdhiman reviewed Jun 23, 2020
View reviewed changes
Comment thread lib/sequelize.js Outdated
sushantdhiman
sushantdhiman approved these changes Jun 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

LGTM, in future we can accept more keys for this sort of expansion other than options

@sushantdhiman sushantdhiman merged commit 663261b into sequelize:master Jun 23, 2020
@yorek
Copy link
Copy Markdown
Author

yorek commented Jun 23, 2020

Should I port this also to v5? My understanding is that the master is the latest (v6) version, right?

@sushantdhiman
Copy link
Copy Markdown
Contributor

sushantdhiman commented Jun 23, 2020

Yes, you may open a PR for v5 branch

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

Reviewers

1 more reviewer

@sushantdhiman sushantdhiman sushantdhiman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yorek @sushantdhiman