Merge pull request #41137 from elpaso/bugfix-gh41124-wms-unstable-fids

Fix Server WMS unstable feature IDs
qgis · Jan 23, 2021 · 6746e67 · 6746e67
1 parent 5084507
commit 6746e67
Show file tree

Hide file tree

Showing 22 changed files with 247 additions and 66 deletions.
diff --git a/src/server/services/wfs3/qgswfs3handlers.cpp b/src/server/services/wfs3/qgswfs3handlers.cpp
@@ -21,6 +21,7 @@
 #include "qgsserverrequest.h"
 #include "qgsserverresponse.h"
 #include "qgsserverapiutils.h"
+#include "qgsserverfeatureid.h"
 #include "qgsfeaturerequest.h"
 #include "qgsjsonutils.h"
 #include "qgsogrutils.h"
@@ -31,6 +32,7 @@
 #include "qgsserverinterface.h"
 #include "qgsexpressioncontext.h"
 #include "qgsexpressioncontextutils.h"
+#include "qgsvectorlayerutils.h"
 #include "qgslogger.h"
 
 #ifdef HAVE_SERVER_PYTHON_PLUGINS
@@ -1274,11 +1276,16 @@ void QgsWfs3CollectionsItemsHandler::handleRequest( const QgsServerApiContext &c
       QgsFeatureIterator features { mapLayer->getFeatures( featureRequest ) };
       QgsFeature feat;
       long i { 0 };
+      QMap<QgsFeatureId, QString> fidMap;
+
       while ( features.nextFeature( feat ) )
       {
         // Ignore records before offset
         if ( i >= offset )
+        {
+          fidMap.insert( feat.id(), QgsServerFeatureId::getServerFid( feat, mapLayer->dataProvider()->pkAttributeIndexes() ) );
           featureList << feat;
+        }
         i++;
       }
 
@@ -1307,6 +1314,12 @@ void QgsWfs3CollectionsItemsHandler::handleRequest( const QgsServerApiContext &c
 
       json data = exporter.exportFeaturesToJsonObject( featureList );
 
+      // Patch feature IDs with server feature IDs
+      for ( int i = 0; i < featureList.length(); i++ )
+      {
+        data[ "features" ][ i ]["id"] = fidMap.value( data[ "features" ][ i ]["id"] ).toStdString();
+      }
+
       // Add some metadata
       data["numberMatched"] = matchedFeaturesCount;
       data["numberReturned"] = featureList.count();
@@ -1413,7 +1426,8 @@ void QgsWfs3CollectionsItemsHandler::handleRequest( const QgsServerApiContext &c
         json postData = json::parse( context.request()->data() );
 
         // Process data: extract geometry (because we need to process attributes in a much more complex way)
-        const QgsFeatureList features = QgsOgrUtils::stringToFeatureList( context.request()->data(), mapLayer->fields(), QTextCodec::codecForName( "UTF-8" ) );
+        const QgsFields fields = QgsOgrUtils::stringToFields( context.request()->data(), QTextCodec::codecForName( "UTF-8" ) );
+        const QgsFeatureList features = QgsOgrUtils::stringToFeatureList( context.request()->data(), fields, QTextCodec::codecForName( "UTF-8" ) );
         if ( features.isEmpty() )
         {
           throw QgsServerApiBadRequestException( QStringLiteral( "Posted data does not contain any feature" ) );
@@ -1488,8 +1502,9 @@ void QgsWfs3CollectionsItemsHandler::handleRequest( const QgsServerApiContext &c
         }
         feat.setId( FID_NULL );
 
-        QgsFeatureList featuresToAdd;
-        featuresToAdd.append( feat );
+        QgsVectorLayerUtils::matchAttributesToFields( feat, mapLayer->fields( ) );
+
+        QgsFeatureList featuresToAdd( { feat } );
         if ( ! mapLayer->dataProvider()->addFeatures( featuresToAdd ) )
         {
           throw QgsServerApiInternalServerError( QStringLiteral( "Error adding feature to collection" ) );
@@ -1558,12 +1573,33 @@ void QgsWfs3CollectionsFeatureHandler::handleRequest( const QgsServerApiContext
   // Retrieve feature from storage
   const QString featureId { match.captured( QStringLiteral( "featureId" ) ) };
   QgsFeatureRequest featureRequest = filteredRequest( mapLayer, context );
-  featureRequest.setFilterFid( featureId.toLongLong() );
+  const QString fidExpression { QgsServerFeatureId::getExpressionFromServerFid( featureId, mapLayer->dataProvider() ) };
+  if ( ! fidExpression.isEmpty() )
+  {
+    QgsExpression *filterExpression { featureRequest.filterExpression() };
+    if ( ! filterExpression )
+    {
+      featureRequest.setFilterExpression( fidExpression );
+    }
+    else
+    {
+      featureRequest.setFilterExpression( QStringLiteral( "(%1) AND (%2)" ).arg( fidExpression, filterExpression->expression() ) );
+    }
+  }
+  else
+  {
+    bool ok;
+    featureRequest.setFilterFid( featureId.toLongLong( &ok ) );
+    if ( ! ok )
+    {
+      throw QgsServerApiInternalServerError( QStringLiteral( "Invalid feature ID [%1]" ).arg( featureId ) );
+    }
+  }
   QgsFeature feature;
   QgsFeatureIterator it { mapLayer->getFeatures( featureRequest ) };
-  if ( ! it.nextFeature( feature ) && feature.isValid() )
+  if ( ! it.nextFeature( feature ) || ! feature.isValid() )
   {
-    QgsServerApiInternalServerError( QStringLiteral( "Invalid feature [%1]" ).arg( featureId ) );
+    throw QgsServerApiInternalServerError( QStringLiteral( "Invalid feature [%1]" ).arg( featureId ) );
   }
 
   auto doGet = [ & ]( )
@@ -1583,6 +1619,8 @@ void QgsWfs3CollectionsFeatureHandler::handleRequest( const QgsServerApiContext
     exporter.setAttributes( featureRequest.subsetOfAttributes() );
     exporter.setAttributeDisplayName( true );
     json data = exporter.exportFeatureToJsonObject( feature );
+    // Patch feature ID
+    data["id"] = featureId.toStdString();
     data["links"] = links( context );
     json navigation = json::array();
     const QUrl url { context.request()->url() };
@@ -1651,7 +1689,8 @@ void QgsWfs3CollectionsFeatureHandler::handleRequest( const QgsServerApiContext
         // Parse
         json postData = json::parse( context.request()->data() );
         // Process data: extract geometry (because we need to process attributes in a much more complex way)
-        const QgsFeatureList features = QgsOgrUtils::stringToFeatureList( context.request()->data(), mapLayer->fields(), QTextCodec::codecForName( "UTF-8" ) );
+        const QgsFields fields( QgsOgrUtils::stringToFields( context.request()->data(), QTextCodec::codecForName( "UTF-8" ) ) );
+        const QgsFeatureList features = QgsOgrUtils::stringToFeatureList( context.request()->data(), fields, QTextCodec::codecForName( "UTF-8" ) );
         if ( features.isEmpty() )
         {
           throw QgsServerApiBadRequestException( QStringLiteral( "Posted data does not contain any feature" ) );

diff --git a/src/server/services/wms/qgswmsrenderer.cpp b/src/server/services/wms/qgswmsrenderer.cpp
@@ -2245,12 +2245,16 @@ namespace QgsWms
         if ( featuresNode.isEmpty() )
           continue;
 
+        QMap<QgsFeatureId, QString> fidMap;
+
         for ( int j = 0; j < featuresNode.size(); ++j )
         {
           const QDomElement featureNode = featuresNode.at( j ).toElement();
           const QgsFeatureId fid = featureNode.attribute( QStringLiteral( "id" ) ).toLongLong();
           QgsFeature feature = QgsFeature( vl->getFeature( fid ) );
 
+          fidMap.insert( feature.id(), fid );
+
           QString wkt;
           if ( withGeometry )
           {
@@ -2296,7 +2300,7 @@ namespace QgsWms
 
         for ( const auto &feature : qgis::as_const( features ) )
         {
-          const QString id = QStringLiteral( "%1.%2" ).arg( layerName ).arg( feature.id() );
+          const QString id = QStringLiteral( "%1.%2" ).arg( layerName ).arg( fidMap.value( feature.id() ) );
           json["features"].push_back( exporter.exportFeatureToJsonObject( feature, QVariantMap(), id ) );
         }
       }
@@ -2347,7 +2351,13 @@ namespace QgsWms
   {
     //qgs:%TYPENAME%
     QDomElement typeNameElement = doc.createElement( "qgs:" + typeName /*qgs:%TYPENAME%*/ );
-    typeNameElement.setAttribute( QStringLiteral( "fid" ), typeName + "." + QString::number( feat->id() ) );
+    QString fid;
+    if ( layer && layer->dataProvider() )
+      fid = QgsServerFeatureId::getServerFid( *feat, layer->dataProvider()->pkAttributeIndexes() );
+    else
+      fid = feat->id();
+
+    typeNameElement.setAttribute( QStringLiteral( "fid" ), QStringLiteral( "%1.%2" ).arg( typeName, fid ) );
 
     QgsCoordinateTransform transform;
     if ( layer && layer->crs() != crs )
@@ -2375,8 +2385,7 @@ namespace QgsWms
       {
         try
         {
-          QgsRectangle transformedBox = transform.transformBoundingBox( box );
-          box = transformedBox;
+          box = transform.transformBoundingBox( box );
         }
         catch ( QgsCsException &e )
         {

diff --git a/tests/src/python/CMakeLists.txt b/tests/src/python/CMakeLists.txt
@@ -356,6 +356,9 @@ if (ENABLE_PGTEST)
   ADD_PYTHON_TEST(PyQgsDatabaseSchemaComboBox test_qgsdatabaseschemacombobox.py)
   ADD_PYTHON_TEST(PyQgsDatabaseTableComboBox test_qgsdatabasetablecombobox.py)
   ADD_PYTHON_TEST(PyQgsProviderConnectionPostgres test_qgsproviderconnection_postgres.py)
+  if (WITH_SERVER)
+    ADD_PYTHON_TEST(PyQgsServerWMSGetFeatureInfoPG test_qgsserver_wms_getfeatureinfo_postgres.py)
+  endif()
 endif()
 
 if (ENABLE_MSSQLTEST)

diff --git a/tests/src/python/test_qgsserver.py b/tests/src/python/test_qgsserver.py
@@ -41,7 +41,7 @@
 from io import StringIO
 from qgis.server import QgsServer, QgsServerRequest, QgsBufferServerRequest, QgsBufferServerResponse
 from qgis.core import QgsRenderChecker, QgsApplication, QgsFontUtils, QgsMultiRenderChecker
-from qgis.testing import unittest
+from qgis.testing import unittest, start_app
 from qgis.PyQt.QtCore import QSize
 from utilities import unitTestDataPath
 
@@ -50,6 +50,8 @@
 import base64
 
 
+start_app()
+
 # Strip path and content length because path may vary
 RE_STRIP_UNCHECKABLE = br'MAP=[^"]+|Content-Length: \d+'
 RE_ELEMENT = br'</*([^>\[\s]+)[ >]'
@@ -106,14 +108,6 @@ def assertXMLEqual(self, response, expected, msg='', raw=False):
                 self.assertEqual(expected_values, response_values, msg=msg + "\nXML attribute values differ at line {0}: {1} != {2}".format(line_no, expected_values, response_values))
             line_no += 1
 
-    @classmethod
-    def setUpClass(cls):
-        cls.app = QgsApplication([], False)
-
-    @classmethod
-    def tearDownClass(cls):
-        cls.app.exitQgis()
-
     def setUp(self):
         """Create the server instance"""
         self.fontFamily = QgsFontUtils.standardTestFontFamily()
@@ -135,6 +129,7 @@ def setUp(self):
                 del os.environ[ev]
             except KeyError:
                 pass
+
         self.server = QgsServer()
 
         # Disable landing page API to test standard legacy XML responses in case of errors

diff --git a/tests/src/python/test_qgsserver_accesscontrol.py b/tests/src/python/test_qgsserver_accesscontrol.py
@@ -138,7 +138,6 @@ def _execute_request(cls, qs, requestMethod=QgsServerRequest.GetMethod, data=Non
     @classmethod
     def setUpClass(cls):
         """Run before all tests"""
-        cls._app = QgsApplication([], False)
         cls._server = QgsServer()
         cls._execute_request("")
         cls._server_iface = cls._server.serverInterface()
@@ -149,7 +148,6 @@ def setUpClass(cls):
     def tearDownClass(cls):
         """Run after all tests"""
         del cls._server
-        cls._app.exitQgis()
 
     def setUp(self):
         super().setUp()

diff --git a/tests/src/python/test_qgsserver_api.py b/tests/src/python/test_qgsserver_api.py
@@ -1144,6 +1144,23 @@ def test_wfs3_excluded_attributes(self):
             request, project, 'test_wfs3_collections_items_exclude_attribute_0.json')
         self.assertEqual(response.statusCode(), 200)
 
+    def test_wfs3_invalid_fids(self):
+        """Test exceptions for invalid fids"""
+
+        project = QgsProject()
+        project.read(unitTestDataPath('qgis_server') + '/test_project_api.qgs')
+        request = QgsBufferServerRequest(
+            'http://server.qgis.org/wfs3/collections/exclude_attribute/items/123456.geojson')
+        response = QgsBufferServerResponse()
+        self.server.handleRequest(request, response, project)
+        self.assertEqual(bytes(response.body()).decode('utf-8'), '[{"code":"Internal server error","description":"Invalid feature [123456]"}]')
+
+        request = QgsBufferServerRequest(
+            'http://server.qgis.org/wfs3/collections/exclude_attribute/items/xYz@#.geojson')
+        response = QgsBufferServerResponse()
+        self.server.handleRequest(request, response, project)
+        self.assertEqual(bytes(response.body()).decode('utf-8'), '[{"code":"Internal server error","description":"Invalid feature ID [xYz@]"}]')
+
     def test_wfs3_time_filters_ranges(self):
         """Test datetime filters"""
 
@@ -1887,6 +1904,8 @@ def testOgcApiHandler(self):
         self.assertTrue(
             h2.templatePath(ctx).endswith('/resources/server/api/ogc/templates/services/api2/handlerTwo.html'))
 
+        del(project)
+
     def testOgcApiHandlerContentType(self):
         """Test OGC API Handler content types"""
 
@@ -1936,6 +1955,8 @@ def testOgcApiHandlerContentType(self):
             'http://localhost:8000/project/7ecb/wfs3/collections/zg.grundnutzung.html')
         self.assertEqual(h3.contentTypeFromRequest(req), QgsServerOgcApi.HTML)
 
+        del(project)
+
     def testOgcApiHandlerException(self):
         """Test OGC API Handler exception"""
 
@@ -1966,6 +1987,8 @@ def testOgcApiHandlerException(self):
         self.assertEqual(
             str(ex.exception), "UTF-8 Exception 2 $ù~à^£")
 
+        del(project)
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/src/python/test_qgsserver_security.py b/tests/src/python/test_qgsserver_security.py
@@ -39,7 +39,7 @@ def setUpClass(cls):
 
     @classmethod
     def tearDownClass(cls):
-        cls.app.exitQgis()
+
         try:
             os.remove(cls.db_clone)
         except OSError: