fix(snowflake): update snowflake-sdk #18005
Conversation
WalkthroughDependency Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/snowflake/src/dialect.ts (1)
78-96: Consider adding inline documentation for the new connection options.The 19 new connection options lack inline documentation explaining their purpose. While the options themselves may be documented in the Snowflake SDK, adding brief JSDoc comments would improve developer experience when using these options through Sequelize.
Example approach:
+ /** OAuth authorization URL for external authentication */ oauthAuthorizationUrl: undefined, + /** OAuth client identifier */ oauthClientId: undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/snowflake/package.json(1 hunks)packages/snowflake/src/connection-manager.ts(1 hunks)packages/snowflake/src/dialect.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/*/src/dialect.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When adding a dialect feature, update packages/{dialect}/src/dialect.ts to declare feature support flags
Files:
packages/snowflake/src/dialect.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Upload install and build artifact (Node 20)
- GitHub Check: Upload install and build artifact (Node 18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (2)
packages/snowflake/package.json (1)
42-42: Confirm snowflake-sdk version and compatibility
[email protected] is published and not affected by any known NPM advisories; please verify that the new connection options in dialect.ts align with the Snowflake SDK’s supported API.packages/snowflake/src/connection-manager.ts (1)
37-38: Remove incorrect deprecation note foroauthHttpAllowedThe Snowflake SDK docs and release notes contain no reference to this option; remove or clarify its origin.Likely an incorrect or invalid review comment.
| oauthAuthorizationUrl: undefined, | ||
| oauthClientId: undefined, | ||
| oauthClientSecret: undefined, | ||
| oauthEnableSingleUseRefreshTokens: undefined, | ||
| oauthTokenRequestUrl: undefined, | ||
| oauthScope: undefined, | ||
| oauthRedirectUri: undefined, | ||
| oauthChallengeMethod: undefined, | ||
| workloadIdentityProvider: undefined, | ||
| workloadIdentityImpersonationPath: undefined, | ||
| workloadIdentityAzureEntraIdResource: undefined, | ||
| workloadIdentityAzureClientId: undefined, | ||
| certRevocationCheckMode: undefined, | ||
| crlAllowCertificatesWithoutCrlURL: undefined, | ||
| crlInMemoryCache: undefined, | ||
| crlOnDiskCache: undefined, | ||
| useConnectionConfigProxyForOCSP: undefined, | ||
| openExternalBrowserCallback: undefined, | ||
| queryTag: undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the new connection options exist in snowflake-sdk v2.3.1.
Ensure these options are actually supported in the target SDK version to prevent runtime errors.
🏁 Script executed:
#!/bin/bash
# Description: Verify new connection options exist in snowflake-sdk v2.3.1 ConnectionOptions type
# Search for OAuth-related option definitions in node_modules type definitions
fd -e d.ts -x rg -l "oauthAuthorizationUrl|workloadIdentityProvider|certRevocationCheckMode|queryTag" {} \; | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Download and inspect [email protected] type definitions
npm pack [email protected]
tar -xzf snowflake-sdk-2.3.1.tgz
rg -n "oauthAuthorizationUrl|oauthClientId|oauthClientSecret|oauthEnableSingleUseRefreshTokens|oauthTokenRequestUrl|oauthScope|oauthRedirectUri|oauthChallengeMethod|workloadIdentityProvider|workloadIdentityImpersonationPath|workloadIdentityAzureEntraIdResource|workloadIdentityAzureClientId|certRevocationCheckMode|crlAllowCertificatesWithoutCrlURL|crlInMemoryCache|crlOnDiskCache|useConnectionConfigProxyForOCSP|openExternalBrowserCallback|queryTag" -g '*.d.ts' packageLength of output: 14144
Revert unsupported Snowflake connection options or upgrade the snowflake-sdk dependency
Verified v2.3.1 only defines openExternalBrowserCallback, useConnectionConfigProxyForOCSP and queryTag; all other newly added options aren’t in the SDK’s types and will be undefined at runtime.
🤖 Prompt for AI Agents
In packages/snowflake/src/dialect.ts around lines 78 to 96, several Snowflake
connection options were added that the installed snowflake-sdk v2.3.1 does not
support (only openExternalBrowserCallback, useConnectionConfigProxyForOCSP and
queryTag exist); revert/remove the unsupported option properties
(oauthAuthorizationUrl, oauthClientId, oauthClientSecret,
oauthEnableSingleUseRefreshTokens, oauthTokenRequestUrl, oauthScope,
oauthRedirectUri, oauthChallengeMethod, workloadIdentityProvider,
workloadIdentityImpersonationPath, workloadIdentityAzureEntraIdResource,
workloadIdentityAzureClientId, certRevocationCheckMode,
crlAllowCertificatesWithoutCrlURL, crlInMemoryCache, crlOnDiskCache) or
alternatively upgrade the snowflake-sdk dependency and its types to a version
that exposes those fields and then update the type definitions accordingly so
runtime and typings match.
sequelize-bot
bot
added
the
conflicted
|
Putting this in draft, I will fix the merge conflicts when the other PRs have been merged |
Pull Request Checklist
Description of Changes
Updates
snowflake-sdkSummary by CodeRabbit
New Features
Refactor
Chores
Tests