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

SNOW-1708396: error: this.determineConnectionDomain is not a function when connecting (since version 1.14.0) #929

Closed
IrakliJani opened this issue Oct 3, 2024 · 13 comments
Assignees
Labels
bug Something isn't working status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. status-triage_done Initial triage done, will be further handled by the driver team

Comments

@IrakliJani
Copy link

Please answer these questions before submitting your issue.
In order to accurately debug the issue this information is required. Thanks!

  1. What version of NodeJS driver are you using?

1.14.0

  1. What operating system and processor architecture are you using?

MacOS/ARM

  1. What version of NodeJS are you using?

v20.12.1 / 10.5.0

  1. What are the component versions in the environment (npm list)?
from yarn lock

snowflake-sdk@~1.14.0:
version "1.14.0"
resolved "https://registry.yarnpkg.com/snowflake-sdk/-/snowflake-sdk-1.14.0.tgz#2db7496592b5a6680ec30d7bf1b35880a5838a2b"
dependencies:
"@aws-sdk/client-s3" "^3.388.0"
"@aws-sdk/node-http-handler" "^3.374.0"
"@azure/storage-blob" "12.18.x"
"@google-cloud/storage" "^7.7.0"
"@techteamer/ocsp" "1.0.1"
asn1.js-rfc2560 "^5.0.0"
asn1.js-rfc5280 "^3.0.0"
axios "^1.6.8"
big-integer "^1.6.43"
bignumber.js "^9.1.2"
binascii "0.0.2"
bn.js "^5.2.1"
browser-request "^0.3.3"
expand-tilde "^2.0.2"
fast-xml-parser "^4.2.5"
fastest-levenshtein "^1.0.16"
generic-pool "^3.8.2"
glob "^10.0.0"
https-proxy-agent "^7.0.2"
jsonwebtoken "^9.0.0"
mime-types "^2.1.29"
mkdirp "^1.0.3"
moment "^2.29.4"
moment-timezone "^0.5.15"
open "^7.3.1"
python-struct "^1.1.3"
simple-lru-cache "^0.0.2"
toml "^3.0.0"
uuid "^8.3.2"
winston "^3.1.0"

6.Server version:* E.g. 1.90.1

8.37.1

  1. What did you do?

We (from @lightdash) recently contributed to this SDK: #908, and yesterday, it was published on NPM as version 1.14.0. However, we've noticed our tests failing after the version bump.

We've looked into the logs and realized that there's an issue with opening a connection, and it throws this error:

error: this.determineConnectionDomain is not a function

  1. What did you expect to see?

Connection should have succeeded.

  1. Can you set logging to DEBUG and collect the logs?

    X

  2. What is your Snowflake account identifier, if any? (Optional)

X

@IrakliJani IrakliJani added the bug Something isn't working label Oct 3, 2024
@github-actions github-actions bot changed the title error: this.determineConnectionDomain is not a function when connecting (since version 1.14.0) SNOW-1708396: error: this.determineConnectionDomain is not a function when connecting (since version 1.14.0) Oct 3, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka self-assigned this Oct 4, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage Issue is under initial triage label Oct 4, 2024
@sfc-gh-dszmolka
Copy link
Collaborator

hi - thank you for reporting this issue. can you please provide a runnable code example, which exhibits the issue you're seeing? our tests did not seem to catch it, I also now used

const snowflake = require('snowflake-sdk');
snowflake.configure({ logLevel: 'trace' });
const connection = snowflake.createConnection({
    account: process.env.SFACCOUNT,
    username: process.env.SFUSER,
    password: process.env.SFPASS
});
connection.connect(function(err, conn) {
    if (err) {
        console.error('Unable to connect: ' + err.message);
    } else {
        console.log('Successfully connected as id: ' + connection.getId());
const statement = connection.execute({
    sqlText: "SELECT CURRENT_USER() as whoami;",
    complete: function(err, stmt, rows) {
        if (err) {
            console.error('Failed to execute statement due to the following error: ' + err.message);
        } else {
            console.log('[queryID ' + statement.getStatementId() + ', requestId ' + statement.getRequestId() + '] Number of rows produced: ' + rows.length);
            for (row of rows) {
	    console.log(row);
	    }
        }
    }
});
}});

to connect manually and it worked. So it would be great to get some details about how the issue happens.

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-information_needed Additional information is required from the reporter label Oct 4, 2024
@IrakliJani
Copy link
Author

hi @sfc-gh-dszmolka Thanks for responding promptly 🙌

here's the code:

https://github.com/lightdash/lightdash/blob/f7a9fc540606611bcfa831df49e987417cfa8ebe/packages/warehouses/src/warehouseClients/SnowflakeWarehouseClient.ts#L196-L197

I believe this happens either because of destructuring when importing the module or because we use promisify.

I was experimenting with the snowflake-sdk in Node modules directly, and it's definitely a binding issue.

in here https://github.com/snowflakedb/snowflake-connector-nodejs/blob/master/lib/connection/connection.js#L196-L197 when I change this to self it works just fine.

similarly here https://github.com/snowflakedb/snowflake-connector-nodejs/blob/master/lib/connection/connection.js#L270-L271

I can either provide a small example of this, or I can also make a quick PR to fix this later today.

Let me know if this is helpful

@sfc-gh-dszmolka
Copy link
Collaborator

thank you for the details - an example would definitely help. Why? Because Connection and the functions inside this looks like was always (6 years) defined like this. If there's now suddenly a binding issue after including #908 in the driver, and not before, we'll need to look into that.

So a runnable repro would help in introducing a solution which solves your particular problem, and still keeps the driver operational for everyone else.

@IrakliJani
Copy link
Author

thanks @sfc-gh-dszmolka

I will follow up with the example and possibly open a PR later today or early next week

for now I was able to fix it on our end. see the diff here: https://github.com/lightdash/lightdash/pull/11779/files#diff-749d13d2e4158294b62b12b5c584291f9d81ce75138402d5bcb13b331692c2afR197

@sfc-gh-dszmolka
Copy link
Collaborator

thank you. I would also like to see if the change coming with #908 affect anyone else and causing problems for them.
For what I know, the reason #908 was introduced was an issue only affecting a single user (correct me please if i'm wrong) .

So it would be great to see the scope of this this issue doesn't affect anyone else and is now fixed for you by explicitly adding `.bind, and make sure it doesn't affect anyone else, before we potentially introduce a breaking change for others.

@sfc-gh-dszmolka sfc-gh-dszmolka added status-triage_done Initial triage done, will be further handled by the driver team and removed status-triage Issue is under initial triage status-information_needed Additional information is required from the reporter labels Oct 4, 2024
@sfc-gh-dszmolka
Copy link
Collaborator

FYI investigating an issue (#936) from another user, potentially caused by #908 . Investigation is in progress.

@btryder
Copy link

btryder commented Oct 16, 2024

Confirming we are seeing this same error on some snowflake scripts.

2024-10-16T15:50:20.442032+00:00 app[advanced-scheduler.8789]: TypeError: this.determineConnectionDomain is not a function
2024-10-16T15:50:20.442033+00:00 app[advanced-scheduler.8789]: at Connection.connect (/app/node_modules/snowflake-sdk/lib/connection/connection.js:197:35)
2024-10-16T15:50:20.442033+00:00 app[advanced-scheduler.8789]: at node:internal/util:432:7
2024-10-16T15:50:20.442034+00:00 app[advanced-scheduler.8789]: at new Promise (<anonymous>)
2024-10-16T15:50:20.442034+00:00 app[advanced-scheduler.8789]: at node:internal/util:418:12
2024-10-16T15:50:20.442034+00:00 app[advanced-scheduler.8789]: at UpdatePortfolioCompaniesJob.snowflakeFetch (/app/server/dist/scripts/util/base-update-job.js:40:15)
2024-10-16T15:50:20.442035+00:00 app[advanced-scheduler.8789]: at UpdatePortfolioCompaniesJob.execute (/app/server/dist/scripts/update-portfolio-companies-job.js:13:41)
2024-10-16T15:50:20.442035+00:00 app[advanced-scheduler.8789]: at Object.<anonymous> (/app/server/dist/scripts/update-portfolio-companies-job.js:45:35)
2024-10-16T15:50:20.442035+00:00 app[advanced-scheduler.8789]: at Module._compile (node:internal/modules/cjs/loader:1469:14)
2024-10-16T15:50:20.442035+00:00 app[advanced-scheduler.8789]: at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
2024-10-16T15:50:20.442036+00:00 app[advanced-scheduler.8789]: at Module.load (node:internal/modules/cjs/loader:1288:32)

I could reproduce it on both v1.14.0 as well as v1.13.0, so I wouldn't limit your scope to just the changes in the latest release.

We resolved it by pinning our version to v1.11.0 for now until it's fixed.

@sfc-gh-dszmolka
Copy link
Collaborator

hi @btryder thanks for this comment ! It is very interesting, that this regression which is only seen with 1.14.0 so far, is seen in older driver versions. Please, don't use v1.13.0 as it's a little bit broken unfortunately.

Can you please provide some more information on this problem you're experiencing?

  1. i take v1.11.0 doesn't reproduce the issue for you. If you upgrade to v1.12.0, then v1.13.1, then v1.14.0 - which snowflake-sdk version is the first you're seeing the issue with?
  2. do you also use Util.promisify ? Asking because of the stack seems to reference util and a creation of a new Promise.
  3. can you please provide a runnable minimal version of the reproduction, which when run, reproduces the issue you're seeing? This would be quite helpful for the investigation.
    Thank you in advance!

@IrakliJani
Copy link
Author

just fyi, if you are using Util.promisify or passing connection.connect to a method that messes up the binding then this is a workaround:

await Util.promisify(connection.connect.bind(connection))(); -> https://github.com/lightdash/lightdash/pull/11779/files#diff-749d13d2e4158294b62b12b5c584291f9d81ce75138402d5bcb13b331692c2afR197

@sfc-gh-dszmolka
Copy link
Collaborator

hi folks - we unfortunately have evidence (#936) that #908 introduced an unexpected regression. We're going to fix that, which fix will likely involve restoring the original functionality for the heartbeat.

However, there must be a reason you raised the PR in the first place; AFAIK there was a user for whom the heartbeats did not work correctly. We were able to reproduce the error this time (with calling a Util.promisify-ed connection.connect), so could test the fix too.

I'll add the relevant PR too, but to avoid an unplanned issue after we fix the regression introduced by #908 ; could you maybe test it with either with
await Util.promisify(connection.connect.bind(connection))();

which you already using, or even
await Util.promisify(connection.connect).call(connection);

to make sure your use-case doesn't break after we fix the regression?

Here's a simple test app which we used in an environment locally, where #908 was already reverted; and could successfully connect to Snowflake even in the promisifyed way:

var connection = snowflake.createConnection({
  account: '..',
  ..
  clientConfigFile: 'sf_client_config.json' // Easy Logging
});

const connectPromise = util.promisify(connection.connect).bind(connection);

try {
  await connectPromise();
  console.log('Connected to Snowflake.');
  const connection_ID = connection.getId();
} catch (err) {
  console.error('Unable to connect: ' + err.message);
  return;
}

This worked with both the vanilla v1.10.1 against which the issue was originally reported, and the master branch as well where locally 908 was reverted.

@sfc-gh-dszmolka
Copy link
Collaborator

linking #939

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-pr_pending_merge A PR is made and is under review label Oct 23, 2024
@sfc-gh-dszmolka
Copy link
Collaborator

PR is merged and will be part of the next release

@sfc-gh-dszmolka sfc-gh-dszmolka added status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. and removed status-pr_pending_merge A PR is made and is under review labels Nov 4, 2024
@sfc-gh-dszmolka
Copy link
Collaborator

released with Snowflake Node.js driver v1.15.0 in the October 2024 release cycle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

4 participants