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

[bugfix][postgres] Browser panel D&D a layer onto a postgresql connection tree … #5694

Merged
merged 2 commits into from
Nov 23, 2017

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Nov 22, 2017

…node icon fails without notice and leaving the destination table half-constructed ...

Fixes #17518 by defaulting to "public" if no schema is given

With test.

…node icon fails without notice

Fixes qgis#17518 by defaulting to "public" if no schema is given
@elpaso
Copy link
Contributor Author

elpaso commented Nov 22, 2017

@jef-n does it make sense to you? Or is there a better logic to get the "default" schema?

@jef-n
Copy link
Member

jef-n commented Nov 22, 2017

Doesn't say what the problem with the missing schema name is. Knowing which schema to refresh? Otherwise creating the table without the schema will do the right thing anyway.

@elpaso
Copy link
Contributor Author

elpaso commented Nov 22, 2017

@jef-n have a look to the test that I've added: it fails (badly, because it creates part of the table but not the geometry or attributes) on 2.x and master because the schema is not specified when creating the empty layer.

The problem with the empty schemaName resides in how the sql is constructed, if it's empty the sql will be ill-formed.

@elpaso
Copy link
Contributor Author

elpaso commented Nov 22, 2017

@jef-n I guess that instead of setting the default to "public" we could also change the logic that build the sql strings, but I didn't check wether AddGeometryColumn works without the schema.

@jef-n
Copy link
Member

jef-n commented Nov 22, 2017

the empty schema case needs to be handled everywhere (at least when checking whether the table exists and when calling AddGeometryColumn). Maybe by replacing schemaName with "current_schema" when it's empty and quotedValue(schemaName) when it's not. Alternatively there are also variants of AddGeometryColumn that don't need a schema.

@@ -3791,6 +3792,20 @@ QgsVectorLayerExporter::ExportError QgsPostgresProvider::createEmptyLayer( const
{
conn->PQexecNR( QStringLiteral( "BEGIN" ) );

// We want a valid schema name ...
if ( schemaName.isEmpty() )
Copy link
Member

@jef-n jef-n Nov 22, 2017

Choose a reason for hiding this comment

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

Ok, what I meant was schemaName = schemaName.isEmpty() ? "current_schema" : quotedValue(schemaName); here and schemaName instead of quotedValue(schemaName) below. Just one query less ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked on psql and you cannot use current_schema to prefix a table. That's why I added an extra query to retrieve its value.

Copy link
Member

Choose a reason for hiding this comment

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

you don't need to. a not prefixed table lands in the current schema

@elpaso elpaso merged commit 9d8a39f into qgis:master Nov 23, 2017
@elpaso elpaso deleted the bugfix-17518-pg-import--without-schema branch November 23, 2017 06:54
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

2 participants