snowflake-connector-nodejs icon indicating copy to clipboard operation
snowflake-connector-nodejs copied to clipboard

Fix Util.constructHostname bug with us-west-2 region

Open jdorn opened this issue 1 year ago • 6 comments

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_FILES and 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:unit and npm 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

jdorn avatar Apr 18 '24 20:04 jdorn

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

github-actions[bot] avatar Apr 18 '24 20:04 github-actions[bot]

I have read the CLA Document and I hereby sign the CLA

jdorn avatar Apr 18 '24 20:04 jdorn

hi - per the Snowflake documentation for account identifiers, the correct notation for an account in us-west-2 is:

  1. locator version: 'abc123' (no region, nothing)
  2. 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 !

sfc-gh-dszmolka avatar Apr 19 '24 06:04 sfc-gh-dszmolka

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.

jdorn avatar Apr 21 '24 00:04 jdorn

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`

sfc-gh-pmotacki avatar May 06 '24 11:05 sfc-gh-pmotacki

Thanks for this fix @jdorn , when are you planning to release it?

thedae avatar May 24 '24 13:05 thedae