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 2022/07/26 18:07:11 UTC
[GitHub] [airflow] fritz-astronomer opened a new issue, #25317: `postgres://` vs `postgresql://` inconsistency
fritz-astronomer opened a new issue, #25317:
URL: https://github.com/apache/airflow/issues/25317
### Apache Airflow version
2.3.3 (latest released)
### What happened
Airflow is inconsistent regarding it's treatment of `postgres` vs `postgresql`
`postgresql` is correct for a URI, however `postgres` is also fine for a "Conn Type".
This at the very least causes confusion, if not errors, depending on how a connection is accessed (correct via `PostgresHook`, incorrect via `BaseHook`).
It is also confusing as Airflow maybe be changing the connection URI supplied, without any notice/reason/explanation
https://github.com/apache/airflow/blob/main/airflow/models/connection.py#L181-L188
`Connection`
``` def __init__( ... ):
....
if uri:
self._parse_from_uri(uri)
....
@staticmethod
def _normalize_conn_type(conn_type):
if conn_type == 'postgresql':
conn_type = 'postgres'
....
def _parse_from_uri(self, uri: str):
....
self.conn_type = self._normalize_conn_type(conn_type)
```
https://github.com/apache/airflow/blob/main/airflow/providers/postgres/hooks/postgres.py#L139-L144
`PostgresHook.get_uri`
``` def get_uri(self) -> str:
uri = super().get_uri().replace("postgres://", "postgresql://")
```
https://github.com/apache/airflow/blob/main/airflow/providers/postgres/hooks/postgres.py#L63-L64
```
conn_type = 'postgres'
hook_name = 'Postgres'
```
SQLAlchemy wants `postgresql`
```>>> print(dialects.__all__)
('firebird', 'mssql', 'mysql', 'oracle', 'postgresql', 'sqlite', 'sybase')
>>> print(sqlalchemy.__version__)
1.4.9
```
### What you think should happen instead
_No response_
### How to reproduce
_No response_
### Operating System
Any
### Versions of Apache Airflow Providers
_No response_
### Deployment
Other
### Deployment details
_No response_
### Anything else
_No response_
### Are you willing to submit PR?
- [ ] Yes I am willing to submit a PR!
### Code of Conduct
- [X] I agree to follow this project's [Code of Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish closed issue #25317: `postgres://` vs `postgresql://` inconsistency
Posted by GitBox <gi...@apache.org>.
dstandish closed issue #25317: `postgres://` vs `postgresql://` inconsistency
URL: https://github.com/apache/airflow/issues/25317
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25317: `postgres://` vs `postgresql://` inconsistency
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25317:
URL: https://github.com/apache/airflow/issues/25317#issuecomment-1195942314
I think we resolved this. Connection.get_uri is for generating airflow conn uri -- which is not the same thing as the uri that sqlalchemy uses for making a db connection (and that's what the Hook get_uri is for). So for airflow connection URI, scheme == conn_type, and that's why it's postgres not postgresql. For sqlalchemy, though, it needs postgresql, and that's what hook provides.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] fritz-astronomer commented on issue #25317: `postgres://` vs `postgresql://` inconsistency
Posted by GitBox <gi...@apache.org>.
fritz-astronomer commented on issue #25317:
URL: https://github.com/apache/airflow/issues/25317#issuecomment-1195817001
That being said - I'm not sure what to fix
* We wouldn't change `conn_type` to be `postgresql` - that seems like an unnecessarily large/breaking change, and `postgres` is a perfectly valid `conn_type`
* We wouldn't remove `postgresql://` in `PostgresHook.get_uri` - as it's required and correct for SQLAlchemy
* we can't add the `get_uri` fix to `BaseHook`, because it doesn't have the method
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mtilda commented on issue #25317: `postgres://` vs `postgresql://` inconsistency
Posted by GitBox <gi...@apache.org>.
mtilda commented on issue #25317:
URL: https://github.com/apache/airflow/issues/25317#issuecomment-1197313881
@dstandish I don't quite follow your explanation...
Can you justify the behavior of this code?
```py
from airflow.models.connection import Connection
conn = Connection(uri="postgresql://")
print(conn.get_uri())
```
Output:
```
postgres://
```
---
It is my understanding that the class `Connection` should be completely agnostic to the system the connection is for (i.e. SQLAlchemy). Why would there be a hardcoded mutation of the `conn_id`?
Let's assume I'm not using SQLAlchemy, rather some other system that requires the URI to start with `postgresql://`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25317: `postgres://` vs `postgresql://` inconsistency
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25317:
URL: https://github.com/apache/airflow/issues/25317#issuecomment-1197321528
@mtilda you are misunderstanding what the `uri` is in `Connection(uri=`
this uri is the airflow connection uri -- airflow's airflow-specific format for serializing an _airflow_ connection to a URI.
this is entirely different from the connection URI that you might use with sqlalchemy. a sqlalchemy connection is not an airflow Connection.
to generate the sqlalchemy conn uri do PostgresHook('my_conn').get_uri()
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org