Merge pull request #4642 from nyalldawson/mssql

MSSQL provider fixes

src/providers/mssql/qgsmssqlexpressioncompiler.cpp

@@ -77,3 +77,12 @@ QString QgsMssqlExpressionCompiler::quotedValue( const QVariant& value, bool& ok
       return QgsSqlExpressionCompiler::quotedValue( value, ok );
   }
 }
+
+QString QgsMssqlExpressionCompiler::quotedIdentifier( const QString &identifier )
+{
+  QString quoted = identifier;
+  quoted.replace( '[', "[[" );
+  quoted.replace( ']', "]]" );
+  quoted = quoted.prepend( '[' ).append( ']' );
+  return quoted;
+}

src/providers/mssql/qgsmssqlexpressioncompiler.h

@@ -29,6 +29,7 @@ class QgsMssqlExpressionCompiler : public QgsSqlExpressionCompiler
   protected:
     virtual Result compileNode( const QgsExpression::Node* node, QString& result ) override;
     virtual QString quotedValue( const QVariant& value, bool& ok ) override;
+    virtual QString quotedIdentifier( const QString& identifier ) override;
 
 };
 

src/providers/mssql/qgsmssqlfeatureiterator.cpp

@@ -109,7 +109,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
     mStatement += QString( ",[%1]" ).arg( mSource->mGeometryColName );
   }
 
-  mStatement += QString( "FROM [%1].[%2]" ).arg( mSource->mSchemaName, mSource->mTableName );
+  mStatement += QString( " FROM [%1].[%2]" ).arg( mSource->mSchemaName, mSource->mTableName );
 
   bool filterAdded = false;
   // set spatial filter
@@ -127,7 +127,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
     <<  qgsDoubleToString( request.filterRect().xMinimum() ) << ' ' <<  qgsDoubleToString( request.filterRect().yMaximum() ) << ", "
     <<  qgsDoubleToString( request.filterRect().xMinimum() ) << ' ' <<  qgsDoubleToString( request.filterRect().yMinimum() );
 
-    mStatement += QString( " where [%1].STIsValid() = 1 AND [%1].STIntersects([%2]::STGeomFromText('POLYGON((%3))',%4)) = 1" ).arg(
+    mStatement += QString( " where [%1].STIsValid() = 1 AND [%1].Filter([%2]::STGeomFromText('POLYGON((%3))',%4)) = 1" ).arg(
                     mSource->mGeometryColName, mSource->mGeometryColType, r, QString::number( mSource->mSRId ) );
     filterAdded = true;
   }
@@ -247,7 +247,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
     mOrderByCompiled = false;
   }
 
-  if ( !mOrderByCompiled )
+  if ( !mOrderByCompiled && !request.orderBy().isEmpty() )
     limitAtProvider = false;
 
   if ( request.limit() >= 0 && limitAtProvider )
@@ -301,8 +301,25 @@ bool QgsMssqlFeatureIterator::fetchFeature( QgsFeature& feature )
     {
       QVariant v = mQuery->value( i );
       const QgsField &fld = mSource->mFields.at( mAttributesToFetch.at( i ) );
-      if ( v.type() != fld.type() )
+
+      // special handling for time fields
+      if ( fld.type() == QVariant::Time && v.type() == QVariant::ByteArray )
+      {
+        QList<QByteArray> parts = v.toByteArray().split( '\0' );
+        if ( parts.count() >= 3 )
+        {
+          int hours = QString( parts.at( 0 ) ).at( 0 ).toAscii();
+          int minutes = QString( parts.at( 1 ) ).at( 0 ).toAscii();
+          int seconds = QString( parts.at( 2 ) ).at( 0 ).toAscii();
+          v = QTime( hours, minutes, seconds );
+        }
+        else
+          v = QgsVectorDataProvider::convertValue( fld.type(), v.toString() );
+      }
+      else if ( v.type() != fld.type() )
+      {
         v = QgsVectorDataProvider::convertValue( fld.type(), v.toString() );
+      }
       feature.setAttribute( mAttributesToFetch.at( i ), v );
     }
 
@@ -311,12 +328,19 @@ bool QgsMssqlFeatureIterator::fetchFeature( QgsFeature& feature )
     if ( mSource->isSpatial() )
     {
       QByteArray ar = mQuery->record().value( mSource->mGeometryColName ).toByteArray();
-      unsigned char* wkb = mParser.ParseSqlGeometry(( unsigned char* )ar.data(), ar.size() );
-      if ( wkb )
+      if ( !ar.isEmpty() )
       {
-        QgsGeometry *g = new QgsGeometry();
-        g->fromWkb( wkb, mParser.GetWkbLen() );
-        feature.setGeometry( g );
+        unsigned char* wkb = mParser.ParseSqlGeometry(( unsigned char* )ar.data(), ar.size() );
+        if ( wkb )
+        {
+          QgsGeometry *g = new QgsGeometry();
+          g->fromWkb( wkb, mParser.GetWkbLen() );
+          feature.setGeometry( g );
+        }
+        else
+        {
+          feature.setGeometry( nullptr );
+        }
       }
       else
       {

src/providers/mssql/qgsmssqlprovider.cpp

@@ -350,6 +350,11 @@ void QgsMssqlProvider::loadMetadata()
   mSRId = 0;
   mWkbType = QGis::WKBUnknown;
 
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
+
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
   if ( !query.exec( QString( "select f_geometry_column, coord_dimension, srid, geometry_type from geometry_columns where f_table_schema = '%1' and f_table_name = '%2'" ).arg( mSchemaName, mTableName ) ) )
@@ -368,7 +373,12 @@ void QgsMssqlProvider::loadFields()
 {
   mAttributeFields.clear();
   mDefaultValues.clear();
+
   // get field spec
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
   if ( !query.exec( QString( "exec sp_columns @table_name = N'%1', @table_owner = '%2'" ).arg( mTableName, mSchemaName ) ) )
@@ -532,6 +542,10 @@ QVariant QgsMssqlProvider::minimumValue( int index )
     sql += QString( " where (%1)" ).arg( mSqlWhereClause );
   }
 
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
 
@@ -563,6 +577,10 @@ QVariant QgsMssqlProvider::maximumValue( int index )
     sql += QString( " where (%1)" ).arg( mSqlWhereClause );
   }
 
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
 
@@ -603,6 +621,10 @@ void QgsMssqlProvider::uniqueValues( int index, QList<QVariant> &uniqueValues, i
     sql += QString( " where (%1)" ).arg( mSqlWhereClause );
   }
 
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
 
@@ -631,6 +653,10 @@ void QgsMssqlProvider::UpdateStatistics( bool estimate )
   // get features to calculate the statistics
   QString statement;
 
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
 
@@ -664,14 +690,14 @@ void QgsMssqlProvider::UpdateStatistics( bool estimate )
   if ( estimate )
   {
     if ( mGeometryColType == "geometry" )
-      statement = QString( "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 );
+      statement = QString( "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 = QString( "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 );
+      statement = QString( "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 == "geometry" )
-      statement = QString( "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 );
+      statement = QString( "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 = QString( "select [%1]" ).arg( mGeometryColName );
@@ -758,6 +784,10 @@ long QgsMssqlProvider::featureCount() const
 
   // If there is no subset set we can get the count from the system tables.
   // Which is faster then doing select count(*)
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
 
@@ -1419,6 +1449,10 @@ QgsCoordinateReferenceSystem QgsMssqlProvider::crs()
       return mCrs;
 
     // try to load crs from the database tables as a fallback
+    if ( !mDatabase.isOpen() )
+    {
+      mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+    }
     QSqlQuery query = QSqlQuery( mDatabase );
     query.setForwardOnly( true );
     bool execOk = query.exec( QString( "select srtext from spatial_ref_sys where srid = %1" ).arg( QString::number( mSRId ) ) );
@@ -1472,6 +1506,10 @@ bool QgsMssqlProvider::setSubsetString( const QString& theSQL, bool )
     sql += QString( " where %1" ).arg( mSqlWhereClause );
   }
 
+  if ( !mDatabase.isOpen() )
+  {
+    mDatabase = GetDatabase( mService, mHost, mDatabaseName, mUserName, mPassword );
+  }
   QSqlQuery query = QSqlQuery( mDatabase );
   query.setForwardOnly( true );
   if ( !query.exec( sql ) )

src/providers/mssql/qgsmssqlprovider.h

@@ -290,7 +290,7 @@ class QgsMssqlProvider : public QgsVectorDataProvider
     QGis::WkbType mWkbType;
 
     // The database object
-    QSqlDatabase mDatabase;
+    mutable QSqlDatabase mDatabase;
 
     // The current sql query
     QSqlQuery mQuery;

src/providers/mssql/qgsmssqlsourceselect.cpp

@@ -495,7 +495,7 @@ void QgsMssqlSourceSelect::on_btnConnect_clicked()
 
   bool allowGeometrylessTables = cbxAllowGeometrylessTables->isChecked();
 
-  bool estimateMetadata = settings.value( key + "/estimatedMetadata", true ).toBool();
+  mUseEstimatedMetadata = settings.value( key + "/estimatedMetadata", true ).toBool();
 
   mConnInfo =  "dbname='" + database + '\'';
   if ( !host.isEmpty() )
@@ -585,7 +585,7 @@ void QgsMssqlSourceSelect::on_btnConnect_clicked()
       {
         if ( type == "GEOMETRY" || type.isNull() || srid.isEmpty() )
         {
-          addSearchGeometryColumn( connectionName, layer, estimateMetadata );
+          addSearchGeometryColumn( connectionName, layer, mUseEstimatedMetadata );
           type = "";
           srid = "";
         }

tests/src/python/test_provider_mssql.py

@@ -16,7 +16,7 @@
 
 import os
 
-from qgis.core import QgsVectorLayer, QgsFeatureRequest
+from qgis.core import QgsVectorLayer, QgsFeatureRequest, QgsRectangle
 
 from qgis.PyQt.QtCore import QSettings, QDate, QTime, QDateTime, QVariant
 
@@ -56,6 +56,50 @@ def enableCompiler(self):
     def disableCompiler(self):
         QSettings().setValue(u'/qgis/compileExpressions', False)
 
+    def uncompiledFilters(self):
+        filters = set(['"name" NOT LIKE \'Ap%\'',
+                       '"name" IS NULL',
+                       '"name" IS NOT NULL',
+                       '"name" NOT ILIKE \'QGIS\'',
+                       '"name" NOT ILIKE \'pEAR\'',
+                       'name <> \'Apple\'',
+                       '"name" <> \'apple\'',
+                       '(name = \'Apple\') is not null',
+                       '"name" || \' \' || "cnt" = \'Orange 100\'',
+                       '\'x\' || "name" IS NOT NULL',
+                       '\'x\' || "name" IS NULL',
+                       '"name" ~ \'[OP]ra[gne]+\'',
+                       'false and NULL',
+                       'true and NULL',
+                       'NULL and false',
+                       'NULL and true',
+                       'NULL and NULL',
+                       'false or NULL',
+                       'true or NULL',
+                       'NULL or false',
+                       'NULL or true',
+                       'NULL or NULL',
+                       'not null',
+                       'not name IS NULL',
+                       'not name = \'Apple\'',
+                       'not name = \'Apple\' or name = \'Apple\'',
+                       'not name = \'Apple\' or not name = \'Apple\'',
+                       'not name = \'Apple\' and pk = 4',
+                       'not name = \'Apple\' and not pk = 4',
+                       'intersects($geometry,geom_from_wkt( \'Polygon ((-72.2 66.1, -65.2 66.1, -65.2 72.0, -72.2 72.0, -72.2 66.1))\'))'])
+        return filters
+
+    def partiallyCompiledFilters(self):
+        return set(['name ILIKE \'QGIS\'',
+                    'name = \'Apple\'',
+                    'name = \'apple\'',
+                    'name LIKE \'Apple\'',
+                    'name LIKE \'aPple\'',
+                    '"name"="name2"',
+                    'name ILIKE \'aPple\'',
+                    'name ILIKE \'%pp%\'',
+                    '"name" || \' \' || "name" = \'Orange Orange\''])
+
     # HERE GO THE PROVIDER SPECIFIC TESTS
     def testDateTimeTypes(self):
         vl = QgsVectorLayer('%s table="qgis_test"."date_times" sql=' %
@@ -83,5 +127,34 @@ def testDateTimeTypes(self):
         self.assertEqual(f.attributes()[datetime_idx], QDateTime(
             QDate(2004, 3, 4), QTime(13, 41, 52)))
 
+    def testInvalidGeometries(self):
+        """ Test what happens when SQL Server is a POS and throws an exception on encountering an invalid geometry """
+        vl = QgsVectorLayer('%s srid=4167 type=POLYGON table="qgis_test"."invalid_polys" (ogr_geometry) sql=' %
+                            (self.dbconn), "testinvalid", "mssql")
+        assert(vl.isValid())
+
+        self.assertEqual(vl.dataProvider().extent().toString(1), QgsRectangle(173.953, -41.513, 173.967, -41.502).toString(1))
+
+        #burn through features - don't want SQL server to trip up on the invalid ones
+        count = 0
+        for f in vl.dataProvider().getFeatures():
+            count += 1
+        self.assertEqual(count, 39)
+
+        count = 0
+
+        for f in vl.dataProvider().getFeatures(QgsFeatureRequest(QgsRectangle(173, -42, 174, -41))):
+            count += 1
+        # two invalid geometry features
+        self.assertEqual(count, 37)
+        # sorry... you get NO chance to see these features exist and repair them... because SQL server. Use PostGIS instead and live a happier life!
+
+        # with estimated metadata
+        vl = QgsVectorLayer('%s srid=4167 type=POLYGON  estimatedmetadata=true table="qgis_test"."invalid_polys" (ogr_geometry) sql=' %
+                            (self.dbconn), "testinvalid", "mssql")
+        assert(vl.isValid())
+        self.assertEqual(vl.dataProvider().extent().toString(1), QgsRectangle(173.954, -41.513, 173.967, -41.502).toString(1))
+
+
 if __name__ == '__main__':
     unittest.main()