dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560828589
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+ """Context manager that suppresses the given exceptions and logs a warning
message."""
+ try:
+ yield
+ except exceptions as e:
+ warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
@jarek @Taragolis It could be nice thought to have the suppress_and_warn
method in the common sql provider, as we could then also reuse it (after some
refactoring allowing a custom warning message to be passed) in the following
method and then we would be DRY (e.g. not repeating ourselves):
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with contextlib.suppress(AttributeError):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
warnings.warn(
"The `_make_serializable` method is deprecated and support
will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
AirflowProviderDeprecationWarning,
stacklevel=2,
)
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
It could become this then:
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with suppress_and_warn(
AttributeError,
message="The `_make_serializable` method is deprecated and
support will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
stacklevel=2,
):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
What do you think? Ofc drawback is the dependency, but all db related hooks
are already dependent on the common sql anyway no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
