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-1511108 SQLAlchemy heartbeat frequency parsed as string #1993

Conversation

sfc-gh-dszmolka
Copy link
Contributor

@sfc-gh-dszmolka sfc-gh-dszmolka commented Jul 5, 2024

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

When coming from (snowflake-)sqlalchemy's Engine, the client_session_keep_alive_heartbeat_frequency option seems to come as string, which then will crash the connector with

..
  File "/usr/local/lib/python3.11/site-packages/snowflake/connector/connection.py", line 1680, in _validate_client_session_keep_alive_heartbeat_frequency
    elif self.client_session_keep_alive_heartbeat_frequency > real_max:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '>' not supported between instances of 'str' and 'int'
  1. Fill out the following pre-review checklist:
  • I am adding a new automated test(s) to verify correctness of my new code
  • I am adding new logging messages
  • I am adding a new telemetry message
  • I am modifying authorization mechanisms
  • I am adding new credentials
  • I am modifying OCSP code
  • I am adding a new dependency
  1. Please describe how your code solves the related issue.

We already seem to have a protection mechanism against such occasions by converting the input to int, however when we do that, it's already too late because we already tried to compare str to int and failed with TypeError.

Functionality isn't changed, just moved the conversion to happen before the comparison to int happens.

Add the fix summary to DESCRIPTION.md
@sfc-gh-dszmolka sfc-gh-dszmolka added DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector and removed DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector labels Jul 5, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka added the DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector label Jul 8, 2024
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

Could you please modify an already existing test case to check this?

🚢 🚢

DESCRIPTION.md Outdated Show resolved Hide resolved
@sfc-gh-dszmolka
Copy link
Contributor Author

thank you again for the review @sfc-gh-mkeller ! could you please advise if skipping the 'Old Driver' test here would be okay? does it even have the change which we want to test here?

(marked it as skipped it because it was the only one failed with the change, using PythonConnector 1.9.1 and all other tests/build passed)

if you approve with this approach, i'll merge. Thank you in advance !

@sfc-gh-dszmolka sfc-gh-dszmolka merged commit 4a89dad into main Jul 15, 2024
93 checks passed
@sfc-gh-dszmolka sfc-gh-dszmolka deleted the SNOW-1511108-sqlalchemy-heartbeat-frequency-parsed-as-string branch July 15, 2024 14:31
@github-actions github-actions bot locked and limited conversation to collaborators Jul 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants