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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jdorn
Copy link

@jdorn jdorn commented Apr 18, 2024

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

  • Format code according to the existing code style (run npm run lint:check -- CHANGED_FILES and fix problems in changed code)
  • 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 jdorn requested a review from a team as a code owner April 18, 2024 20:05
Copy link

github-actions bot commented Apr 18, 2024

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

@jdorn
Copy link
Author

jdorn commented Apr 18, 2024

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

@sfc-gh-dszmolka
Copy link
Collaborator

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 !

@jdorn
Copy link
Author

jdorn commented Apr 21, 2024

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.

@sfc-gh-pmotacki sfc-gh-pmotacki force-pushed the jdorn/fix-construct-hostname branch from d92e25c to b095a8b Compare April 30, 2024 13:14
@sfc-gh-pmotacki
Copy link
Collaborator

sfc-gh-pmotacki commented May 6, 2024

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:

@thedae
Copy link

thedae commented May 24, 2024

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

@sfc-gh-pmotacki sfc-gh-pmotacki force-pushed the jdorn/fix-construct-hostname branch from b095a8b to e9950b5 Compare June 18, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants