Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use sqlalchemy_url property in get_uri for postgresql provider #38831

Merged
merged 23 commits into from
May 9, 2024

Conversation

rawwar
Copy link
Contributor

@rawwar rawwar commented Apr 8, 2024

closes: #38187
related : #38195

Update get_uri to return URI in sqlAlchemy URI format.

Signed-off-by: kalyanr <kalyan.ben10@live.com>
Signed-off-by: kalyanr <kalyan.ben10@live.com>
@rawwar rawwar marked this pull request as ready for review April 8, 2024 15:05
@rawwar rawwar changed the title fix: Build correct SqlAlchemy URI fix: update get_uri to return URI in sqlAlchemy URI format Apr 8, 2024
@rawwar
Copy link
Contributor Author

rawwar commented Apr 8, 2024

@Taragolis , based on your comment here

As far as i remember, we recently discuss this case in slack. I guess better introduce sa_uri property (or some similar naming) which returns sqlalchemy.engine.URL into the DbApiHook without any implementation, e.g. raise NotImplementedError and implements it per hook which have implementation for SQAlchemy.

I guess we leave the implementation of get_uri dependent on the provider type. If so, do we really need to have sa_uri property with NotImplementedError? Since SqlAlchemy URI is pre-defined, we can consider using the connection and try to form the URI. Raise an error if it has issues. What do you think?

EDIT: I updated the PR according to your original comments.

@rawwar rawwar marked this pull request as draft April 8, 2024 15:36
@rawwar rawwar marked this pull request as ready for review April 8, 2024 15:51
@rawwar rawwar requested a review from eladkal as a code owner April 8, 2024 15:51
Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DBApiHook.get_uri has ambiguous and not well documented usage, so there is consideration that it might be any of the usage of this property, depends on implementations. There is 24 Hooks (±couple) right now so it turned into the hell all implementations of DBApiHook which I guess should be more or less interface/protocol implementation rather than actual implementation, except generic things, if it even possible

The problem right now that every single change in DBApiHook spread across many of the implementations and we do not have any tools and methods to control that.

I would rather do not mix-in introduce changes into DBApiHook and other hook in the same PR unless absolutely necessary. So I would rather create a new propery (via separate PR), describe where it could be used right now and where it could be used in the future.

After start implements into the other hooks with potential usage in particular this hook. And after it implements in many (more that 50% I guess) community supported hooks we might start use it internally in DBApiHook, e.g. obtain SA Connections, pandas dataframes and others, with backward compatible logic to the previous implementation for prevent case if hook do not implements this one and for some unknown reason it work (or not) previously. So we could reduce number of potential mistakes which spread across other hooks.

If we talk about this PR, it is not clear what at this moment it fix, new property it is fine, but there is no any description about this property and how to use it.

airflow/providers/common/sql/hooks/sql.py Outdated Show resolved Hide resolved
@rawwar
Copy link
Contributor Author

rawwar commented Apr 9, 2024

I would rather do not mix-in introduce changes into DBApiHook and other hook in the same PR unless absolutely necessary. So I would rather create a new propery (via separate PR), describe where it could be used right now and where it could be used in the future.

I understand. I will first make the updates to the DBApiHook and then go ahead with the individual provider updates.

If we talk about this PR, it is not clear what at this moment it fix, new property it is fine, but there is no any description about this property and how to use it.

I understand. Thanks for the feedback. What I intended to do is as you described here . To have a property sa_uri in the DBApiHook which doesn't have any implementation. But, rather a placeholder property that should be implemented in the providers that use SqlAlchemy URI. This property returns the sqlalchemy.engine.URL object which can be used by the get_uri to form a SqlAlchemy-based URI. Hope I am making sense.

For now, I will separate out the PR.

@rawwar rawwar changed the title fix: update get_uri to return URI in sqlAlchemy URI format fix: use sqlalchemy_url property in get_uri for postgresql provider Apr 14, 2024
@rawwar rawwar requested a review from Taragolis April 14, 2024 13:57
@rawwar
Copy link
Contributor Author

rawwar commented May 7, 2024

@Taragolis , all tests passed. can this be merged?

@Lee-W Lee-W self-requested a review May 8, 2024 02:18
@Taragolis Taragolis merged commit 4ada175 into apache:main May 9, 2024
40 checks passed
@rawwar rawwar deleted the kalyan/connections/38187 branch May 9, 2024 08:12
RodrigoGanancia pushed a commit to RodrigoGanancia/airflow that referenced this pull request May 10, 2024
…er (apache#38831)

* update get_uri

* update get_uri

Signed-off-by: kalyanr <kalyan.ben10@live.com>

* update docstring

Signed-off-by: kalyanr <kalyan.ben10@live.com>

* add and use sa_uri property

* update database in sa_uri

* update tests

* remove client_encoding from test_get_uri

* use sqlalchemy_url property

* add default port

* update tests

* update usage of ports

* revert client_encoding updates

---------

Signed-off-by: kalyanr <kalyan.ben10@live.com>
pateash pushed a commit to pateash/airflow that referenced this pull request May 13, 2024
…er (apache#38831)

* update get_uri

* update get_uri

Signed-off-by: kalyanr <kalyan.ben10@live.com>

* update docstring

Signed-off-by: kalyanr <kalyan.ben10@live.com>

* add and use sa_uri property

* update database in sa_uri

* update tests

* remove client_encoding from test_get_uri

* use sqlalchemy_url property

* add default port

* update tests

* update usage of ports

* revert client_encoding updates

---------

Signed-off-by: kalyanr <kalyan.ben10@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection to Postgres failed due to special characters in a password
2 participants