-
Notifications
You must be signed in to change notification settings - Fork 133
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-502598: Add async query execution #672
Conversation
It is to confirm that issue #208 is a duplicate? |
That seems to be what issue 19 from sdks-drivers is based off |
Codecov Report
@@ Coverage Diff @@
## master #672 +/- ##
==========================================
+ Coverage 87.77% 87.80% +0.03%
==========================================
Files 60 61 +1
Lines 5628 5725 +97
==========================================
+ Hits 4940 5027 +87
- Misses 688 698 +10
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…nector-nodejs into SNOW-502598-async-query-exec
…nector-nodejs into SNOW-502598-async-query-exec
…nector-nodejs into SNOW-502598-async-query-exec
…nector-nodejs into SNOW-502598-async-query-exec
Please rebase and check all test pass. |
Ok |
return axios.request(requestOptions); | ||
let response = await axios.request(requestOptions); | ||
|
||
if (Util.isString(response['data']) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sfc-gh-ext-simba-lf - we have been refactoring and some questions arose from this part of code.
Specifically - did You ever noticed such case, that response['data'] is string, but 'content-type' headers are set to 'application/json' at the same time?
We expected axios to parse such data automatically (ad. e.g. https://masteringjs.io/tutorials/axios/json).
Could You provide some reproduction for such scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, the tests added with the PR can help repro that scenario. Or a simpler repro would be something like:
connection.execute({
sqlText: "select 1;",
asyncExec: true,
complete: async function (err, stmt) {
queryId = stmt.getQueryId();
await connection.getQueryStatus(queryId);
}
});
The getQueryStatus
function receives a response in string that is parsed into JSON
Description
asyncExec
flag when calling executegetResultsFromQueryId()
getQueryStatus()
orgetQueryStatusThrowIfError()
isStillRunning()
andisAnError()
Checklist
npm run lint:check -- CHANGED_FILES
and fix problems in changed code)npm run test:unit
andnpm run test:integration
)