[mssql][needs-docs] Add connection setting to ignore invalid geometry…

… handling Sets whether the connection should skip all handling of records with invalid geometry, which are slow and costly. This speeds up the provider, however, if any invalid geometries are present in a table then the result is unpredictable and may include missing records. Only check this option if you are certain that all geometries present in the database are valid, and any newly added geometries or tables will also be valid! Why would we want this? Well, SQL Server invalid geometry handling is AWEFUL. A seriously lame, data mangling and corrupting piece of s***. Use Postgres instead. But if you can't, then you can at least choose to use your layers at full speed, if you can take the responsibility that a SINGLE invalid geometry hiding somewhere in the table will result in a whole bunch of missing (valid) features. SQL server is at fault here, not us. There's nothing we (or GDAL, or MapServer, or GeoServer, or anyone else) can do to fix this. Suffice to say, this option is off by default, as we're better to have a slow provider which actually shows all features. Fixes #15752 Rant over
qgis · Oct 8, 2018 · dafeaf4 · dafeaf4
1 parent 7464575
commit dafeaf4
Show file tree

Hide file tree

Showing 13 changed files with 157 additions and 36 deletions.
diff --git a/src/providers/mssql/qgsmssqlconnection.cpp b/src/providers/mssql/qgsmssqlconnection.cpp
@@ -142,6 +142,18 @@ void QgsMssqlConnection::setUseEstimatedMetadata( const QString &name, bool enab
   settings.setValue( "/MSSQL/connections/" + name + "/estimatedMetadata", enabled );
 }
 
+bool QgsMssqlConnection::isInvalidGeometryHandlingDisabled( const QString &name )
+{
+  QgsSettings settings;
+  return settings.value( "/MSSQL/connections/" + name + "/disableInvalidGeometryHandling", false ).toBool();
+}
+
+void QgsMssqlConnection::setInvalidGeometryHandlingDisabled( const QString &name, bool disabled )
+{
+  QgsSettings settings;
+  settings.setValue( "/MSSQL/connections/" + name + "/disableInvalidGeometryHandling", disabled );
+}
+
 QString QgsMssqlConnection::dbConnectionName( const QString &name )
 {
   // Starting with Qt 5.11, sharing the same connection between threads is not allowed.

diff --git a/src/providers/mssql/qgsmssqlconnection.h b/src/providers/mssql/qgsmssqlconnection.h
@@ -61,27 +61,63 @@ class QgsMssqlConnection
     /**
      * Returns true if the connection with matching \a name should
      * show geometryless tables when scanning for tables.
+     *
+     * \see setAllowGeometrylessTables()
      */
     static bool allowGeometrylessTables( const QString &name );
 
     /**
      * Sets whether the connection with matching \a name should
      * show geometryless tables when scanning for tables.
+     *
+     * \see allowGeometrylessTables()
      */
     static void setAllowGeometrylessTables( const QString &name, bool enabled );
 
     /**
      * Returns true if the connection with matching \a name should
      * use estimated table parameters.
+     *
+     * \see setUseEstimatedMetadata()
      */
     static bool useEstimatedMetadata( const QString &name );
 
     /**
      * Sets whether the connection with matching \a name should
      * use estimated table parameters.
+     *
+     * \see useEstimatedMetadata()
      */
     static void setUseEstimatedMetadata( const QString &name, bool enabled );
 
+    /**
+     * Returns true if the connection with matching \a name should
+     * skip all handling of records with invalid geometry.
+     *
+     * This speeds up the provider, however, if any invalid geometries
+     * are present in a table then the result is unpredictable and may
+     * include missing records. Only check this option if you are certain
+     * that all geometries present in the database are valid, and any newly
+     * added geometries or tables will also be valid.
+     *
+     * \see setInvalidGeometryHandlingDisabled()
+     */
+    static bool isInvalidGeometryHandlingDisabled( const QString &name );
+
+    /**
+     * Sets whether the the connection with matching \a name should
+     * skip all handling of records with invalid geometry.
+     *
+     * This speeds up the provider, however, if any invalid geometries
+     * are present in a table then the result is unpredictable and may
+     * include missing records. Only check this option if you are certain
+     * that all geometries present in the database are valid, and any newly
+     * added geometries or tables will also be valid.
+     *
+     * \see isInvalidGeometryHandlingDisabled()
+     */
+    static void setInvalidGeometryHandlingDisabled( const QString &name, bool disabled );
+
   private:
 
     /**

diff --git a/src/providers/mssql/qgsmssqldataitems.cpp b/src/providers/mssql/qgsmssqldataitems.cpp
@@ -40,7 +40,7 @@
 #include <QProgressDialog>
 
 // ---------------------------------------------------------------------------
-QgsMssqlConnectionItem::QgsMssqlConnectionItem( QgsDataItem *parent, QString name, QString path )
+QgsMssqlConnectionItem::QgsMssqlConnectionItem( QgsDataItem *parent, const QString &name, const QString &path )
   : QgsDataCollectionItem( parent, name, path )
   , mUseGeometryColumns( false )
   , mUseEstimatedMetadata( false )
@@ -109,7 +109,7 @@ void QgsMssqlConnectionItem::refresh()
     int index = findItem( mChildren, item );
     if ( index >= 0 )
     {
-      ( ( QgsMssqlSchemaItem * )mChildren.at( index ) )->addLayers( item );
+      static_cast< QgsMssqlSchemaItem * >( mChildren.at( index ) )->addLayers( item );
       delete item;
       continue;
     }
@@ -154,6 +154,8 @@ QVector<QgsDataItem *> QgsMssqlConnectionItem::createChildren()
     query += QLatin1String( " union all select sys.schemas.name, sys.objects.name, null, null, 'NONE' from sys.objects join sys.schemas on sys.objects.schema_id = sys.schemas.schema_id where not exists (select * from sys.columns sc1 join sys.types on sc1.system_type_id = sys.types.system_type_id where (sys.types.name = 'geometry' or sys.types.name = 'geography') and sys.objects.object_id = sc1.object_id) and (sys.objects.type = 'U' or sys.objects.type = 'V')" );
   }
 
+  const bool disableInvalidGeometryHandling = QgsMssqlConnection::isInvalidGeometryHandlingDisabled( mName );
+
   // issue the sql query
   QSqlQuery q = QSqlQuery( db );
   q.setForwardOnly( true );
@@ -181,7 +183,8 @@ QVector<QgsDataItem *> QgsMssqlConnectionItem::createChildren()
         {
           Q_FOREACH ( QgsDataItem *child2, child->children() )
           {
-            if ( child2->name() == layer.tableName )
+            QgsMssqlLayerItem *layerItem = qobject_cast< QgsMssqlLayerItem *>( child2 );
+            if ( child2->name() == layer.tableName && layerItem && layerItem->disableInvalidGeometryHandling() == disableInvalidGeometryHandling )
             {
               newLayers.append( child2 );
               skip = true; // already added
@@ -202,7 +205,7 @@ QVector<QgsDataItem *> QgsMssqlConnectionItem::createChildren()
       {
         if ( child->name() == layer.schemaName )
         {
-          schemaItem = ( QgsMssqlSchemaItem * )child;
+          schemaItem = static_cast< QgsMssqlSchemaItem * >( child );
           break;
         }
       }
@@ -289,14 +292,14 @@ void QgsMssqlConnectionItem::setLayerType( QgsMssqlLayerProperty layerProperty )
   {
     if ( child->name() == layerProperty.schemaName )
     {
-      schemaItem = ( QgsMssqlSchemaItem * )child;
+      schemaItem = static_cast< QgsMssqlSchemaItem * >( child );
       break;
     }
   }
 
   if ( !schemaItem )
   {
-    QgsDebugMsg( QString( "schema item for %1 not found." ).arg( layerProperty.schemaName ) );
+    QgsDebugMsg( QStringLiteral( "schema item for %1 not found." ).arg( layerProperty.schemaName ) );
     return;
   }
 
@@ -315,7 +318,7 @@ void QgsMssqlConnectionItem::setLayerType( QgsMssqlLayerProperty layerProperty )
     QgsWkbTypes::Type wkbType = QgsMssqlTableModel::wkbTypeFromMssql( typeList[i] );
     if ( wkbType == QgsWkbTypes::Unknown )
     {
-      QgsDebugMsg( QString( "unsupported geometry type:%1" ).arg( typeList[i] ) );
+      QgsDebugMsg( QStringLiteral( "unsupported geometry type:%1" ).arg( typeList[i] ) );
       continue;
     }
 
@@ -476,7 +479,7 @@ bool QgsMssqlConnectionItem::handleDrop( const QMimeData *data, const QString &t
 
 
 // ---------------------------------------------------------------------------
-QgsMssqlLayerItem::QgsMssqlLayerItem( QgsDataItem *parent, QString name, QString path, QgsLayerItem::LayerType layerType, QgsMssqlLayerProperty layerProperty )
+QgsMssqlLayerItem::QgsMssqlLayerItem( QgsDataItem *parent, const QString &name, const QString &path, QgsLayerItem::LayerType layerType, const QgsMssqlLayerProperty &layerProperty )
   : QgsLayerItem( parent, name, path, QString(), layerType, QStringLiteral( "mssql" ) )
   , mLayerProperty( layerProperty )
 {
@@ -489,27 +492,36 @@ QgsMssqlLayerItem *QgsMssqlLayerItem::createClone()
   return new QgsMssqlLayerItem( mParent, mName, mPath, mLayerType, mLayerProperty );
 }
 
+bool QgsMssqlLayerItem::disableInvalidGeometryHandling() const
+{
+  return mDisableInvalidGeometryHandling;
+}
+
 QString QgsMssqlLayerItem::createUri()
 {
   QString pkColName = !mLayerProperty.pkCols.isEmpty() ? mLayerProperty.pkCols.at( 0 ) : QString();
   QgsMssqlConnectionItem *connItem = qobject_cast<QgsMssqlConnectionItem *>( parent() ? parent()->parent() : nullptr );
 
   if ( !connItem )
   {
-    QgsDebugMsg( "connection item not found." );
+    QgsDebugMsg( QStringLiteral( "connection item not found." ) );
     return QString();
   }
 
   QgsDataSourceUri uri = QgsDataSourceUri( connItem->connInfo() );
   uri.setDataSource( mLayerProperty.schemaName, mLayerProperty.tableName, mLayerProperty.geometryColName, mLayerProperty.sql, pkColName );
   uri.setSrid( mLayerProperty.srid );
   uri.setWkbType( QgsMssqlTableModel::wkbTypeFromMssql( mLayerProperty.type ) );
-  QgsDebugMsg( QString( "layer uri: %1" ).arg( uri.uri() ) );
+  uri.setUseEstimatedMetadata( QgsMssqlConnection::useEstimatedMetadata( connItem->name() ) );
+  mDisableInvalidGeometryHandling = QgsMssqlConnection::isInvalidGeometryHandlingDisabled( connItem->name() );
+  uri.setParam( QStringLiteral( "disableInvalidGeometryHandling" ), mDisableInvalidGeometryHandling ? QStringLiteral( "1" ) : QStringLiteral( "0" ) );
+
+  QgsDebugMsg( QStringLiteral( "layer uri: %1" ).arg( uri.uri() ) );
   return uri.uri();
 }
 
 // ---------------------------------------------------------------------------
-QgsMssqlSchemaItem::QgsMssqlSchemaItem( QgsDataItem *parent, QString name, QString path )
+QgsMssqlSchemaItem::QgsMssqlSchemaItem( QgsDataItem *parent, const QString &name, const QString &path )
   : QgsDataCollectionItem( parent, name, path )
 {
   mIconName = QStringLiteral( "mIconDbSchema.svg" );
@@ -519,7 +531,6 @@ QgsMssqlSchemaItem::QgsMssqlSchemaItem( QgsDataItem *parent, QString name, QStri
 
 QVector<QgsDataItem *> QgsMssqlSchemaItem::createChildren()
 {
-  QgsDebugMsg( "Entering." );
   return QVector<QgsDataItem *>();
 }
 
@@ -533,7 +544,7 @@ void QgsMssqlSchemaItem::addLayers( QgsDataItem *newLayers )
     {
       continue;
     }
-    QgsMssqlLayerItem *layer = ( ( QgsMssqlLayerItem * )child )->createClone();
+    QgsMssqlLayerItem *layer = static_cast< QgsMssqlLayerItem * >( child )->createClone();
     addChildItem( layer, true );
   }
 }
@@ -591,7 +602,7 @@ QgsMssqlLayerItem *QgsMssqlSchemaItem::addLayer( const QgsMssqlLayerProperty &la
 }
 
 // ---------------------------------------------------------------------------
-QgsMssqlRootItem::QgsMssqlRootItem( QgsDataItem *parent, QString name, QString path )
+QgsMssqlRootItem::QgsMssqlRootItem( QgsDataItem *parent, const QString &name, const QString &path )
   : QgsDataCollectionItem( parent, name, path )
 {
   mIconName = QStringLiteral( "mIconMssql.svg" );

diff --git a/src/providers/mssql/qgsmssqldataitems.h b/src/providers/mssql/qgsmssqldataitems.h
@@ -34,7 +34,7 @@ class QgsMssqlRootItem : public QgsDataCollectionItem
 {
     Q_OBJECT
   public:
-    QgsMssqlRootItem( QgsDataItem *parent, QString name, QString path );
+    QgsMssqlRootItem( QgsDataItem *parent, const QString &name, const QString &path );
 
     QVector<QgsDataItem *> createChildren() override;
 
@@ -56,7 +56,7 @@ class QgsMssqlConnectionItem : public QgsDataCollectionItem
 {
     Q_OBJECT
   public:
-    QgsMssqlConnectionItem( QgsDataItem *parent, QString name, QString path );
+    QgsMssqlConnectionItem( QgsDataItem *parent, const QString &name, const QString &path );
     ~QgsMssqlConnectionItem() override;
 
     QVector<QgsDataItem *> createChildren() override;
@@ -109,7 +109,7 @@ class QgsMssqlSchemaItem : public QgsDataCollectionItem
 {
     Q_OBJECT
   public:
-    QgsMssqlSchemaItem( QgsDataItem *parent, QString name, QString path );
+    QgsMssqlSchemaItem( QgsDataItem *parent, const QString &name, const QString &path );
 
     QVector<QgsDataItem *> createChildren() override;
 
@@ -125,14 +125,17 @@ class QgsMssqlLayerItem : public QgsLayerItem
     Q_OBJECT
 
   public:
-    QgsMssqlLayerItem( QgsDataItem *parent, QString name, QString path, QgsLayerItem::LayerType layerType, QgsMssqlLayerProperty layerProperties );
+    QgsMssqlLayerItem( QgsDataItem *parent, const QString &name, const QString &path, QgsLayerItem::LayerType layerType, const QgsMssqlLayerProperty &layerProperties );
 
     QString createUri();
 
     QgsMssqlLayerItem *createClone();
 
+    bool disableInvalidGeometryHandling() const;
+
   private:
     QgsMssqlLayerProperty mLayerProperty;
+    bool mDisableInvalidGeometryHandling = false;
 };
 
 #endif // QGSMSSQLDATAITEMS_H
diff --git a/src/providers/mssql/qgsmssqlfeatureiterator.cpp b/src/providers/mssql/qgsmssqlfeatureiterator.cpp
@@ -30,6 +30,7 @@
 
 QgsMssqlFeatureIterator::QgsMssqlFeatureIterator( QgsMssqlFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
   : QgsAbstractFeatureIteratorFromSource<QgsMssqlFeatureSource>( source, ownSource, request )
+  , mDisableInvalidGeometryHandling( source->mDisableInvalidGeometryHandling )
 {
   mClosed = false;
 
@@ -141,7 +142,10 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest &request )
         <<  qgsDoubleToString( mFilterRect.xMinimum() ) << ' ' <<  qgsDoubleToString( mFilterRect.yMaximum() ) << ", "
         <<  qgsDoubleToString( mFilterRect.xMinimum() ) << ' ' <<  qgsDoubleToString( mFilterRect.yMinimum() );
 
-    mStatement += QStringLiteral( " where [%1].STIsValid() = 1 AND [%1].STIntersects([%2]::STGeomFromText('POLYGON((%3))',%4)) = 1" ).arg(
+    mStatement += QStringLiteral( " WHERE " );
+    if ( !mDisableInvalidGeometryHandling )
+      mStatement += QStringLiteral( "[%1].STIsValid() = 1 AND " ).arg( mSource->mGeometryColName );
+    mStatement += QStringLiteral( "[%1].STIntersects([%2]::STGeomFromText('POLYGON((%3))',%4)) = 1" ).arg(
                     mSource->mGeometryColName, mSource->mGeometryColType, r, QString::number( mSource->mSRId ) );
     filterAdded = true;
   }
@@ -495,6 +499,7 @@ QgsMssqlFeatureSource::QgsMssqlFeatureSource( const QgsMssqlProvider *p )
   , mDatabaseName( p->mDatabaseName )
   , mHost( p->mHost )
   , mSqlWhereClause( p->mSqlWhereClause )
+  , mDisableInvalidGeometryHandling( p->mDisableInvalidGeometryHandling )
   , mCrs( p->crs() )
 {}
 

diff --git a/src/providers/mssql/qgsmssqlfeatureiterator.h b/src/providers/mssql/qgsmssqlfeatureiterator.h
@@ -61,6 +61,8 @@ class QgsMssqlFeatureSource : public QgsAbstractFeatureSource
     // SQL statement used to limit the features retrieved
     QString mSqlWhereClause;
 
+    bool mDisableInvalidGeometryHandling = false;
+
     QgsCoordinateReferenceSystem mCrs;
 
     // Return True if this feature source has spatial attributes.
@@ -116,6 +118,7 @@ class QgsMssqlFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsM
 
     bool mExpressionCompiled = false;
     bool mOrderByCompiled = false;
+    bool mDisableInvalidGeometryHandling = false;
 
     QgsCoordinateTransform mTransform;
     QgsRectangle mFilterRect;

diff --git a/src/providers/mssql/qgsmssqlnewconnection.cpp b/src/providers/mssql/qgsmssqlnewconnection.cpp
@@ -52,6 +52,7 @@ QgsMssqlNewConnection::QgsMssqlNewConnection( QWidget *parent, const QString &co
     cb_geometryColumns->setChecked( QgsMssqlConnection::geometryColumnsOnly( connName ) );
     cb_allowGeometrylessTables->setChecked( QgsMssqlConnection::allowGeometrylessTables( connName ) );
     cb_useEstimatedMetadata->setChecked( QgsMssqlConnection::useEstimatedMetadata( connName ) );
+    mCheckNoInvalidGeometryHandling->setChecked( QgsMssqlConnection::isInvalidGeometryHandlingDisabled( connName ) );
 
     if ( settings.value( key + "/saveUsername" ).toString() == QLatin1String( "true" ) )
     {
@@ -116,6 +117,7 @@ void QgsMssqlNewConnection::accept()
   QgsMssqlConnection::setGeometryColumnsOnly( connName, cb_geometryColumns->isChecked() );
   QgsMssqlConnection::setAllowGeometrylessTables( connName, cb_allowGeometrylessTables->isChecked() );
   QgsMssqlConnection::setUseEstimatedMetadata( connName, cb_useEstimatedMetadata->isChecked() );
+  QgsMssqlConnection::setInvalidGeometryHandlingDisabled( connName, mCheckNoInvalidGeometryHandling->isChecked() );
 
   QDialog::accept();
 }

diff --git a/src/providers/mssql/qgsmssqlprovider.cpp b/src/providers/mssql/qgsmssqlprovider.cpp
@@ -82,6 +82,10 @@ QgsMssqlProvider::QgsMssqlProvider( const QString &uri, const ProviderOptions &o
 
   mUseEstimatedMetadata = anUri.useEstimatedMetadata();
 
+  mDisableInvalidGeometryHandling = anUri.hasParam( QStringLiteral( "disableInvalidGeometryHandling" ) )
+                                    ? anUri.param( QStringLiteral( "disableInvalidGeometryHandling" ) ).toInt()
+                                    : false;
+
   mSqlWhereClause = anUri.sql();
 
   mDatabase = QgsMssqlConnection::getDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
@@ -701,14 +705,29 @@ void QgsMssqlProvider::UpdateStatistics( bool estimate ) const
   if ( estimate )
   {
     if ( mGeometryColType == QLatin1String( "geometry" ) )
-      statement = QStringLiteral( "select min([%1].MakeValid().STPointN(1).STX), min([%1].MakeValid().STPointN(1).STY), max([%1].MakeValid().STPointN(1).STX), max([%1].MakeValid().STPointN(1).STY)" ).arg( mGeometryColName );
+    {
+      if ( mDisableInvalidGeometryHandling )
+        statement = QStringLiteral( "select min([%1].STPointN(1).STX), min([%1].STPointN(1).STY), max([%1].STPointN(1).STX), max([%1].STPointN(1).STY)" ).arg( mGeometryColName );
+      else
+        statement = QStringLiteral( "select min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STX else NULL end), min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STY else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STX else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STY else NULL end)" ).arg( mGeometryColName );
+    }
     else
-      statement = QStringLiteral( "select min([%1].MakeValid().STPointN(1).Long), min([%1].MakeValid().STPointN(1).Lat), max([%1].MakeValid().STPointN(1).Long), max([%1].MakeValid().STPointN(1).Lat)" ).arg( mGeometryColName );
+    {
+      if ( mDisableInvalidGeometryHandling )
+        statement = QStringLiteral( "select min([%1].STPointN(1).Long), min([%1].STPointN(1).Lat), max([%1].STPointN(1).Long), max([%1].STPointN(1).Lat)" ).arg( mGeometryColName );
+      else
+        statement = QStringLiteral( "select min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Long  else NULL end), min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Lat else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Long else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Lat else NULL end)" ).arg( mGeometryColName );
+    }
   }
   else
   {
     if ( mGeometryColType == QLatin1String( "geometry" ) )
-      statement = QStringLiteral( "select min([%1].MakeValid().STEnvelope().STPointN(1).STX), min([%1].MakeValid().STEnvelope().STPointN(1).STY), max([%1].MakeValid().STEnvelope().STPointN(3).STX), max([%1].MakeValid().STEnvelope().STPointN(3).STY)" ).arg( mGeometryColName );
+    {
+      if ( mDisableInvalidGeometryHandling )
+        statement = QStringLiteral( "select min(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(1).STX  else NULL end), min(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(1).STY else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(3).STX else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(3).STY else NULL end)" ).arg( mGeometryColName );
+      else
+        statement = QStringLiteral( "select min([%1].STEnvelope().STPointN(1).STX), min([%1].STEnvelope().STPointN(1).STY), max([%1].STEnvelope().STPointN(3).STX), max([%1].STEnvelope().STPointN(3).STY)" ).arg( mGeometryColName );
+    }
     else
     {
       statement = QStringLiteral( "select [%1]" ).arg( mGeometryColName );

diff --git a/src/providers/mssql/qgsmssqlprovider.h b/src/providers/mssql/qgsmssqlprovider.h
@@ -205,6 +205,8 @@ class QgsMssqlProvider : public QgsVectorDataProvider
     // SQL statement used to limit the features retrieved
     QString mSqlWhereClause;
 
+    bool mDisableInvalidGeometryHandling = false;
+
     // Sets the error messages
     void setLastError( const QString &error );