You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/06/18 21:02:56 UTC
[GitHub] [airflow] LukeHong opened a new pull request #16521: Implement get_uri() in PostgresHook
LukeHong opened a new pull request #16521:
URL: https://github.com/apache/airflow/pull/16521
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
related: #16520
Overwrite `get_uri()` from DbApiHook to use `schema` argument in PostgresHook.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655807828
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ahh, no lol. Yeah, I'm suggesting adding it as an argument to `DbApiHook` and pass it through the `__init__` call, so all subclasses can inherit this properly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16521: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r654844308
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -115,6 +116,23 @@ def get_conn(self) -> connection:
self.conn = psycopg2.connect(**conn_args)
return self.conn
+ def get_uri(self) -> str:
+ """
+ Extract the URI from the connection.
+
+ :return: the extracted uri.
+ """
+ conn = self.get_connection(getattr(self, self.conn_name_attr))
+ login = ''
+ if conn.login:
+ login = f'{quote_plus(conn.login)}:{quote_plus(conn.password)}@'
+ host = conn.host
+ if conn.port is not None:
+ host += f':{conn.port}'
+ schema = self.schema or conn.schema or ''
+ uri = urlunsplit((conn.conn_type, f'{login}{host}', schema, '', ''))
+ return uri
Review comment:
I wonder if we should do some refactoring to eliminate the duplicate code between this and `DbApiHook.get_uri()`, they are way too similar (basically only one different line). Maybe we should promote `self.schema` and this implementation to `DbApiHook`, and setting `schema` on `PostgresHook` will automatically get the correct behaviour. This also eliminates the possible bug if another database also has a schema concept but we forget to override `get_uri()` for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655743068
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute. What's the purpose of `schema` at the class level? When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #16521: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#issuecomment-864170714
Closes #16520
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655743068
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing right now because we're mixing class attribute and instance attribute. What's the purpose of `schema` at the class level? When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#issuecomment-867080493
Awesome work, congrats on your first merged pull request!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham edited a comment on pull request #16521: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
jedcunningham edited a comment on pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#issuecomment-864170714
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655807828
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ahh, no lol. Yeah, I'm suggesting adding it as an argument to `DbApiHook` and pass it through the `__init__` call, so all subclasses can all inherit this properly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655660499
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
This is better IMO, because a subclass can simply do `self.schema = None` in its `__init__` without worrying the value getting overridden by `super().__init__`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655660499
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
This is better IMO, because a subclass can simply do `self.schema = None` in its `__init__` without worrying the value getting overridden by `super().__init__`.
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ah I see, you mean we should add `schema` as an argument on `DbApiHook`, and use it to populate `self.schema`. Yes, that would be a reasonable design.
Previously I thought you were suggesting something like
```python
class DbApiHook:
def __init__(self, ...): # No schema= here.
...
self.schema = None
class PostgresHook(DbApiHook):
def __init__(self, ..., schema=None):
super().__init__(...)
self.schema = schema
```
This would be more difficult to maintain (not *too* difficult, but comparitively so).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655608121
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
I believe the schema here is actually an instance attribute so it should go inside the `__init__` call.
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
I believe the schema here should be an instance attribute and it should go inside the `__init__` call.
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing right now because we're mixing class attribute and instance attribute. What's the purpose of `schema` at the class level? When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute. What's the purpose of `schema` at the class level? When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute. I'm not sure I understand the purpose of the class level `schema.? When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute. I'm not sure I understand the purpose of the class level `schema`. When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ahh, no lol. Yeah, I'm suggesting adding it as an argument to `DbApiHook` and pass it through the `__init__` call, so all subclasses can all inherit this properly.
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ahh, no lol. Yeah, I'm suggesting adding it as an argument to `DbApiHook` and pass it through the `__init__` call, so all subclasses can inherit this properly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LukeHong commented on a change in pull request #16521: WIP: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
LukeHong commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655156357
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -115,6 +116,23 @@ def get_conn(self) -> connection:
self.conn = psycopg2.connect(**conn_args)
return self.conn
+ def get_uri(self) -> str:
+ """
+ Extract the URI from the connection.
+
+ :return: the extracted uri.
+ """
+ conn = self.get_connection(getattr(self, self.conn_name_attr))
+ login = ''
+ if conn.login:
+ login = f'{quote_plus(conn.login)}:{quote_plus(conn.password)}@'
+ host = conn.host
+ if conn.port is not None:
+ host += f':{conn.port}'
+ schema = self.schema or conn.schema or ''
+ uri = urlunsplit((conn.conn_type, f'{login}{host}', schema, '', ''))
+ return uri
Review comment:
I think that is a better way to implement this.
I'll work on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#issuecomment-867080718
@LukeHong Thanks for the contribution! :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LukeHong commented on pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
LukeHong commented on pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#issuecomment-866512818
Update: fix tests
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #16521: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r654570321
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -115,6 +116,23 @@ def get_conn(self) -> connection:
self.conn = psycopg2.connect(**conn_args)
return self.conn
+ def get_uri(self) -> str:
+ """
+ Extract the URI from the connection.
+
+ :return: the extracted uri.
+ """
+ conn = self.get_connection(getattr(self, self.conn_name_attr))
+ login = ''
+ if conn.login:
+ login = f'{quote_plus(conn.login)}:{quote_plus(conn.password)}@'
+ host = conn.host
+ if conn.port is not None:
+ host += f':{conn.port}'
+ schema = self.schema or conn.schema or ''
+ uri = urlunsplit((conn.conn_type, f'{login}{host}', schema, '', ''))
+ return uri
Review comment:
```suggestion
return urlunsplit((conn.conn_type, f'{login}{host}', schema, '', ''))
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16521: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r654131855
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -114,6 +115,25 @@ def get_conn(self) -> connection:
self.conn = psycopg2.connect(**conn_args)
return self.conn
+
+ def get_uri(self) -> str:
+ """
+ Extract the URI from the connection.
+
+ :return: the extracted uri.
+ """
+ conn = self.get_connection(getattr(self, self.conn_name_attr))
+ login = ''
+ if conn.login:
+ login = f'{quote_plus(conn.login)}:{quote_plus(conn.password)}@'
+ host = conn.host
+ if conn.port is not None:
+ host += f':{conn.port}'
+ uri = f'{conn.conn_type}://{login}{host}/'
+ schema = self.schema or conn.schema
+ if schema:
+ uri += conn.schema
+ return uri
Review comment:
`urlunsplit` would save you a lot of trouble :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LukeHong commented on a change in pull request #16521: WIP: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
LukeHong commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655156357
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -115,6 +116,23 @@ def get_conn(self) -> connection:
self.conn = psycopg2.connect(**conn_args)
return self.conn
+ def get_uri(self) -> str:
+ """
+ Extract the URI from the connection.
+
+ :return: the extracted uri.
+ """
+ conn = self.get_connection(getattr(self, self.conn_name_attr))
+ login = ''
+ if conn.login:
+ login = f'{quote_plus(conn.login)}:{quote_plus(conn.password)}@'
+ host = conn.host
+ if conn.port is not None:
+ host += f':{conn.port}'
+ schema = self.schema or conn.schema or ''
+ uri = urlunsplit((conn.conn_type, f'{login}{host}', schema, '', ''))
+ return uri
Review comment:
I think that is a better way to implement this.
I'll work on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655608121
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
I believe the schema here is actually an instance attribute so it should go inside the `__init__` call.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#issuecomment-866247788
@LukeHong can you try to rebase to master? Pylint is failing but likely not due to your changes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang merged pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang merged pull request #16521:
URL: https://github.com/apache/airflow/pull/16521
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LukeHong commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
LukeHong commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r656058536
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
I've made some change at latest commit:
1. Move schema to instance attribute in DbApiHook
2. Set `self.schema` from kwargs or None in `DbApiHook.__init__`
3. Remove the code same as (2) in `PostgresHook.__init__`
I think those are what you discussed above?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LukeHong commented on a change in pull request #16521: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
LukeHong commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r654156094
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -114,6 +115,25 @@ def get_conn(self) -> connection:
self.conn = psycopg2.connect(**conn_args)
return self.conn
+
+ def get_uri(self) -> str:
+ """
+ Extract the URI from the connection.
+
+ :return: the extracted uri.
+ """
+ conn = self.get_connection(getattr(self, self.conn_name_attr))
+ login = ''
+ if conn.login:
+ login = f'{quote_plus(conn.login)}:{quote_plus(conn.password)}@'
+ host = conn.host
+ if conn.port is not None:
+ host += f':{conn.port}'
+ uri = f'{conn.conn_type}://{login}{host}/'
+ schema = self.schema or conn.schema
+ if schema:
+ uri += conn.schema
+ return uri
Review comment:
ok, thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655743068
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute. I'm not sure I understand the purpose of the class level `schema.? When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655608121
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
I believe the schema here should be an instance attribute and it should go inside the `__init__` call.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655786768
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ah I see, you mean we should add `schema` as an argument on `DbApiHook`, and use it to populate `self.schema`. Yes, that would be a reasonable design.
Previously I thought you were suggesting something like
```python
class DbApiHook:
def __init__(self, ...): # No schema= here.
...
self.schema = None
class PostgresHook(DbApiHook):
def __init__(self, ..., schema=None):
super().__init__(...)
self.schema = schema
```
This would be more difficult to maintain (not *too* difficult, but comparitively so).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16521: Implement get_uri() in PostgresHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#issuecomment-863738661
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #16521: Add schema override in DbApiHook
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655743068
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific params? Also, all other superclass shouldn't worry about `self.schema = <schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute. I'm not sure I understand the purpose of the class level `schema`. When we do `self.schema = None` in subclass, the class attribture `schema` will simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org