Skip to content

Refactoring connection in httphook #53407

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

Closed
kyungjunleeme wants to merge 3 commits into from
Closed

Refactoring connection in httphook #53407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a3937f
ADD: get_conn_name and super().__init__
kyungjunleeme Jul 16, 2025
cec4e2c
CHG: get_connection in http provider
kyungjunleeme Jul 17, 2025
3a4b88a
CHG: instance_variable
kyungjunleeme Jul 17, 2025
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
2 changes: 1 addition & 1 deletion providers/apache/livy/src/airflow/providers/apache/livy/hooks/livy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def __init__(
) -> None:
super().__init__()
self.method = "POST"
self.http_conn_id = livy_conn_id
self.livy_conn_id = livy_conn_id
self.extra_headers = extra_headers or {}
self.extra_options = extra_options or {}
self.endpoint_prefix = sanitize_endpoint_prefix(endpoint_prefix)
Expand Down
6 changes: 5 additions & 1 deletion providers/http/src/airflow/providers/http/hooks/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ def auth_type(self):
def auth_type(self, v):
self._auth_type = v

def get_conn_id(self) -> str:
return getattr(self, self.conn_name_attr)

# headers may be passed through directly or in the "extra" field in the connection
# definition
def get_conn(
Expand All @@ -172,7 +175,7 @@ def get_conn(
:return: A configured requests.Session object.
"""
session = Session()
connection = self.get_connection(self.http_conn_id)
connection = self.get_connection(self.get_conn_id())
Copy link
Collaborator

@jroachgolf84 jroachgolf84 Jul 17, 2025

Choose a reason for hiding this comment

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

I don't love this approach. The previous syntax is the standard for almost all provider hooks. I don't think this change should be made.

Copy link
Contributor Author

@kyungjunleeme kyungjunleeme Jul 17, 2025

Choose a reason for hiding this comment

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

at first, Thanks ur commeting about my pr. :)

I recently worked on this PR, and I believe the same approach could be applied more broadly.

I'd like to take a bit more time to think it through, and then I'll request your review again. I think there's value in discussing it together to identify meaningful improvements.

Thank you!

Copy link
Contributor Author

@kyungjunleeme kyungjunleeme Jul 17, 2025

Choose a reason for hiding this comment

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

I plan to gradually refactor providers that currently follow this pattern:

connection = self.get_connection(self.aws_conn_id)
connection = self.get_connection(self.get_conn_id())

To support this, I've added the following method in BaseHook:

    class BaseHook:
        def get_conn_id(self) -> str:
            if hasattr(self, "conn_name_attr"):
                return getattr(self, self.conn_name_attr)
            raise NotImplementedError("Subclasses must implement get_conn_id()")

Hardcoding the connection ID attribute directly can become a maintenance burden, especially as new providers are introduced. Using a consistent, dynamic approach like get_conn_id() makes the codebase more extensible and reduces the risk of subtle bugs. I believe this change will make future development and maintenance smoother.

Copy link
Contributor

@jscheffl jscheffl Jul 17, 2025

Choose a reason for hiding this comment

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

I agree to this change. I do not understand why the hard coded <type>_conn_id was actually made and what the benefit of this is/was. I assume this is still from times of Python 2.x and is coming from Airflow 1.x-times. But this was from the past/legacy before I joined the project.

So would be cool asking some other committer with more history knowing a reason why the current construct exists. I hope there is a reason behind and not just a strange field design.

@potiuk @ashb @kaxil Do you know somebody from that point of history knowing the reason behind this design?

Copy link
Member

@potiuk potiuk Jul 18, 2025

Choose a reason for hiding this comment

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

The reason why we havexxx_conn_id is default_args.

The pattern is that you can specify one aws_conn_id in default_args and one google_conn_id and then all AWS operators in the Dag would use the former and all Google operators would use the latter. So it makes perfect sense to use it and continue using it in the future - this reason still "stands" - so we should keep having that approach - i.e. generally same xxx_conn_id per provider should be standard and we should keep it.

And conn_attr_name is defined so that Connection.get_hook() used in a number of places - for example in test_connection can properly pass the conn_id to the right __init__ parameter when it tries to instantiate the Hook dynamically.

And I am not sure if we want to add get_conn_id() in BaseHook. It does not bring a lot of value IMHO. With autocomplete in IDE, MyPy and ruff checks, i pretty much like the explicit using of self.xxx_conn_id rather than get_conn_id(). I am not sure if there are real benefits of having it for all hooks. The real benefit would be to get rid of conn_attr_name and finding the xxx_conn_id automaticaly - because that is where we have some level of duplication - same parameter name and "conn_attr_name" string.

The only reason we used get_conn_id() pattern so far was I think common.sql where it is actually needed - if you look at DbApiHook, it has this implementation:

    def get_conn_id(self) -> str:
        return getattr(self, self.conn_name_attr)

And it is needed, because all SQL operators that take DbApiHook-derived Hooks are using get_conn_id to actually retrieve the connection id in consistent way. But this is really a DbApiHook API rather than "BaseHook" API - and I am afraid moving it to BaseHook will muddy the waters and dillute that strong tie between DBApiHook and get_conn_id().

Also in general - we should be VERY careful about modifying Base classes from task.sdk. Those "task.sdk" classes are Public APIs of Airflow. I think basically anything we add there should go through discussion on devlist with justifying why we want to do it - because adding anything there will mean we will have to maintain it forever, and it will be extremely painful if we make some mistake there and release airflow with some changes that could make things difficult to maintain.

Copy link
Contributor

@jscheffl jscheffl Jul 18, 2025

Choose a reason for hiding this comment

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

Thanks for the details. Did not consider the default_args - in this light it makes perfectly sense.

self._set_base_url(connection)
session = self._configure_session_from_auth(session, connection) # type: ignore[arg-type]

Expand Down Expand Up @@ -418,6 +421,7 @@ def __init__(
retry_limit: int = 3,
retry_delay: float = 1.0,
) -> None:
super().__init__()
self.http_conn_id = http_conn_id
self.method = method.upper()
self.base_url: str = ""
Expand Down