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.

Sign up for GitHub

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

Jump to bottom

Add support for quoted primary key columns in uri upon import #3599

Closed
wants to merge 4 commits into from
Closed

Add support for quoted primary key columns in uri upon import #3599

wants to merge 4 commits into from

Conversation

strk
Copy link
Contributor

@strk strk commented Oct 13, 2016

@strk strk added Requires Changes! Waiting on the submitter to make requested changes Requires Tests! Waiting on the submitter to add unit tests before eligible for merging Bugfix labels Oct 13, 2016
@strk
Copy link
Contributor Author

strk commented Oct 13, 2016

Still needs further testing (including automated) and since we're at it I could add support for importing layers with multi-column primary keys

jef-n
jef-n approved these changes Oct 13, 2016
View reviewed changes
src/providers/postgres/qgspostgresprovider.cpp
for ( int fldIdx = 0; fldIdx < fields.count(); ++fldIdx )
QStringList cols = parseUriKey(primaryKey);
if ( cols.size() > 1 ) {
QgsMessageLog::logMessage( tr( "crateEmptyLayer does not support composite keys yet" ) );
Copy link
Member

Choose a reason for hiding this comment

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

s/crateEmptyLayer/createEmptyLayer/

src/providers/postgres/qgspostgresprovider.h
@@ -119,6 +119,11 @@ class QgsPostgresProvider : public QgsVectorDataProvider
static QString endianString();

/**
* Returns a lits of unquoted column names from an uri key
Copy link
Member

Choose a reason for hiding this comment

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

s/lits/list/

@strk strk added For Review and removed Requires Changes! Waiting on the submitter to make requested changes labels Oct 13, 2016
@strk
Copy link
Contributor Author

strk commented Oct 13, 2016

Typo fixed and the "unsupported multi-column key" message removed as with new commits we now support that !

I've tried with multi-column key, a FidMapped (string) key and a view, plus a normal table.
It's still weird that for a normal table you get an unquoted attribute in the Uri key while for a view or fid-mapped you get a quoted attribute instead, but with the code in this branch the importer would assume quotes are never part of the attribute name.

If I understood correctly these URI keys end up in project files, so we can't just change semantic w/out breaking backward compatibility ?

@strk strk force-pushed the master_2_createEmptyTableQuotedPkey branch from 8a0f486 to 79b30a2 Compare October 13, 2016 12:44
@strk
Copy link
Contributor Author

strk commented Oct 13, 2016

I've now also added a test, but I still don't like to see those quoted identifiers in the QgsDatasourceUri.key parameter. It's good to support them for backward compatibility, but going forward I think we should avoid the quotes in the URI.

@jef-n how about reverting daa6510 and deal with that bug on the reading side ?

@strk
Copy link
Contributor Author

strk commented Oct 13, 2016

@jef-n I have just tested, and reverting daa6510 does not reintroduce the bug in http://hub.qgis.org/issues/13710 -- clean URI with no quotes are better, aren't them ? I'll push another commit to that extent

@strk strk removed Requires Tests! Waiting on the submitter to add unit tests before eligible for merging For Review labels Oct 13, 2016
@strk strk self-assigned this Oct 13, 2016
@strk
Copy link
Contributor Author

strk commented Oct 14, 2016

Pushed as 5abdfcb and dd85796

@strk strk closed this Oct 14, 2016
@strk strk deleted the master_2_createEmptyTableQuotedPkey branch May 15, 2017 16:54
@qgib qgib mentioned this pull request May 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jef-n jef-n jef-n approved these changes

Assignees

@strk strk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@strk @jef-n