- Notifications
You must be signed in to change notification settings - Fork2k
feat(adapter-mssql): support pool idle timeout and min connections#28813
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR adds support for configuring pool idle timeout and minimum connections via connection string parameters for the MSSQL adapter. It extends the existing pool configuration capabilities to includepoolIdleTimeout andpoolMinConnections, while also addingpoolMaxConnections as an alias for the existingconnectionLimit parameter.
Key changes:
- Added parsing for
poolIdleTimeoutconnection string parameter (converted from seconds to milliseconds) - Added parsing for
poolMinConnectionsconnection string parameter - Added
poolMaxConnectionsas an alias forconnectionLimit
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/adapter-mssql/src/connection-string.ts | Implements parsing logic for new pool configuration parameters with validation and unit conversion |
| packages/adapter-mssql/src/connection-string.test.ts | Adds test coverage for the three new/updated connection string parameters |
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
| constpoolIdleTimeout=firstKey(parameters,'poolIdleTimeout') | ||
| if(poolIdleTimeout!==null){ | ||
| consttimeout=parseInt(poolIdleTimeout,10) | ||
| if(isNaN(timeout)){ | ||
| thrownewError(`Invalid pool idle timeout:${poolIdleTimeout}`) | ||
| } | ||
| config.pool=config.pool||{} | ||
| config.pool.idleTimeoutMillis=timeout*1000 | ||
| } |
CopilotAIDec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The validation logic for pool timeout parameters doesn't check for negative values. Consider adding validation to ensure timeout values are positive, similar to how you might validate other numeric constraints. For example:if (isNaN(timeout) || timeout < 0) { throw new Error(...) }
| constpoolMinConnections=firstKey(parameters,'poolMinConnections') | ||
| if(poolMinConnections!==null){ | ||
| constmin=parseInt(poolMinConnections,10) | ||
| if(isNaN(min)){ | ||
| thrownewError(`Invalid pool min connections:${poolMinConnections}`) | ||
| } | ||
| config.pool=config.pool||{} | ||
| config.pool.min=min | ||
| } |
CopilotAIDec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The validation forpoolMinConnections doesn't check for negative values. Pool minimum connections should be a non-negative integer. Consider adding validation:if (isNaN(min) || min < 0) { throw new Error(...) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have addedisNaNOrNegative in a second commit.
84e8621 to642d754Compare
Uh oh!
There was an error while loading.Please reload this page.
This PR updates parseConnectionString to support more pool config parameters from a connection string. This includes
idleTimeoutMillisandmin(connections), the new connection string values are:connectionLimit)