Fix Util.constructHostname bug with us-west-2 region
Description
There is a bug in the Util.constructHostname method, specifically when the region us-west-2 is used. This was a breaking change in either 1.9.x or 1.10.x (not sure which). It worked fine in version 1.8.0.
To reproduce:
const conn = createConnection({
account: "abc123.us-west-2",
username: "foo",
password: "foo",
database: "foo",
schema: "foo",
warehouse: "foo",
role: "foo"
});
conn.connect((err, conn) => {
// Never reaches this code
});
Throws an error:
Cannot read properties of undefined (reading 'endsWith')
Now, it works as expected.
Checklist
- [x] Format code according to the existing code style (run
npm run lint:check -- CHANGED_FILESand fix problems in changed code) - [x] Create tests which fail without the change (if possible)
- [ ] Make all tests (unit and integration) pass (
npm run test:unitandnpm run test:integration) - [ ] Extend the README / documentation and ensure is properly displayed (if necessary)
- [ ] Provide JIRA issue id (if possible) or GitHub issue id in commit message
All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.
I have read the CLA Document and I hereby sign the CLA
hi - per the Snowflake documentation for account identifiers, the correct notation for an account in us-west-2 is:
- locator version: 'abc123' (no region, nothing)
- regionless version: 'myorg-myaccount' (same for every region)
as a mitigation, could you please try account: "abc123" ?
however of course, i agree that we could be more resilient in treating the configuration errors thus thank you for your contribution !
Yes, using just "abc123" fixes it, but we have a multi-tenant product and some customers had previously entered "abc123.us-west-2" and it worked fine in version 1.8.0.
We updated the SDK to the latest 1.10.1 and this broke our customers dashboards. And worse, it didn't just fail to connect, it threw a syntax error (because of a typo in the code), which happened outside of our try/catch and crashed the entire Node server.
This PR doesn't introduce any new logic. It just fixes the typo in the existing code and adds unit test coverage to the function.
Hi @jdorn Thank you for commit with implementation to fix the problem. I've run a tests and one test fails, can you verify and fix it. `1) ConnectionConfig: basic NOT global url:
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
-
actual - expected
-
'https://account-123xyz.snowflakecomputing.com/'
- 'https://account-123xyz.us-west-2.snowflakecomputing.com/' + expected - actual`
Thanks for this fix @jdorn , when are you planning to release it?