oracle provider: use prepared statements with parameters (implements #…

…16251)
qgis · Mar 2, 2017 · fb409b59a9ab2e748372cf1b0e346cc24144ee7f · fb409b5
1 parent 09e1c8c
commit fb409b59a9ab2e748372cf1b0e346cc24144ee7f
diff --git a/src/providers/oracle/ocispatial/qsql_ocispatial.cpp b/src/providers/oracle/ocispatial/qsql_ocispatial.cpp
@@ -2920,20 +2920,20 @@ void QOCISpatialCols::getValues( QVector<QVariant> &v, int index )
 
 QOCISpatialResultPrivate::QOCISpatialResultPrivate( QOCISpatialResult *q, const QOCISpatialDriver *drv )
     : QSqlCachedResultPrivate( q, drv )
-    , cols( 0 )
+    , cols( nullptr )
     , env( drv_d_func()->env )
-    , err( 0 )
+    , err( nullptr )
     , svc( const_cast<OCISvcCtx*&>( drv_d_func()->svc ) )
-    , sql( 0 )
+    , sql( nullptr )
     , sdoobj()
     , sdoind()
     , transaction( drv_d_func()->transaction )
     , serverVersion( drv_d_func()->serverVersion )
     , prefetchRows( drv_d_func()->prefetchRows )
     , prefetchMem( drv_d_func()->prefetchMem )
     , geometryTDO( drv_d_func()->geometryTDO )
-    , geometryObj( 0 )
-    , geometryInd( 0 )
+    , geometryObj( nullptr )
+    , geometryInd( nullptr )
 {
   ENTER
   int r = OCIHandleAlloc( env,
@@ -2960,7 +2960,14 @@ QOCISpatialResultPrivate::~QOCISpatialResultPrivate()
 
   r = OCIHandleFree( err, OCI_HTYPE_ERROR );
   if ( r != OCI_SUCCESS )
-    qWarning( "~QOCISpatialResult: unable to free statement handle" );
+    qWarning( "~QOCISpatialResult: unable to free error handle" );
+
+  if ( sql )
+  {
+    r = OCIHandleFree( sql, OCI_HTYPE_STMT );
+    if ( r != OCI_SUCCESS )
+      qWarning( "~QOCISpatialResult: unable to free statement handle" );
+  }
 }
 
 
@@ -3009,6 +3016,7 @@ bool QOCISpatialResult::gotoNext( QSqlCachedResult::ValueCache &values, int inde
       break;
     case OCI_SUCCESS_WITH_INFO:
       qOraWarning( "QOCISpatialResult::gotoNext: SuccessWithInfo: ", d->err );
+      qDebug() << "QOCISpatialResult::gotoNext: statement " << lastQuery();
       r = OCI_SUCCESS; //ignore it
       break;
     case OCI_NO_DATA:
@@ -3114,6 +3122,7 @@ bool QOCISpatialResult::prepare( const QString& query )
     r = OCIHandleFree( d->sql, OCI_HTYPE_STMT );
     if ( r != OCI_SUCCESS )
       qOraWarning( "QOCISpatialResult::prepare: unable to free statement handle:", d->err );
+    d->sql = nullptr;
   }
   if ( query.isEmpty() )
     return false;

diff --git a/src/providers/oracle/qgsoracleconn.cpp b/src/providers/oracle/qgsoracleconn.cpp
@@ -123,7 +123,7 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
   {
     QSqlQuery qry( mDatabase );
 
-    if ( !qry.exec( QString( "BEGIN\nDBMS_WM.GotoWorkspace(%1);\nEND;" ).arg( quotedValue( workspace ) ) ) )
+    if ( !qry.prepare( QString( "BEGIN\nDBMS_WM.GotoWorkspace(?);\nEND;" ) ) || !( qry.addBindValue( workspace ), qry.exec() ) )
     {
       mDatabase.close();
       QgsMessageLog::logMessage( tr( "Could not switch to workspace %1 [%2]" ).arg( workspace, qry.lastError().databaseText() ), tr( "Oracle" ) );
@@ -172,11 +172,22 @@ void QgsOracleConn::disconnect()
   deleteLater();
 }
 
-bool QgsOracleConn::exec( QSqlQuery &qry, QString sql )
+bool QgsOracleConn::exec( QSqlQuery &qry, QString sql, const QVariantList &params )
 {
   QgsDebugMsgLevel( QString( "SQL: %1" ).arg( sql ), 4 );
 
-  bool res = qry.exec( sql );
+  bool res = qry.prepare( sql );
+  if ( res )
+  {
+  for ( const auto &param: params )
+    {
+      QgsDebugMsgLevel( QString( " ARG: %1 [%2]" ).arg( param.toString() ).arg( param.typeName() ), 4 );
+      qry.addBindValue( param );
+    }
+
+    res = qry.exec();
+  }
+
   if ( !res )
   {
     QgsDebugMsg( QString( "SQL: %1\nERROR: %2" )
@@ -192,10 +203,10 @@ QStringList QgsOracleConn::pkCandidates( QString ownerName, QString viewName )
   QStringList cols;
 
   QSqlQuery qry( mDatabase );
-  if ( !exec( qry, QString( "SELECT column_name FROM all_tab_columns WHERE owner=%1 AND table_name=%2 ORDER BY column_id" )
-              .arg( quotedValue( ownerName ) ).arg( quotedValue( viewName ) ) ) )
+  if ( !exec( qry, QString( "SELECT column_name FROM all_tab_columns WHERE owner=? AND table_name=? ORDER BY column_id" ),
+              QVariantList() << ownerName << viewName ) )
   {
-    QgsMessageLog::logMessage( tr( "SQL:%1\nerror:%2\n" ).arg( qry.lastQuery() ).arg( qry.lastError().text() ), tr( "Oracle" ) );
+    QgsMessageLog::logMessage( tr( "SQL:%1 [owner:%2 table_name:%3]\nerror:%4\n" ).arg( qry.lastQuery(), qry.lastError().text(), ownerName, viewName ), tr( "Oracle" ) );
     return cols;
   }
 
@@ -242,7 +253,7 @@ bool QgsOracleConn::tableInfo( bool geometryColumnsOnly, bool userTablesOnly, bo
   // sql += " ORDER BY owner,isview,table_name,column_name";
 
   QSqlQuery qry( mDatabase );
-  if ( !exec( qry, sql ) )
+  if ( !exec( qry, sql, QVariantList() ) )
   {
     QgsMessageLog::logMessage( tr( "Querying available tables failed.\nSQL:%1\nerror:%2\n" ).arg( qry.lastQuery() ).arg( qry.lastError().text() ), tr( "Oracle" ) );
     return false;
@@ -451,7 +462,7 @@ void QgsOracleConn::retrieveLayerTypes( QgsOracleLayerProperty &layerProperty, b
   if ( !exec( qry, sql
               .arg( quotedIdentifier( layerProperty.geometryColName ) )
               .arg( table )
-              .arg( where.isEmpty() ? "" : QString( " AND (%1)" ).arg( where ) ) ) )
+              .arg( where.isEmpty() ? "" : QString( " AND (%1)" ).arg( where ) ), QVariantList() ) )
   {
     QgsMessageLog::logMessage( tr( "SQL:%1\nerror:%2\n" )
                                .arg( qry.lastQuery() )
@@ -809,7 +820,7 @@ bool QgsOracleConn::hasSpatial()
   if ( mHasSpatial == -1 )
   {
     QSqlQuery qry( mDatabase );
-    mHasSpatial = exec( qry, "SELECT 1 FROM v$option WHERE parameter='Spatial' AND value='TRUE'" ) && qry.next();
+    mHasSpatial = exec( qry, "SELECT 1 FROM v$option WHERE parameter='Spatial' AND value='TRUE'", QVariantList() ) && qry.next();
   }
 
   return mHasSpatial;
@@ -820,7 +831,7 @@ QString QgsOracleConn::currentUser()
   if ( mCurrentUser.isNull() )
   {
     QSqlQuery qry( mDatabase );
-    if ( exec( qry, "SELECT user FROM dual" ) && qry.next() )
+    if ( exec( qry, "SELECT user FROM dual", QVariantList() ) && qry.next() )
     {
       mCurrentUser = qry.value( 0 ).toString();
     }

diff --git a/src/providers/oracle/qgsoracleconn.h b/src/providers/oracle/qgsoracleconn.h
@@ -24,6 +24,7 @@
 #include <QMap>
 #include <QSet>
 #include <QThread>
+#include <QVariant>
 
 #include "qgis.h"
 #include "qgsdatasourceuri.h"
@@ -172,7 +173,7 @@ class QgsOracleConn : public QObject
     explicit QgsOracleConn( QgsDataSourceUri uri );
     ~QgsOracleConn();
 
-    bool exec( QSqlQuery &qry, QString sql );
+    bool exec( QSqlQuery &qry, QString sql, const QVariantList &params );
 
     //! reference count
     int mRef;

diff --git a/src/providers/oracle/qgsoraclefeatureiterator.cpp b/src/providers/oracle/qgsoraclefeatureiterator.cpp
@@ -38,6 +38,7 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource* sour
     return;
   }
 
+  QVariantList args;
   mQry = QSqlQuery( *mConnection );
 
   if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
@@ -79,17 +80,15 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource* sour
       if ( mSource->mHasSpatialIndex )
       {
         QgsRectangle rect( mRequest.filterRect() );
-        QString bbox = QString( "mdsys.sdo_geometry(2003,%1,NULL,"
-                                "mdsys.sdo_elem_info_array(1,1003,3),"
-                                "mdsys.sdo_ordinate_array(%2,%3,%4,%5)"
-                                ")" )
-                       .arg( mSource->mSrid < 1 ? "NULL" : QString::number( mSource->mSrid ) )
-                       .arg( qgsDoubleToString( rect.xMinimum() ) )
-                       .arg( qgsDoubleToString( rect.yMinimum() ) )
-                       .arg( qgsDoubleToString( rect.xMaximum() ) )
-                       .arg( qgsDoubleToString( rect.yMaximum() ) );
-
-        whereClause = QString( "sdo_filter(%1,%2)='TRUE'" ).arg( QgsOracleProvider::quotedIdentifier( mSource->mGeometryColumn ) ).arg( bbox );
+        QString bbox = QStringLiteral( "mdsys.sdo_geometry(2003,?,NULL,"
+                                       "mdsys.sdo_elem_info_array(1,1003,3),"
+                                       "mdsys.sdo_ordinate_array(?,?,?,?)"
+                                       ")" );
+
+        whereClause = QStringLiteral( "sdo_filter(%1,%2)='TRUE'" )
+                      .arg( QgsOracleProvider::quotedIdentifier( mSource->mGeometryColumn ) ).arg( bbox );
+
+        args << ( mSource->mSrid < 1 ? QVariant( QVariant::Int ) : mSource->mSrid ) << rect.xMinimum() << rect.yMinimum() << rect.xMaximum() << rect.yMaximum();
 
         if (( mRequest.flags() & QgsFeatureRequest::ExactIntersect ) != 0 )
         {
@@ -99,6 +98,7 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource* sour
             whereClause += QString( " AND sdo_relate(%1,%2,'mask=ANYINTERACT')='TRUE'" )
                            .arg( QgsOracleProvider::quotedIdentifier( mSource->mGeometryColumn ) )
                            .arg( bbox );
+            args << ( mSource->mSrid < 1 ? QVariant( QVariant::Int ) : mSource->mSrid ) << rect.xMinimum() << rect.yMinimum() << rect.xMaximum() << rect.yMaximum();
           }
           else
           {
@@ -123,14 +123,14 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource* sour
   {
     case QgsFeatureRequest::FilterFid:
     {
-      QString fidWhereClause = QgsOracleUtils::whereClause( mRequest.filterFid(), mSource->mFields, mSource->mPrimaryKeyType, mSource->mPrimaryKeyAttrs, mSource->mShared );
+      QString fidWhereClause = QgsOracleUtils::whereClause( mRequest.filterFid(), mSource->mFields, mSource->mPrimaryKeyType, mSource->mPrimaryKeyAttrs, mSource->mShared, args );
       whereClause = QgsOracleUtils::andWhereClauses( whereClause, fidWhereClause );
     }
     break;
 
     case QgsFeatureRequest::FilterFids:
     {
-      QString fidsWhereClause = QgsOracleUtils::whereClause( mRequest.filterFids(), mSource->mFields, mSource->mPrimaryKeyType, mSource->mPrimaryKeyAttrs, mSource->mShared );
+      QString fidsWhereClause = QgsOracleUtils::whereClause( mRequest.filterFids(), mSource->mFields, mSource->mPrimaryKeyType, mSource->mPrimaryKeyAttrs, mSource->mShared, args );
       whereClause = QgsOracleUtils::andWhereClauses( whereClause, fidsWhereClause );
     }
     break;
@@ -205,14 +205,15 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource* sour
     if ( !whereClause.isEmpty() )
       whereClause += " AND ";
 
-    whereClause += QString( "rownum<=%1" ).arg( mRequest.limit() );
-    fallbackStatement += QString( "rownum<=%1" ).arg( mRequest.limit() );
+    whereClause += QStringLiteral( "rownum<=?" );
+    fallbackStatement += QStringLiteral( "rownum<=?" );
+    args << QVariant::fromValue( mRequest.limit() );
   }
 
-  bool result = openQuery( whereClause, !useFallback );
+  bool result = openQuery( whereClause, args, !useFallback );
   if ( !result && useFallback )
   {
-    result = openQuery( fallbackStatement );
+    result = openQuery( fallbackStatement, args );
     if ( result )
     {
       mExpressionCompiled = false;
@@ -249,7 +250,7 @@ bool QgsOracleFeatureIterator::fetchFeature( QgsFeature& feature )
     if ( mRewind )
     {
       mRewind = false;
-      if ( !QgsOracleProvider::exec( mQry, mSql ) )
+      if ( !QgsOracleProvider::exec( mQry, mSql, mArgs ) )
       {
         QgsMessageLog::logMessage( QObject::tr( "Fetching features failed.\nSQL:%1\nError: %2" )
                                    .arg( mQry.lastQuery() )
@@ -418,14 +419,14 @@ bool QgsOracleFeatureIterator::close()
 
   if ( mConnection )
     QgsOracleConnPool::instance()->releaseConnection( mConnection );
-  mConnection = 0;
+  mConnection = nullptr;
 
   iteratorClosed();
 
   return true;
 }
 
-bool QgsOracleFeatureIterator::openQuery( QString whereClause, bool showLog )
+bool QgsOracleFeatureIterator::openQuery( QString whereClause, QVariantList args, bool showLog )
 {
   try
   {
@@ -478,7 +479,8 @@ bool QgsOracleFeatureIterator::openQuery( QString whereClause, bool showLog )
 
     QgsDebugMsg( QString( "Fetch features: %1" ).arg( query ) );
     mSql = query;
-    if ( !QgsOracleProvider::exec( mQry, query ) )
+    mArgs = args;
+    if ( !QgsOracleProvider::exec( mQry, query, args ) )
     {
       if ( showLog )
       {

diff --git a/src/providers/oracle/qgsoraclefeatureiterator.h b/src/providers/oracle/qgsoraclefeatureiterator.h
@@ -70,7 +70,7 @@ class QgsOracleFeatureIterator : public QgsAbstractFeatureIteratorFromSource<Qgs
     virtual bool fetchFeature( QgsFeature& feature ) override;
     bool nextFeatureFilterExpression( QgsFeature& f ) override;
 
-    bool openQuery( QString whereClause, bool showLog = true );
+    bool openQuery( QString whereClause, QVariantList args, bool showLog = true );
 
     QgsOracleConn *mConnection = nullptr;
     QSqlQuery mQry;
@@ -79,6 +79,7 @@ class QgsOracleFeatureIterator : public QgsAbstractFeatureIteratorFromSource<Qgs
     bool mFetchGeometry;
     QgsAttributeList mAttributeList;
     QString mSql;
+    QVariantList mArgs;
 };
 
 #endif // QGSORACLEFEATUREITERATOR_H