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

WIP: Add support for "options" in DatasourceURI #32833

Closed
wants to merge 2 commits into from

Conversation

strk
Copy link
Contributor

@strk strk commented Nov 13, 2019

See #32832

@strk strk requested a review from jef-n November 13, 2019 15:03
@strk strk self-assigned this Nov 13, 2019
@elpaso
Copy link
Contributor

elpaso commented Nov 13, 2019

If you want to store it see: https://github.com/qgis/QGIS/blob/master/src/providers/postgres/qgspostgresproviderconnection.cpp#L455

Also, add it to: https://github.com/qgis/QGIS/blob/master/src/providers/postgres/qgspostgresprovider.cpp#L5288

And feel free to deprecate QgsDataSourceUri: it the source of all kind of wrong assumptions and IMO it should not be in core unless completely rewritten to something like: "providerkey://provider specific uri here" and then the decode URI from provider metadata should be used.

Btw, aren't those "options" partly redundant? I mean, are you sure that they will not override other URI parameters? And if they do, which one is supposed to take precedence?

@jef-n
Copy link
Member

jef-n commented Nov 13, 2019

I also think that this moves QgsDataSourceUri further in the wrong direction - there should be less provider(/postgres) specific stuff in it, instead of more.

@strk
Copy link
Contributor Author

strk commented Nov 13, 2019

My current problem is writing a test for the fix in #32822

The test needs to use 2 different "roles" for the connection, but I'd like NOT to create a new user with login capabilities to do this. The only way I see to do so is to use PGOPTIONS to set a role which is different from the connection user, so there's no overlap.

Alternatively I could have the database setup script create a user, with a password, but this would mean exposing the target PostgreSQL cluster to a time window in which people can connect using the hard-coded username/password of the qgis test role...

@jef-n
Copy link
Member

jef-n commented Nov 13, 2019

Why not remove all the ifs that skip parameters and just let it fallthough to the setParam?

@strk
Copy link
Contributor Author

strk commented Nov 13, 2019

Why not remove all the ifs that skip parameters and just let it fallthough to the setParam?

Could work, I'll try that in pr #32822 and see if it sticks

@strk
Copy link
Contributor Author

strk commented Nov 13, 2019

Note, from Travis testing, that QgsDatasourceUri is not able to parse the escape sequence anyway:

../tests/src/core/testqgsdatasourceuri.cpp:144:37: error: unknown escape sequence '\ ' [-Werror,-Wunknown-escape-sequence]

      << "sql= table=\"\" options=-c\ role=guest "

                                    ^~

From https://travis-ci.org/qgis/QGIS/jobs/611410954#L467

So some work would be needed here anyway

@strk
Copy link
Contributor Author

strk commented Nov 13, 2019

Removing the special handling for options still "eats" the options:

431: XXXX: creating connection with uridbname='qgis_test' sslmode=disable options=-c\ role=qgis_test_role
XXX ::tables got uri: dbname='qgis_test' port=5432 sslmode=disable

somewhere those options get lost, and I guess it's wherever some class (Uri itself?) tries to separate connection-oriented parts from other parts, to determine if a pre-existing connection exists

@strk
Copy link
Contributor Author

strk commented Nov 13, 2019

Found the stripper: it is QgsDataSourceUri::connectionInfo who doesn't care about parameters, do you see that ?

In turn, QgsPostgresProviderConnection::QgsPostgresProviderConnection initializes the uri to whatever is returned from QgsDataSourceUri::connectionInfo() so that information is lost (which role to use)

@strk
Copy link
Contributor Author

strk commented Nov 13, 2019

Ok I'm closing this PR as I see you want QgsDatasourceUri deprecated

@strk strk closed this Nov 13, 2019
@strk strk deleted the datasource-options branch November 13, 2019 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants