Merge pull request #39183 from elpaso/bugfix-gh37666-no-geometry-in-f…

…ields Fields items: various fixes and enhancements
qgis · Oct 5, 2020 · ac6ee22 · ac6ee22
2 parents 38eabb7 + 399beba
commit ac6ee22
Show file tree

Hide file tree

Showing 7 changed files with 122 additions and 18 deletions.
diff --git a/python/core/auto_generated/qgsdataitem.sip.in b/python/core/auto_generated/qgsdataitem.sip.in
@@ -1036,6 +1036,12 @@ Returns the connection URI
 Creates and returns a (possibly NULL) layer from the connection URI and schema/table information
 %End
 
+    QgsAbstractDatabaseProviderConnection::TableProperty *tableProperty() const;
+%Docstring
+Returns the (possibly NULL) properties of the table this fields belong to.
+
+.. versionadded:: 3.16
+%End
 
 };
 
@@ -1070,6 +1076,8 @@ Constructor for QgsFieldItem, with the specified ``parent`` item and ``field``.
     virtual QIcon icon();
 
 
+
+
 };
 
 

diff --git a/src/app/browser/qgsinbuiltdataitemproviders.cpp b/src/app/browser/qgsinbuiltdataitemproviders.cpp
@@ -836,7 +836,16 @@ void QgsFieldItemGuiProvider::populateContextMenu( QgsDataItem *item, QMenu *men
           connect( deleteFieldAction, &QAction::triggered, fieldsItem, [ md, fieldsItem, itemName, context, supportsCascade ]
           {
             // Confirmation dialog
-            QMessageBox msgbox{QMessageBox::Icon::Question, tr( "Delete Field" ), tr( "Delete '%1' permanently?" ).arg( itemName ), QMessageBox::Ok | QMessageBox::Cancel };
+            QString message {  tr( "Delete '%1' permanently?" ).arg( itemName ) };
+            if ( fieldsItem->tableProperty() && fieldsItem->tableProperty()->primaryKeyColumns().contains( itemName ) )
+            {
+              message.append( tr( "\nThis field is part of a primary key, its removal may make the table unusable by QGIS!" ) );
+            }
+            if ( fieldsItem->tableProperty() && fieldsItem->tableProperty()->geometryColumn() == itemName )
+            {
+              message.append( tr( "\nThis field is a geometry column, its removal may make the table unusable by QGIS!" ) );
+            }
+            QMessageBox msgbox{QMessageBox::Icon::Question, tr( "Delete Field" ), message, QMessageBox::Ok | QMessageBox::Cancel };
             QCheckBox *cb = new QCheckBox( tr( "Delete all related objects (CASCADE)?" ) );
             msgbox.setCheckBox( cb );
             msgbox.setDefaultButton( QMessageBox::Cancel );
@@ -917,8 +926,8 @@ void QgsDatabaseItemGuiProvider::populateContextMenu( QgsDataItem *item, QMenu *
                 const QString geometryColumn { dlg.geometryColumnName() };
                 const QgsWkbTypes::Type geometryType { dlg.geometryType() };
                 const bool createSpatialIndex { dlg.createSpatialIndex() &&
-                  geometryType != QgsWkbTypes::NoGeometry &&
-                  geometryType != QgsWkbTypes::Unknown };
+                                                geometryType != QgsWkbTypes::NoGeometry &&
+                                                geometryType != QgsWkbTypes::Unknown };
                 const QgsCoordinateReferenceSystem crs { dlg.crs( ) };
                 // This flag tells to the provider that field types do not need conversion
                 QMap<QString, QVariant> options { { QStringLiteral( "skipConvertFields" ), true } };

diff --git a/src/core/qgsdataitem.cpp b/src/core/qgsdataitem.cpp
@@ -124,6 +124,19 @@ QgsFieldsItem::QgsFieldsItem( QgsDataItem *parent,
   , mConnectionUri( connectionUri )
 {
   mCapabilities |= ( Fertile | Collapse );
+  QgsProviderMetadata *md { QgsProviderRegistry::instance()->providerMetadata( providerKey ) };
+  if ( md )
+  {
+    try
+    {
+      std::unique_ptr<QgsAbstractDatabaseProviderConnection> conn { static_cast<QgsAbstractDatabaseProviderConnection *>( md->createConnection( mConnectionUri, {} ) ) };
+      mTableProperty = qgis::make_unique<QgsAbstractDatabaseProviderConnection::TableProperty>( conn->table( schema, tableName ) );
+    }
+    catch ( QgsProviderConnectionException &ex )
+    {
+      QgsDebugMsg( QStringLiteral( "Error creating fields item: %1" ).arg( ex.what() ) );
+    }
+  }
 }
 
 QgsFieldsItem::~QgsFieldsItem()
@@ -202,6 +215,11 @@ QgsVectorLayer *QgsFieldsItem::layer()
   return nullptr;
 }
 
+QgsAbstractDatabaseProviderConnection::TableProperty *QgsFieldsItem::tableProperty() const
+{
+  return mTableProperty.get();
+}
+
 QString QgsFieldsItem::tableName() const
 {
   return mTableName;
@@ -227,6 +245,30 @@ QgsFieldItem::~QgsFieldItem()
 
 QIcon QgsFieldItem::icon()
 {
+  // Check if this is a geometry column and show the right icon
+  QgsFieldsItem *parentFields { static_cast<QgsFieldsItem *>( parent() ) };
+  if ( parentFields && parentFields->tableProperty() &&
+       parentFields->tableProperty()->geometryColumn() == mName &&
+       parentFields->tableProperty()->geometryColumnTypes().count() )
+  {
+    if ( mField.typeName() == QStringLiteral( "raster" ) )
+    {
+      return QgsLayerItem::iconRaster();
+    }
+    const QgsWkbTypes::GeometryType geomType { QgsWkbTypes::geometryType( parentFields->tableProperty()->geometryColumnTypes().first().wkbType ) };
+    switch ( geomType )
+    {
+      case QgsWkbTypes::GeometryType::LineGeometry:
+        return QgsLayerItem::iconLine();
+      case QgsWkbTypes::GeometryType::PointGeometry:
+        return QgsLayerItem::iconPoint();
+      case QgsWkbTypes::GeometryType::PolygonGeometry:
+        return QgsLayerItem::iconPolygon();
+      case QgsWkbTypes::GeometryType::UnknownGeometry:
+      case QgsWkbTypes::GeometryType::NullGeometry:
+        return QgsLayerItem::iconDefault();
+    }
+  }
   const QIcon icon { QgsFields::iconForFieldType( mField.type() ) };
   // Try subtype if icon is null
   if ( icon.isNull() )

diff --git a/src/core/qgsdataitem.h b/src/core/qgsdataitem.h
@@ -33,6 +33,7 @@
 #include "qgscoordinatereferencesystem.h"
 #include "qgsmimedatautils.h"
 #include "qgswkbtypes.h"
+#include "qgsabstractdatabaseproviderconnection.h"
 
 class QgsDataProvider;
 class QgsDataItem;
@@ -1021,12 +1022,19 @@ class CORE_EXPORT QgsFieldsItem : public QgsDataItem
      */
     QgsVectorLayer *layer() SIP_FACTORY;
 
+    /**
+     * Returns the (possibly NULL) properties of the table this fields belong to.
+     * \since QGIS 3.16
+     */
+    QgsAbstractDatabaseProviderConnection::TableProperty *tableProperty() const;
 
   private:
 
     QString mSchema;
     QString mTableName;
     QString mConnectionUri;
+    std::unique_ptr<QgsAbstractDatabaseProviderConnection::TableProperty> mTableProperty;
+
 };
 
 
@@ -1053,6 +1061,9 @@ class CORE_EXPORT QgsFieldItem : public QgsDataItem
 
     QIcon icon() override;
 
+    //QgsField field() const;
+
+
   private:
 
     const QgsField mField;

diff --git a/src/providers/postgres/qgspostgresproviderconnection.cpp b/src/providers/postgres/qgspostgresproviderconnection.cpp
@@ -472,16 +472,16 @@ QList<QgsPostgresProviderConnection::TableProperty> QgsPostgresProviderConnectio
   }
   else
   {
-    bool ok = conn->getTableInfo( false, false, true, schema );
+    QVector<QgsPostgresLayerProperty> properties;
+    const bool aspatial { ! flags || flags.testFlag( TableFlag::Aspatial ) };
+    bool ok = conn->supportedLayers( properties, false, schema == QStringLiteral( "public" ), aspatial, schema );
     if ( ! ok )
     {
       errCause = QObject::tr( "Could not retrieve tables: %1" ).arg( uri() );
     }
     else
     {
-      QVector<QgsPostgresLayerProperty> properties;
-      const bool aspatial { ! flags || flags.testFlag( TableFlag::Aspatial ) };
-      conn->supportedLayers( properties, false, schema == QStringLiteral( "public" ), aspatial, schema );
+
       bool dontResolveType = configuration().value( QStringLiteral( "dontResolveType" ), false ).toBool();
       bool useEstimatedMetadata = configuration().value( QStringLiteral( "estimatedMetadata" ), false ).toBool();
 
@@ -533,9 +533,42 @@ QList<QgsPostgresProviderConnection::TableProperty> QgsPostgresProviderConnectio
           property.setTableName( pr.tableName );
           property.setSchema( pr.schemaName );
           property.setGeometryColumn( pr.geometryColName );
-          property.setPrimaryKeyColumns( pr.pkCols );
+          // These are candidates, not actual PKs
+          // property.setPrimaryKeyColumns( pr.pkCols );
           property.setGeometryColumnCount( static_cast<int>( pr.nSpCols ) );
           property.setComment( pr.tableComment );
+
+          // Get PKs
+          if ( pr.isView || pr.isMaterializedView || pr.isForeignTable )
+          {
+            // Set the candidates
+            property.setPrimaryKeyColumns( pr.pkCols );
+          }
+          else  // Fetch and set the real pks
+          {
+            try
+            {
+              const auto pks = executeSql( QStringLiteral( R"(
+              WITH pkrelid AS (
+              SELECT indexrelid AS idxri FROM pg_index WHERE indrelid='%1.%2'::regclass AND (indisprimary OR indisunique)
+                ORDER BY CASE WHEN indisprimary THEN 1 ELSE 2 END LIMIT 1)
+              SELECT attname FROM pg_index,pg_attribute, pkrelid
+              WHERE indexrelid=pkrelid.idxri AND indrelid=attrelid AND pg_attribute.attnum=any(pg_index.indkey);
+             )" ).arg( QgsPostgresConn::quotedIdentifier( pr.schemaName ) )
+                                           .arg( QgsPostgresConn::quotedIdentifier( pr.tableName ) ) );
+              QStringList pkNames;
+              for ( const auto &pk : qgis::as_const( pks ) )
+              {
+                pkNames.push_back( pk.first().toString() );
+              }
+              property.setPrimaryKeyColumns( pkNames );
+            }
+            catch ( const QgsProviderConnectionException &ex )
+            {
+              QgsDebugMsg( QStringLiteral( "Error retrieving primary keys: %1" ).arg( ex.what() ) );
+            }
+          }
+
           tables.push_back( property );
         }
       }

diff --git a/tests/src/python/test_qgsproviderconnection_postgres.py b/tests/src/python/test_qgsproviderconnection_postgres.py
@@ -309,15 +309,15 @@ def test_foreign_table_server(self):
         service = uri.service()
 
         foreign_table_definition = """
-CREATE EXTENSION IF NOT EXISTS postgres_fdw;
-CREATE SERVER IF NOT EXISTS postgres_fdw_test_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (service '{service}', dbname '{dbname}', host '{host}', port '{port}');
-DROP SCHEMA  IF EXISTS foreign_schema CASCADE;
-CREATE SCHEMA IF NOT EXISTS foreign_schema;
-CREATE USER MAPPING IF NOT EXISTS FOR CURRENT_USER SERVER postgres_fdw_test_server OPTIONS (user '{user}', password '{password}');
-IMPORT FOREIGN SCHEMA qgis_test LIMIT TO ( "someData" )
-  FROM SERVER postgres_fdw_test_server
-  INTO foreign_schema;
-""".format(host=host, user=user, port=port, dbname=dbname, password=password, service=service)
+        CREATE EXTENSION IF NOT EXISTS postgres_fdw;
+        CREATE SERVER IF NOT EXISTS postgres_fdw_test_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (service '{service}', dbname '{dbname}', host '{host}', port '{port}');
+        DROP SCHEMA  IF EXISTS foreign_schema CASCADE;
+        CREATE SCHEMA IF NOT EXISTS foreign_schema;
+        CREATE USER MAPPING IF NOT EXISTS FOR CURRENT_USER SERVER postgres_fdw_test_server OPTIONS (user '{user}', password '{password}');
+        IMPORT FOREIGN SCHEMA qgis_test LIMIT TO ( "someData" )
+        FROM SERVER postgres_fdw_test_server
+        INTO foreign_schema;
+        """.format(host=host, user=user, port=port, dbname=dbname, password=password, service=service)
         conn.executeSql(foreign_table_definition)
         self.assertEquals(conn.tables('foreign_schema', QgsAbstractDatabaseProviderConnection.Foreign)[0].tableName(), 'someData')
 
@@ -329,7 +329,6 @@ def test_fields(self):
         fields = conn.fields('qgis_test', 'someData')
         self.assertEqual(fields.names(), ['pk', 'cnt', 'name', 'name2', 'num_char', 'dt', 'date', 'time', 'geom'])
 
-        # Test regression GH #37666
         sql = """
         DROP TABLE IF EXISTS qgis_test.gh_37666;
         CREATE TABLE qgis_test.gh_37666 (id SERIAL PRIMARY KEY);
@@ -343,6 +342,8 @@ def test_fields(self):
         fields = conn.fields('qgis_test', 'gh_37666')
         self.assertEqual([f.name() for f in fields], ['id', 'geom', 'geog'])
         self.assertEqual([f.typeName() for f in fields], ['int4', 'geometry', 'geography'])
+        table = conn.table('qgis_test', 'gh_37666')
+        self.assertEqual(table.primaryKeyColumns(), ['id'])
 
     def test_fields_no_pk(self):
         """Test issue: no fields are exposed for raster_columns"""

diff --git a/tests/testdata/qgis_server/bug_gh30264_empty_layer_wrong_bbox.gpkg b/tests/testdata/qgis_server/bug_gh30264_empty_layer_wrong_bbox.gpkg