-
Notifications
You must be signed in to change notification settings - Fork 527
Async connect context manager support - approach 2 - aenter refactored into separate prepare method. #2613
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
Async connect context manager support - approach 2 - aenter refactored into separate prepare method. #2613
Changes from all commits
c6153e6
647f3af
526e3f5
be1d0a7
53a04c1
60b5620
50372ac
4271a01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,6 @@ | |
| Error, | ||
| OperationalError, | ||
| ProgrammingError, | ||
| proxy, | ||
| ) | ||
|
|
||
| from .._query_context_cache import QueryContextCache | ||
|
|
@@ -80,6 +79,7 @@ | |
| from ._session_manager import ( | ||
| AioHttpConfig, | ||
| SessionManager, | ||
| SessionManagerFactory, | ||
| SnowflakeSSLConnectorFactory, | ||
| ) | ||
| from ._telemetry import TelemetryClient | ||
|
|
@@ -128,6 +128,7 @@ def __init__( | |
| if "platform_detection_timeout_seconds" not in kwargs: | ||
| self._platform_detection_timeout_seconds = 0.0 | ||
|
|
||
| # TODO: why we have it here if never changed | ||
| self._connected = False | ||
| self.expired = False | ||
| # check SNOW-1218851 for long term improvement plan to refactor ocsp code | ||
|
|
@@ -165,8 +166,20 @@ def __exit__(self, exc_type, exc_val, exc_tb): | |
| "'SnowflakeConnection' object does not support the context manager protocol" | ||
| ) | ||
|
|
||
| def _prepare_aenter(self) -> None: | ||
| """ | ||
| All connection changes done before entering connection context have to be done here, as we expose the same api through snowflake.connector.aio.connect() and call this function there at __aenter__ as well. | ||
| """ | ||
| pass | ||
|
Comment on lines
+169
to
+173
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was the clue of this (second) approach - to prevent future differences between with SnowflakeConnection() vs with connect(). But I think I am more in favour of the first one so I am closing this today, unless any objections are there. |
||
|
|
||
| async def __aenter__(self) -> SnowflakeConnection: | ||
| """Context manager.""" | ||
| """ | ||
| Context manager. | ||
|
|
||
| All connection changes done before entering connection context have to be done in the _prepare_aenter() method only. | ||
| We expose the same api through snowflake.connector.aio.connect() and call that method there at its __aenter__ as well, so there cannot be any logic executed here, but not there. We cannot just call conn.__aenter__() there as it contains already connected connection. | ||
| """ | ||
| self._prepare_aenter() | ||
| await self.connect() | ||
| return self | ||
|
|
||
|
|
@@ -191,10 +204,6 @@ async def __open_connection(self): | |
| use_numpy=self._numpy, support_negative_year=self._support_negative_year | ||
| ) | ||
|
|
||
| proxy.set_proxies( | ||
| self.proxy_host, self.proxy_port, self.proxy_user, self.proxy_password | ||
| ) | ||
|
|
||
| self._rest = SnowflakeRestful( | ||
| host=self.host, | ||
| port=self.port, | ||
|
|
@@ -1014,13 +1023,17 @@ async def connect(self, **kwargs) -> None: | |
| else: | ||
| self.__config(**self._conn_parameters) | ||
|
|
||
| self._http_config = AioHttpConfig( | ||
| self._http_config: AioHttpConfig = AioHttpConfig( | ||
| connector_factory=SnowflakeSSLConnectorFactory(), | ||
| use_pooling=not self.disable_request_pooling, | ||
| proxy_host=self.proxy_host, | ||
| proxy_port=self.proxy_port, | ||
| proxy_user=self.proxy_user, | ||
| proxy_password=self.proxy_password, | ||
| snowflake_ocsp_mode=self._ocsp_mode(), | ||
| trust_env=True, # Required for proxy support via environment variables | ||
| ) | ||
| self._session_manager = SessionManager(self._http_config) | ||
| self._session_manager = SessionManagerFactory.get_manager(self._http_config) | ||
|
|
||
| if self.enable_connection_diag: | ||
| raise NotImplementedError( | ||
|
|
||
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.
All of those are by default reassigned when using
@wraps(inupdate_wrapper)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.
Correct - I leaned toward this first approach proposal so I have addressed it there - ptal
#2614