Skip to content

Cherrypicks to aio connector part8 #2458

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

Open
wants to merge 23 commits into
base: dev/aio-connector
Choose a base branch
from

Conversation

sfc-gh-pczajka
Copy link
Collaborator

@sfc-gh-pczajka sfc-gh-pczajka commented Aug 7, 2025

@sfc-gh-pczajka sfc-gh-pczajka force-pushed the cherrypicks-to-aio-connector-part8 branch from c290ee1 to 9acde0c Compare August 7, 2025 13:53
@sfc-gh-pczajka sfc-gh-pczajka marked this pull request as draft August 7, 2025 14:05
@sfc-gh-pczajka sfc-gh-pczajka marked this pull request as ready for review August 7, 2025 15:17
@sfc-gh-pczajka sfc-gh-pczajka force-pushed the cherrypicks-to-aio-connector-part7 branch from 2a0f804 to e8f0564 Compare August 11, 2025 10:49
@sfc-gh-pczajka sfc-gh-pczajka force-pushed the cherrypicks-to-aio-connector-part8 branch from 609427f to 511d01e Compare August 11, 2025 10:50
@@ -47,7 +47,6 @@ def test_valid_pat(wiremock_client: WiremockClient) -> None:
)

cnx = snowflake.connector.connect(
user="testUser",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change was missed in the async code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -74,7 +73,6 @@ def test_invalid_pat(wiremock_client: WiremockClient) -> None:

with pytest.raises(snowflake.connector.errors.DatabaseError) as execinfo:
snowflake.connector.connect(
user="testUser",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

c, "_timebomb", new_callable=MagicMock
) as mock_timerbomb:
mock_timerbomb.executed = True
with mock.patch(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this test in async code called test_timeout_query as well.
It still contains the below line:
_with mock.patch.object(c, "timebomb", mock_timebomb):

which was removed due to being identified as a source of flakiness.

Should we move this change to async code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timebombs are different objects in sync and async, so we should be fine (as long as mock.patch.object itself was flaky)

@@ -37,8 +36,6 @@

logger = getLogger(__name__)

getLogger("aiohttp").addFilter(AzureCredentialFilter())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add aiohttp to MODULES_TO_MASK_LOGS_NAMES in externals_setup.py as well as aioboto3 -> we do not control their logs and there might be some HTTP related logs which can accidently include signatures/keys

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -42,9 +45,10 @@
@pytest.mark.parametrize(
"from_path", [True, pytest.param(False, marks=pytest.mark.skipolddriver)]
)
def test_put_get_with_aws(tmpdir, conn_cnx, from_path):
def test_put_get_with_aws(tmpdir, conn_cnx, from_path, caplog):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would convert those to async as well. - since we know we are using different library (especially unlike requests - aiohttp is not using our vendored urllib3 under the hood - which would be masked using the already present MODULES_TO_MASK_LOGS_NAMES).

We should ensure in tests that async code do not leak sensitive data as well. This comment can be applied to most tests changes from the commit b2ae3e2 .

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

assert (
"sig=" not in line
expected_token_prefix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad - I meant this URL iirc: #2458 (comment)

Generally speaking we may want to test that everything is masked correctly in the Async version as well - and those tests were not applied to async.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -165,3 +168,19 @@ def test_query_large_result_set(conn_cnx, db_parameters, ingest_data):
"Expected three telemetry logs (one per query) "
"for log type {}".format(field.value)
)

aws_request_present = False
expected_token_prefix = "X-Amz-Signature="
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad - I meant this URL iirc: #2458 (comment)

Generally speaking we may want to test that everything is masked correctly in the Async version as well - and those tests were not applied to async.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@sfc-gh-pczajka sfc-gh-pczajka force-pushed the cherrypicks-to-aio-connector-part7 branch 2 times, most recently from 0a25d12 to 228f0a2 Compare August 12, 2025 08:27
Base automatically changed from cherrypicks-to-aio-connector-part7 to dev/aio-connector August 12, 2025 15:12
@sfc-gh-pczajka sfc-gh-pczajka force-pushed the cherrypicks-to-aio-connector-part8 branch from 511d01e to 24ac817 Compare August 13, 2025 11:08
@sfc-gh-pczajka sfc-gh-pczajka force-pushed the cherrypicks-to-aio-connector-part8 branch 3 times, most recently from b48434d to a513b10 Compare August 14, 2025 14:19
@sfc-gh-pczajka sfc-gh-pczajka force-pushed the cherrypicks-to-aio-connector-part8 branch from a513b10 to eb23308 Compare August 14, 2025 15:19
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.

9 participants