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/02/23 14:58:53 UTC
[GitHub] [airflow] bolkedebruin opened a new pull request #21764: Make dbapi inherit get_uri from connection
bolkedebruin opened a new pull request #21764:
URL: https://github.com/apache/airflow/pull/21764
DBApi has its own get_uri method which does not deal
with quoting properly and neither with empty passwords.
Connection also has a get_uri method that deals properly
with the above issues.
@dimberman @ashb
--
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] potiuk commented on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050969315
> Interesting! Maybe we should move away from "conn_type" etc and just rely on people specifying a URI (as happens in others systems) and allow for specifying a key or username/password?
I think connections need redesign badly. There are a number of ambiguities and duplications there (especially in-connection with extras and UI). And I also think @dimberman you already take steps in that direction too right ? :D.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it lkely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri even in DBApi. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection you have no object schema and SQLAlchemy is useless.
You can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway - so it makes no sense to instantiate the sqlalchemy engine for it.
So while we likely should keep backwards compatibility I think hardly anyone uses it or make some assumptions about returned URI correctness.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it likely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri even in DBApi. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection you have no object schema and SQLAlchemy is useless.
You can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway - so it makes no sense to instantiate the sqlalchemy engine for it.
So while we likely should keep backwards compatibility I think hardly anyone uses it or make some assumptions about returned URI correctness.
--
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] ashb commented on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050298951
I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
--
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] bolkedebruin commented on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
bolkedebruin commented on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050695156
Interesting! Maybe we should move away from "conn_type" etc and just rely on people specifying a URI (as happens in others systems) and allow for specifying a key or username/password?
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it lkely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri even in DBApi. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection you have no object schema and SQLAlchemy is useless.
You can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway - so it makes no sense to instantiate the sqlalchemy engine for it.
So while we likely should keep backwards compatibility I think hardly anyone uses it or make some assumptions about it's correctness.
--
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] github-actions[bot] commented on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1048895848
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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] ashb merged pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #21764:
URL: https://github.com/apache/airflow/pull/21764
--
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] bolkedebruin commented on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
bolkedebruin commented on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050296628
@potiuk which one did you look up? the one in dbapi was pretty buggy and I am not sure why it worked :-). The one in connection is a bit better, but I am confused why we use so little standardization here while url encoding is actually pretty hard.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it likely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri even in DBApi. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection from "Connection" object - you have no objects schema - by definition - and SQLAlchemy is well, useless.
You can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway - so it makes no sense to instantiate the sqlalchemy engine for it.
So while we likely should keep backwards compatibility (just in case) with even more "correct" URIs returned, I think hardly anyone uses it or make some assumptions about returned URI correctness.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it likely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri even in DBApi. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection from "Connection" object - you have no objects schema - by definition - and SQLAlchemy is well, useless.
You can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway - so it makes no sense to instantiate the sqlalchemy engine for it.
So while we likely should keep backwards compatibility (just in case) with even "more correct" URIs returned, I think hardly anyone uses it or make some assumptions about returned URI correctness.
--
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] potiuk commented on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it lkely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection you have no object schema and SQLAlchemy is useless (you can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method.
So while we likely should keep backwards compatibility I think hardly anyone uses it or make some assumptions about it's correctness.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it lkely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri even in DBApi. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection you have no object schema and SQLAlchemy is useless (you can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway).
So while we likely should keep backwards compatibility I think hardly anyone uses it or make some assumptions about it's correctness.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it lkely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection you have no object schema and SQLAlchemy is useless (you can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway).
So while we likely should keep backwards compatibility I think hardly anyone uses it or make some assumptions about it's correctness.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
> I have a feeling Connection.get_uri is a "relatively" recent addition, I think added for `airflow connections export` command.
Yep. It is. And I think this is simply a very straightforward manifestation of "If is is not used, it lkely does not work".
From what I looked today the "Connection" get_uri() was added in 2.0.0a.
And if you look closely - there is no "REAL" usage of get_uri even in DBApi. There is nearly no use case for it - except displaying it in the `airflow CLI` and possibly UI (which does not matter at all if it is correct or not).
The only `actual` usage of get_uri - from what I see - is "get_sqlalchemy_engine" which is actually hardly useful anyway. Getting sqlalchemy engine for a connection makes pretty much no sense. SQLAlchemy is really an ORM mapper, and when you have generic connection you have no object schema and SQLAlchemy is useless.
You can instantiate an SQLAlchemy session and run a raw query - maybe - but then you can run query via DBApi's "run" method anyway).
So while we likely should keep backwards compatibility I think hardly anyone uses it or make some assumptions about it's correctness.
--
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] potiuk edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050312924
--
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] bolkedebruin edited a comment on pull request #21764: Make dbapi inherit get_uri from connection
Posted by GitBox <gi...@apache.org>.
bolkedebruin edited a comment on pull request #21764:
URL: https://github.com/apache/airflow/pull/21764#issuecomment-1050695156
Interesting! Maybe we should move away from "conn_type" etc and just rely on people specifying a URI (as happens in others systems) and allow for specifying a key or username/password?
And btw @dimberman is relying on this function in the new sql-decorator (to be renamed I guess).
--
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