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

Fix: force timeouts to integer types #226

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

andrewargeros
Copy link

@andrewargeros andrewargeros commented Jun 14, 2024

Description

literally just 2 int() casts for various cases where timeout and idp_response_timeout are read in as strings, but should be integers

Motivation and Context

Since sqlalchemy (and other packages) force url parameters to be passed as strings to redshift_connector, we can run into basic and avoidable issues from this package when connecting to Redshift via SQLAlchemy via the BrowserAzureOAuth2CredentialsProvider method.

Testing

Built package, connected, fixed the bug.

Screenshots (if appropriate)

code to reproduce:

from sqlalchemy import create_engine
engine = create_engine(
"redshift+redshift_connector://{HOST}:5439/{DATABASE}credentials_provider=BrowserAzureOAuth2CredentialsProvider&client_id={CLIENT}&idp_tenant={IDP_TENANT}&scope={SCOPE}/jdbc_login&idp_response_timeout=500&timeout=900000"
)
with engine.connect() as conn:
    result = conn.execute("SELECT 1")
    print(result.fetchone())

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • Local run of ./build.sh succeeds
  • Code changes have been run against the repository's pre-commit hooks
  • Commit messages follow Conventional Commit Specification
  • I have read the README document
  • I have added tests to cover my changes
  • I have run all unit tests using pytest test/unit and they are passing.
  • By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@andrewargeros
Copy link
Author

@Brooke-white not sure what the best way to request review on this is.

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.

1 participant