Add property conn_name and connection to dbapi_hook

[zhongjiajie]

---

Issue link: WILL BE INSERTED BY boring-cyborg

Make sure to mark the boxes below before creating PR: [x]

• Description above provides context of the change
• Unit tests coverage for changes (not needed for documentation changes)
• Commits follow "How to write a good git commit message"
• Relevant documentation is updated including usage instructions.
• I will engage committers as explained in Contribution Workflow Example.

---

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

[zhongjiajie]

We have one subclass hook still WIP in #7901

[zhongjiajie]

For reviewer, in cf0a5f6 I just add property conn_name to get conn_name.

And then I find out we could change conn = self.get_connection(self.conn_name) to conn = self.get_connection() due to self.conn_name almost same in subclass of DBAPI and is no need, So I add function get_connection in dbapi_hook to get conn_name if no conn_id pass from subclass. In the last change in e5aa2a4

[potiuk]

It looks good. I would like to understand though why it was done like that in the first place. seems unnecessary complex - maybe others (@feluelle @mik-laj can take a look and shed some light on it ) :)

[feluelle]

It looks good. I would like to understand though why it was done like that in the first place. seems unnecessary complex - maybe others (@feluelle @mik-laj can take a look and shed some light on it ) :)

I agree with you Jarek. I also don't know why it is as it is. 🤔

[zhongjiajie]

My laptop almost power off and I forget the charger in office, have to continue in tomorrow.

[codecov-io]

Codecov Report

Merging #7903 into master will decrease coverage by 55.04%.
The diff coverage is 11.42%.

@@             Coverage Diff             @@
##           master    #7903       +/-   ##
===========================================
- Coverage   88.31%   33.26%   -55.05%     
===========================================
  Files         935      935               
  Lines       45170    45170               
===========================================
- Hits        39892    15027    -24865     
- Misses       5278    30143    +24865     

Impacted Files	Coverage Δ
airflow/providers/apache/druid/hooks/druid.py	24.35% <0.00%> (-65.39%)	⬇️
airflow/providers/apache/hive/hooks/hive.py	13.88% <0.00%> (-63.64%)	⬇️
airflow/providers/apache/pinot/hooks/pinot.py	19.23% <0.00%> (-72.31%)	⬇️
...low/providers/elasticsearch/hooks/elasticsearch.py	21.62% <0.00%> (-24.54%)	⬇️
airflow/providers/jdbc/hooks/jdbc.py	47.36% <0.00%> (-52.64%)	⬇️
airflow/providers/microsoft/mssql/hooks/mssql.py	61.90% <0.00%> (-14.29%)	⬇️
...rflow/providers/microsoft/mssql/operators/mssql.py	42.42% <0.00%> (-48.21%)	⬇️
airflow/providers/mysql/hooks/mysql.py	19.79% <0.00%> (-72.92%)	⬇️
airflow/providers/odbc/hooks/odbc.py	25.55% <0.00%> (-66.67%)	⬇️
airflow/providers/oracle/hooks/oracle.py	9.40% <0.00%> (-86.33%)	⬇️
... and 822 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dafdd0...124c19d. Read the comment docs.

[zhongjiajie]

And for now, the solution two pass CI, and I think it work.

The question is, should we take solution one and change get_connection in dbapi_hook to property connection? If we use solution one, subclass of dbapi_hook have to use self.connection to get connection which is different from subclass from base_hook(using get_connection)

But if we take solution two, get_connection don't pass mypy check, and self.get_connection and DbApiHook.get_connection call different method, I think it will confuse user when calling it

[zhongjiajie]

Two solutions CI passed, we have two fix solutions here, I prefer the second one, and it also the PR final status

• Add # type: ignore in L76 in dbapi_hook to avoid error: Signature of "get_connection" incompatible with supertype "BaseHook"

-    def get_connection(self, conn_id: Optional[str] = None) -> Connection:
+    def get_connection(self, conn_id: Optional[str] = None) -> Connection:  # type: ignore

• Using property function connection in dbapi_hook to get connection, which complete fix Signature error, and make dbapi_hook more easier.

[zhongjiajie]

Reopen this stale PR

[zhongjiajie]

@feluelle @potiuk @kaxil @kaxil @turbaszek I think I need one approving review here. I checked and find out there no DbApiHook subclass add during this time, which mean this patch cover/change all DbApiHook subclass.

[zhongjiajie]

Ok, we have +1 here, if no one disapproval here in a few day I will merge this patch to master.

[feluelle]

Please rebase once again - just to be sure. :)

It is very long ago since you last pushed a commit.

[zhongjiajie]

Thanks @turbaszek @feluelle, now I rebase and push to restart the CI, will merge when it pass

[feluelle]

Actually I am a bit confused by all these connection, _connection, ... parameters.

Wouldn't it be easier if we just use the conn_id like we do in other hooks which are not based on dbapi_hook ?

[feluelle]

Suggested change

	def connection_(self):
	def connection(self):

You can overwrite it.

[feluelle]

This is unrelated.

[zhongjiajie]

I have to change it to pass CI, I have no idea why previous code could pass the CI, mypy will raise error cause aws_access_key_id, aws_secret_access_key maybe not define

[feluelle]

Please use the Connection class to init a connection like in the other cases.

For example:

self.conn = mock.Mock(return_value=Connection(...))

[feluelle]

So my suggestion is to remove conn_name_attr and add a *_conn_id parameter to init. WDYT @turbaszek @zhongjiajie ?

As it is now I am not a fan of it anymore I am sorry @zhongjiajie but this looks really too complex, don't you think?

[zhongjiajie]

I think we should keep conn_name_attr but change property name which confused users, like connection as you said above. Cause we need dbapi_hook as parent class to handle some RDBMS like hook.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ashb]

@zhongjiajie Are you going to carry on with this PR? Where did we get to with it when we left off?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.