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

Connection String in the Config #851

Closed
rubenvde opened this issue Apr 12, 2019 · 6 comments
Closed

Connection String in the Config #851

rubenvde opened this issue Apr 12, 2019 · 6 comments

Comments

@rubenvde
Copy link

Expected behaviour:

I want to use the await sql.connect("mssql://") function but change the default requestTimeout in the config file.

Actual behaviour:

There is no way to actually doing this. The config json doesn't accept connection strings.

Configuration:

Just a string starting with "mssql://"

Software versions

  • NodeJS: 11.10.0
  • node-mssql: 5.0.5
  • SQL Server: MSSQL 15.00.1000
@dhensby
Copy link
Collaborator

dhensby commented Apr 14, 2019

@rubenvde can you tell me what connection strings you've attempted to use that don't work?

@dhensby
Copy link
Collaborator

dhensby commented Apr 14, 2019

I'm going to close this as I think that there is no bug here and I'd like to know what configuration you're using that means there's "no way" to do what you want.

Note that from our connection string parser (https://github.com/tediousjs/node-mssql/blob/v5.0.5/lib/connectionstring.js#L225) you can set connectionTimeout, timeout, connect timeout, or connection timeout as options in your connection string to set the timeout value.

@dhensby dhensby closed this as completed Apr 14, 2019
@rubenvde
Copy link
Author

I checked your latest test files and found the solution by adding a setting to the connectionstring like so in the unit.js. Thanks!

@mccolljr
Copy link

mccolljr commented Jan 15, 2024

I know this is "closed", but almost 5 years later this gap still exists and I don't understand why the "manually modify the connection string" solution was acceptable to the authors or the original requester.

In the environment I'm working in, we have a connection string that is passed in through a container's environment. This connection string, from the perspective of the application, is "opaque". It may already contain a "default" request timeout value (used by all the "normal" connections in the code). In a small corner of the code meant to handle long-running operations for populating a cache, we want to override just the request timeout. As far as I can tell, this library still does not support this without manually parsing the connection string: your parser doesn't seem to be exported, and we can't pass in overrides to the ConnectionPool constructor when using a connection string. Is there truly no better solution available?

@dhensby
Copy link
Collaborator

dhensby commented Jan 15, 2024

hi @mccolljr, thanks for the feedback.

I'm not entirely sure I agree with your logic about the opaque nature of a connection string. If you're able to make a decision about wanting to set a "longer" request timeout in a small corner of your code, that corner of the code needs to be able to know (or assume) that the connection string's timeout is not long enough as it is - a decision to discard the potentially defined connection string value is still required. What if your small corner sets a request timeout that is shorter than the connection string had defined? The opaqueness of the connection string is irrelevant if you're going make domain specific decisions about the settings needed in certain circumstances.

Whilst I accept that I could see a use case for overriding specific settings with a supplementary configuration object, the main reason this doesn't exist is because no one has wanted this feature enough to submit an enhancement to the library for it.

In terms of no better solutions... there are two.

  1. The parser is exported via ConnectionPool.parseConnectionString(), so you could use that and then modify/set the desired properties
  2. Feature Request: Request.prototype.setTimeout() #1529 specifically requested a feature to set a per connection request timeout, with a PR here: feat: add ability to set per-request requestTimeout #1530 - this potentially lays a bit of a foundation for a more generic override of connections settings - however, this PR could do with some feedback from those that actually want the feature.

@mccolljr
Copy link

mccolljr commented Jan 16, 2024

I'm not entirely sure I agree with your logic about the opaque nature of a connection string. If you're able to make a decision about wanting to set a "longer" request timeout in a small corner of your code, that corner of the code needs to be able to know (or assume) that the connection string's timeout is not long enough as it is - a decision to discard the potentially defined connection string value is still required. What if your small corner sets a request timeout that is shorter than the connection string had defined? The opaqueness of the connection string is irrelevant if you're going make domain specific decisions about the settings needed in certain circumstances.

I can understand the general point, but in our case it is a matter of "the default might be up to a minute, but we know our query needs 5-8 minutes". We have knowledge of the time required by our cache filling queries, and we want to simply configure our private connection pool with a timeout that we know is sufficient, regardless of what might already be in the connection string (or not).

In terms of no better solutions... there are two.

1. The parser _is_ exported via [`ConnectionPool.parseConnectionString()`](https://github.com/tediousjs/node-mssql/blob/v10.0.1/lib/base/connection-pool.js#L98-L100), so you could use that and then modify/set the desired properties

Ah, this would be perfect. Thank you for pointing this out to me, I looked for a top-level function and did not consider it would be scoped under ConnectionPool. I'll give this a shot, I think anything that can give me the bag of options from a connection string works.

Whilst I accept that I could see a use case for overriding specific settings with a supplementary configuration object, the main reason this doesn't exist is because no one has wanted this feature enough to submit an enhancement to the library for it.

If the connection string can be parsed into a bag of options using the method you mentioned above, I think maybe it is just a matter of mentioning this in the documentation of the ConnectionPool constructor. It seems to me that this would be sufficient for 99% of use cases where someone is looking for specific overrides.

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

No branches or pull requests

3 participants