potiuk commented on a change in pull request #17423:
URL: https://github.com/apache/airflow/pull/17423#discussion_r683017452
##########
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:
> That's why I'm thinking letting both DbApiHook and any derived hooks
both set schema might be the best of both worlds here? Then the derived hooks
could stop setting schema in Airflow 3 (or the next time they get a min core
version bump really).
The problem with this is - that it has already happened that we overlooked
that the `DBApiHook.schema` object has been accessed directly by a provider,
which made it backwards incompatible with Airflow 2.1. We do not have any
mechanism to prevents this is in the future again, if we have it as a public
field in DBApiHook. This is a bit of a problem that DBAPiHook is "public API"
part and by making a public field in this class, we change the API.
The way I proposed - in the last fixup, `schema` becomes part of the DBApi
'initialization' API. If any other hook stores the schema field as self.schema
- so be it, it is "its own responsibiliity" - we make it clear now in the
constructor of the DBApiHook that passing "schema" as kwargs is THE way how to
override the schema, and by not having a public field, we clearly say that the
deriving hook cannot expect that it can change it and expect "DBApiHook"
getUri() method will use it.
--
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]
