[WFS provider] Handle server exposing paging capabilities but obvious…

…ly not implementing it. Handle feature duplicates when server return features without fid/gml:id
qgis · Jun 10, 2016 · 544e8bd · 544e8bd
1 parent 08ead81
commit 544e8bd
Show file tree

Hide file tree

Showing 7 changed files with 168 additions and 51 deletions.
diff --git a/src/providers/wfs/qgswfsfeatureiterator.cpp b/src/providers/wfs/qgswfsfeatureiterator.cpp
@@ -399,6 +399,9 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
   const int maxRetry = s.value( "/qgis/defaultTileMaxRetry", "3" ).toInt();
   int retryIter = 0;
   int lastValidTotalDownloadedFeatureCount = 0;
+  int pagingIter = 1;
+  QString gmlIdFirstFeatureFirstIter;
+  bool disablePaging = false;
   while ( true )
   {
     success = true;
@@ -418,7 +421,7 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
              true, /* forceRefresh */
              false /* cache */ );
 
-    int featureCount = 0;
+    int featureCountForThisResponse = 0;
     while ( true )
     {
       loop.exec( QEventLoop::ExcludeUserInputEvents );
@@ -528,7 +531,6 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
       QVector<QgsGmlStreamingParser::QgsGmlFeaturePtrGmlIdPair> featurePtrList =
         parser->getAndStealReadyFeatures();
 
-      featureCount += featurePtrList.size();
       mTotalDownloadedFeatureCount += featurePtrList.size();
 
       if ( !mStop )
@@ -542,7 +544,29 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
         for ( int i = 0;i < featurePtrList.size();i++ )
         {
           QgsGmlStreamingParser::QgsGmlFeaturePtrGmlIdPair& featPair = featurePtrList[i];
-          featureList.push_back( QgsWFSFeatureGmlIdPair( *( featPair.first ), featPair.second ) );
+          const QgsFeature& f = *( featPair.first );
+          QString gmlId( featPair.second );
+          if ( gmlId.isEmpty() )
+          {
+            // Should normally not happen on sane WFS sources, but can happen with
+            // Geomedia
+            gmlId = QgsWFSUtils::getMD5( f );
+            if ( !mShared->mHasWarnedAboutMissingFeatureId )
+            {
+              QgsDebugMsg( "Server returns features without fid/gml:id. Computing a fake one using feature attributes" );
+              mShared->mHasWarnedAboutMissingFeatureId = true;
+            }
+          }
+          if ( pagingIter == 1 && featureCountForThisResponse == 0 )
+          {
+            gmlIdFirstFeatureFirstIter = gmlId;
+          }
+          else if ( pagingIter == 2 && featureCountForThisResponse == 0 && gmlIdFirstFeatureFirstIter == gmlId )
+          {
+            disablePaging = true;
+            QgsDebugMsg( "Server does not seem to properly support paging since it returned the same first feature for 2 different page requests. Disabling paging" );
+          }
+          featureList.push_back( QgsWFSFeatureGmlIdPair( f, gmlId ) );
           delete featPair.first;
           if (( i > 0 && ( i % 1000 ) == 0 ) || i + 1 == featurePtrList.size() )
           {
@@ -561,6 +585,8 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
 
             featureList.clear();
           }
+
+          featureCountForThisResponse ++;
         }
       }
 
@@ -584,7 +610,7 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
       if ( ++retryIter <= maxRetry )
       {
         QgsMessageLog::logMessage( tr( "Retrying request %1: %2/%3" ).arg( url.toString() ).arg( retryIter ).arg( maxRetry ), tr( "WFS" ) );
-        featureCount = 0;
+        featureCountForThisResponse = 0;
         mTotalDownloadedFeatureCount = lastValidTotalDownloadedFeatureCount;
         continue;
       }
@@ -600,8 +626,18 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
     if ( maxFeatures == 1 )
       break;
     // Detect if we are at the last page
-    if (( mShared->mMaxFeatures > 0 && featureCount < mShared->mMaxFeatures ) || featureCount == 0 )
+    if (( mShared->mMaxFeatures > 0 && featureCountForThisResponse < mShared->mMaxFeatures ) || featureCountForThisResponse == 0 )
       break;
+    ++ pagingIter;
+    if ( disablePaging )
+    {
+      mSupportsPaging = mShared->mCaps.supportsPaging = false;
+      mTotalDownloadedFeatureCount = 0;
+      if ( mShared->mMaxFeaturesWasSetFromDefaultForPaging )
+      {
+        mShared->mMaxFeatures = 0;
+      }
+    }
   }
 
   mStop = true;

diff --git a/src/providers/wfs/qgswfsprovider.cpp b/src/providers/wfs/qgswfsprovider.cpp
@@ -45,6 +45,7 @@
 #include <QWidget>
 #include <QPair>
 #include <QTimer>
+#include <QSettings>
 
 #include <cfloat>
 
@@ -1441,6 +1442,14 @@ bool QgsWFSProvider::getCapabilities()
   else
     mShared->mMaxFeatures = mShared->mCaps.maxFeatures;
 
+  if ( mShared->mMaxFeatures <= 0 && mShared->mCaps.supportsPaging )
+  {
+    QSettings settings;
+    mShared->mMaxFeatures = settings.value( "wfs/max_feature_count_if_not_provided", "1000" ).toInt();
+    mShared->mMaxFeaturesWasSetFromDefaultForPaging = true;
+    QgsDebugMsg( QString( "Server declares paging but does not advertize max feature count and user did not specify it. Using %1" ).arg( mShared->mMaxFeatures ) );
+  }
+
   //find the <FeatureType> for this layer
   QString thisLayerName = mShared->mURI.typeName();
   bool foundLayer = false;

diff --git a/src/providers/wfs/qgswfsshareddata.cpp b/src/providers/wfs/qgswfsshareddata.cpp
@@ -34,16 +34,15 @@
 
 #include <sqlite3.h>
 
-#include <QCryptographicHash>
-
-
 QgsWFSSharedData::QgsWFSSharedData( const QString& uri )
     : mURI( uri )
     , mSourceCRS( 0 )
     , mCacheDataProvider( nullptr )
     , mMaxFeatures( 0 )
+    , mMaxFeaturesWasSetFromDefaultForPaging( false )
     , mHideProgressDialog( mURI.hideDownloadProgressDialog() )
     , mDistinctSelect( false )
+    , mHasWarnedAboutMissingFeatureId( false )
     , mDownloader( nullptr )
     , mDownloadFinished( false )
     , mGenCounter( 0 )
@@ -544,41 +543,6 @@ int QgsWFSSharedData::getUpdatedCounter()
   return mGenCounter ++;
 }
 
-static QString getMD5( const QgsFeature& f )
-{
-  const QgsAttributes attrs = f.attributes();
-  QCryptographicHash hash( QCryptographicHash::Md5 );
-  for ( int i = 0;i < attrs.size();i++ )
-  {
-    const QVariant &v = attrs[i];
-    hash.addData( QByteArray(( const char * )&i, sizeof( i ) ) );
-    if ( v.isNull() )
-    {
-      hash.addData( "#&~NULL#&~", static_cast<int>( strlen( "#&~NULL#&~" ) ) );
-    }
-    else if ( v.type() == QVariant::DateTime )
-    {
-      qint64 val = v.toDateTime().toMSecsSinceEpoch();
-      hash.addData( QByteArray(( const char * )&val, sizeof( val ) ) );
-    }
-    else if ( v.type() == QVariant::Int )
-    {
-      int val = v.toInt();
-      hash.addData( QByteArray(( const char * )&val, sizeof( val ) ) );
-    }
-    else if ( v.type() == QVariant::LongLong )
-    {
-      qint64 val = v.toLongLong();
-      hash.addData( QByteArray(( const char * )&val, sizeof( val ) ) );
-    }
-    else  if ( v.type() == QVariant::String )
-    {
-      hash.addData( v.toByteArray() );
-    }
-  }
-  return hash.result().toHex();
-}
-
 QSet<QString> QgsWFSSharedData::getExistingCachedGmlIds( const QVector<QgsWFSFeatureGmlIdPair>& featureList )
 {
   QString expr;
@@ -651,7 +615,7 @@ QSet<QString> QgsWFSSharedData::getExistingCachedMD5( const QVector<QgsWFSFeatur
       first = false;
     }
     expr += "'";
-    expr += getMD5( featureList[i].first );
+    expr += QgsWFSUtils::getMD5( featureList[i].first );
     expr += "'";
 
     if (( i > 0 && ( i % 1000 ) == 0 ) || i + 1 == featureList.size() )
@@ -842,7 +806,7 @@ void QgsWFSSharedData::serializeFeatures( QVector<QgsWFSFeatureGmlIdPair>& featu
     QString md5;
     if ( mDistinctSelect )
     {
-      md5 = getMD5( gmlFeature );
+      md5 = QgsWFSUtils::getMD5( gmlFeature );
       if ( existingMD5s.contains( md5 ) )
         continue;
       existingMD5s.insert( md5 );

diff --git a/src/providers/wfs/qgswfsshareddata.h b/src/providers/wfs/qgswfsshareddata.h
@@ -150,6 +150,9 @@ class QgsWFSSharedData : public QObject
     /** Server-side or user-side limit of downloaded features (in a single GetFeature()). Valid if > 0 */
     int mMaxFeatures;
 
+    /** Whether mMaxFeatures was set to a non 0 value for the purpose of paging */
+    bool mMaxFeaturesWasSetFromDefaultForPaging;
+
     /** Server capabilities */
     QgsWFSCapabilities::Capabilities mCaps;
 
@@ -162,6 +165,9 @@ class QgsWFSSharedData : public QObject
     /** Bounding box for the layer as returned by GetCapabilities */
     QgsRectangle mCapabilityExtent;
 
+    /** If we have already issued a warning about missing feature ids */
+    bool mHasWarnedAboutMissingFeatureId;
+
     /** Create GML parser */
     QgsGmlStreamingParser* createParser();
 

diff --git a/src/providers/wfs/qgswfsutils.cpp b/src/providers/wfs/qgswfsutils.cpp
@@ -16,6 +16,7 @@
 #include "qgsapplication.h"
 #include "qgslogger.h"
 #include "qgswfsutils.h"
+#include "qgsgeometry.h"
 
 // 1 minute
 #define KEEP_ALIVE_DELAY        (60 * 1000)
@@ -26,6 +27,7 @@
 #include <QSharedMemory>
 #include <QDateTime>
 #include <QSettings>
+#include <QCryptographicHash>
 
 QMutex QgsWFSUtils::gmMutex;
 QThread* QgsWFSUtils::gmThread = nullptr;
@@ -314,4 +316,51 @@ QString QgsWFSUtils::nameSpacePrefix( const QString& tname )
     return QString();
   }
   return splitList.at( 0 );
-}
+}
+
+
+QString QgsWFSUtils::getMD5( const QgsFeature& f )
+{
+  const QgsAttributes attrs = f.attributes();
+  QCryptographicHash hash( QCryptographicHash::Md5 );
+  for ( int i = 0;i < attrs.size();i++ )
+  {
+    const QVariant &v = attrs[i];
+    hash.addData( QByteArray(( const char * )&i, sizeof( i ) ) );
+    if ( v.isNull() )
+    {
+      // nothing to do
+    }
+    else if ( v.type() == QVariant::DateTime )
+    {
+      qint64 val = v.toDateTime().toMSecsSinceEpoch();
+      hash.addData( QByteArray(( const char * )&val, sizeof( val ) ) );
+    }
+    else if ( v.type() == QVariant::Int )
+    {
+      int val = v.toInt();
+      hash.addData( QByteArray(( const char * )&val, sizeof( val ) ) );
+    }
+    else if ( v.type() == QVariant::LongLong )
+    {
+      qint64 val = v.toLongLong();
+      hash.addData( QByteArray(( const char * )&val, sizeof( val ) ) );
+    }
+    else  if ( v.type() == QVariant::String )
+    {
+      hash.addData( v.toByteArray() );
+    }
+  }
+
+  const int attrCount = attrs.size();
+  hash.addData( QByteArray(( const char * )&attrCount, sizeof( attrCount ) ) );
+  const QgsGeometry* geometry = f.constGeometry();
+  if ( geometry )
+  {
+    const unsigned char *geom = geometry->asWkb();
+    int geomSize = geometry->wkbSize();
+    hash.addData( QByteArray(( const char* )geom, geomSize ) );
+  }
+
+  return hash.result().toHex();
+}
diff --git a/src/providers/wfs/qgswfsutils.h b/src/providers/wfs/qgswfsutils.h
@@ -15,6 +15,8 @@
 #ifndef QGSWFSUTILS_H
 #define QGSWFSUTILS_H
 
+#include "qgsfeature.h"
+
 #include <QString>
 #include <QThread>
 #include <QMutex>
@@ -39,6 +41,9 @@ class QgsWFSUtils
     /** Returns namespace prefix (or an empty string if there is no prefix)*/
     static QString nameSpacePrefix( const QString& tname );
 
+    /** Return a unique identifier made from feature content */
+    static QString getMD5( const QgsFeature& f );
+
   protected:
     friend class QgsWFSUtilsKeepAlive;
     static QSharedMemory* createAndAttachSHM();