potiuk commented on a change in pull request #17423:
URL: https://github.com/apache/airflow/pull/17423#discussion_r682983517
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -70,6 +70,7 @@ def __init__(self, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
self.connection: Optional[Connection] = kwargs.pop("connection", None)
self.conn: connection = None
+ self.schema: Optional[str] = kwargs.pop("schema", None)
Review comment:
Removed "pop" from DBApi to leave it for other hooks.
Actually I just realized the way it was implemented before - with pop() had
undesired side effect for Hooks that already used kwargs.pop() .The side
effect was that kwargs.pop() in DBApiHook would remove the schema from kwargs
in derived classes and their `self.schema = kwargs.pop("schema", None)` would
override the schema to None (!)
Example: mysql:
https://github.com/apache/airflow/blob/main/airflow/providers/mysql/hooks/mysql.py#L61
So in fact, the original change (not yet released luckily) in DBApiHook was
even more disruptive than the failed `PostgresHook`. I am actually glad it was
uncovered now, as it would be far more disruptive it was released in Airflow.
That's why we should be **extremely** careful with changing DBApiHook (and
BaseHook and similar).
--
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]
