Skip to content

Remove Provider Deprecations in Common SQL #44645

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.

Sign up for GitHub

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

Merged
jscheffl merged 2 commits into from
Dec 5, 2024
Merged

Remove Provider Deprecations in Common SQL #44645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62cf7fb
Remove Provider Deprecations in Common SQL
jason810496 Dec 4, 2024
107caf7
Fix Changelog header
jason810496 Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions providers/src/airflow/providers/common/sql/CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@
Changelog
---------

main
.....

.. warning::
All deprecated classes, parameters and features have been removed from the Common SQL provider package.
The following breaking changes were introduced:

* Hooks
* Remove ``_make_serializable`` method from ``DbApiHook``. Use ``_make_common_data_structure`` instead.
Comment on lines +35 to +36
Copy link
Contributor

@eladkal eladkal Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need more context here?
To a user it might seem like we remove private internal function.
I don't know what it's used for but either it's not part of the public API and we shouldn't have deprecate it to begin with or it has user impact and we need to better explain what it is?
(To clarify since we already deprecated we will have to cut breaking release with remove not but the change log should better indicate what is the change)

similar to what I commented on #44567 (comment)
WDYT?

Copy link
Member

@potiuk potiuk Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK. So the thing is - we should basically never (unless we have a very good reason) make a breaking release of a common provider. If we do - this breaks a lot of things when it comes to provider up/downgrability. This means that if ONE provider will start using features from such "breaking change" release. other providers will have to use it as well. So if the other providers depend on the "broken" feature, they will be broken once you upgrade common.sql to the breaking version.

That's one of the difficulties with "common" - because in Python (unlike in npm or system libraries) we cannot have two versions of the same library used at the same time.

I'd say this change is "lightly breaking" - it's likely breaking single (or very few) versions of past released providers. It does not break user code, it breaks some old version of providers (and we could consider that as a bugifix. We could likely try to determine which versions of which providers were affected by this change.

There were two heated but otherwise resulting in no conclusiosns (except renaming of _make_serializable) discussion a year ago in December:

Having common.sql incompatibility and potential impact on already released providers was one of the issues I raised then as a reason why we should not rename it. We added deprecation instead.

IMHO year is long enough to consider those "old providers" are "out of fashion" and assessing that it's highly unlikely somoene will have new common.sql and very old one of the providers that depend o common.sql - with this specific _make_serializable used. It's also very easy to fix such issue (by upgrading the affected provider). So for me I'd not classify it as a breaking change but "misc" and accept the fact that some past providers might be broken.

Alternative is to add 2. version of the common.sql (and it would likely have no big impact because the same case - probability of breaking change is small) but I'd say the reason for bumping to 2.* is not too strong.

Copy link
Contributor

@eladkal eladkal Dec 10, 2024

Choose a reason for hiding this comment

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

We can also decide to exclude common.sql and do the removal at later phase

Copy link
Contributor

@eladkal eladkal Dec 10, 2024

Choose a reason for hiding this comment

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

I tend to agree that we should just tweak the message on the change log


1.20.0
......

Expand Down
12 changes: 0 additions & 12 deletions providers/src/airflow/providers/common/sql/hooks/sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
from airflow.exceptions import (
AirflowException,
AirflowOptionalProviderFeatureException,
AirflowProviderDeprecationWarning,
)
from airflow.hooks.base import BaseHook

Expand Down Expand Up @@ -502,17 +501,6 @@ def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple | list[t
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)
Expand Down
19 changes: 0 additions & 19 deletions providers/tests/common/sql/hooks/test_sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@

import logging
import logging.config
import warnings
from unittest.mock import MagicMock

import pytest

from airflow.config_templates.airflow_local_settings import DEFAULT_LOGGING_CONFIG
from airflow.exceptions import AirflowProviderDeprecationWarning
from airflow.models import Connection
from airflow.providers.common.sql.hooks.sql import DbApiHook, fetch_all_handler
from airflow.utils.session import provide_session
Expand Down Expand Up @@ -238,23 +236,6 @@ def test_no_query(self, empty_statement):
dbapi_hook.run(sql=empty_statement)
assert err.value.args[0] == "List of SQL statements is empty"

@pytest.mark.db_test
def test_make_common_data_structure_hook_has_deprecated_method(self):
"""If hook implements ``_make_serializable`` warning should be raised on call."""
hook = mock_hook(DbApiHook)
hook._make_serializable = lambda result: result
with pytest.warns(
AirflowProviderDeprecationWarning, match="`_make_serializable` method is deprecated"
):
hook._make_common_data_structure(["foo", "bar", "baz"])

@pytest.mark.db_test
def test_make_common_data_structure_no_deprecated_method(self):
"""If hook not implements ``_make_serializable`` there is no warning should be raised on call."""
with warnings.catch_warnings():
warnings.simplefilter("error", AirflowProviderDeprecationWarning)
mock_hook(DbApiHook)._make_common_data_structure(["foo", "bar", "baz"])

@pytest.mark.db_test
def test_placeholder_config_from_extra(self):
dbapi_hook = mock_hook(DbApiHook, conn_params={"extra": {"placeholder": "?"}})
Expand Down
4 changes: 2 additions & 2 deletions scripts/ci/pre_commit/check_common_sql_dependency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def is_subclass_of_dbapihook(node: ast.ClassDef) -> bool:
return False


def has_make_serializable_method(node: ast.ClassDef) -> bool:
def has_make_common_data_structure_method(node: ast.ClassDef) -> bool:
"""Return True if the given class implements `_make_common_data_structure` method."""
for body_element in node.body:
if isinstance(body_element, ast.FunctionDef) and (body_element.name == MAKE_COMMON_METHOD_NAME):
Expand Down Expand Up @@ -98,7 +98,7 @@ def check_sql_providers_dependency():
continue

for clazz in get_classes(path):
if is_subclass_of_dbapihook(node=clazz) and has_make_serializable_method(node=clazz):
if is_subclass_of_dbapihook(node=clazz) and has_make_common_data_structure_method(node=clazz):
provider_yaml_path: str = determine_provider_yaml_path(file_path=path)
provider_metadata: dict = get_yaml_content(file_path=provider_yaml_path)

Expand Down