Refactoring connection in httphook

[kyungjunleeme]

While investigating the root cause of issue #53371, I noticed a part of the code that could benefit from refactoring, similar to what was done in PR #53335.

Instead of directly accessing a specific attribute, it's more maintainable and consistent with the existing codebase to dynamically retrieve the attribute name. This approach improves consistency and makes the code easier to maintain in the long run.

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[kyungjunleeme]

There seem to be other parts that could be improved as well — I'm currently reviewing them.
I'm also taking a closer look at get_conn_id() and considering improvements related to caching.

[kyungjunleeme]

I think this is a great opportunity to clearly understand the roles of conn_id and conn_name.

[jroachgolf84]

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.

[kyungjunleeme]

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!

[kyungjunleeme]

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.

[jscheffl]

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?

[potiuk]

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.

[jscheffl]

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

[kyungjunleeme]

@jscheffl hello:)
Since I inspired by ur recent work. I mentioned you.(plz understand)

If you have a moment, I’d really appreciate hearing your thoughts or feedback on this approach and the changes I’m working on.

[kyungjunleeme]

Your comment gave me a lot to think about. I realize now that my reasoning was lacking in some areas. I’ll go ahead and close this PR.

Thank you for sharing your thoughts. all