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-728803: Do not replace sqlText if it is specified by the user #639

Merged
merged 22 commits into from
Oct 6, 2023

Conversation

sfc-gh-ext-simba-lf
Copy link
Collaborator

Description

Regarding issue 137

The PR fixes a special case for submitting queries with requestId where:

  1. User executes a request and the requestId is generated
  2. The request fails due to a network error
  3. Resubmitting the request with the requestId fails because the original request failed in step 2 and sqlText is replaced by the error message

With the PR, the sqlText will not be replaced if the user specifies a sqlText
So even if the original request fails, the connector will still execute the sqlText specified by the user

  • Current
1. Call INSERT using sqlText that results in a network error
Network error. Could not reach Snowflake.

2. Call INSERT again using sqlText and requestId from step 1
[
  {
    ERRORMESSAGE: 'Error retrieving query results for request id: b15ce10a-88c8-
4cc1-a574-951698da3ce2, please use RESULT_SCAN instead'
  }
]

3. Call SELECT
[]
  • With PR
1. Call INSERT using sqlText that results in a network error
Network error. Could not reach Snowflake.

2. Call INSERT again using sqlText and requestId from step 1
[ { 'number of rows inserted': 1 } ]

3. Call SELECT
[ { C1: 'hello' } ]

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

@sfc-gh-ext-simba-lf
Copy link
Collaborator Author

Added test for special case when resubmitting a request:

  1. Send an initial request that fails with an error
  2. Send another request with the requestId and the sqlText from step 1 and passes if the sqlText is not replaced

If the sqlText is replaced, the response from mock_http_client.js is an error and the test will fail

@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf marked this pull request as ready for review September 22, 2023 23:42
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf requested a review from a team as a code owner September 22, 2023 23:42
lib/connection/statement.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/mock/mock_http_client.js Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_test.js Outdated Show resolved Hide resolved
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf merged commit 1bae9ce into master Oct 6, 2023
5 checks passed
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf deleted the SNOW-728803-request-id-sqlText branch October 6, 2023 16:37
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants