[afs] Handle invalid responses returned from FeatureServer multipoint

nyalldawson · nyalldawson · commit 244ba5ca3ad0 · 2018-10-25T10:52:02.000+10:00
layers, where individual features may have point geometries Not sure if this is a bug in ArcGIS server (probably, yeah, let's go with definitely. I couldn't check the source to see.) But in general QGIS approach is to be forgiving and do our best to make up for badly behaved servers). See https://community.esri.com/thread/14037
diff --git a/src/providers/arcgisrest/qgsarcgisrestutils.cpp b/src/providers/arcgisrest/qgsarcgisrestutils.cpp
@@ -202,21 +202,31 @@ static std::unique_ptr< QgsPoint > parseEsriGeometryPoint( const QVariantMap &ge
 static std::unique_ptr< QgsMultiPoint > parseEsriGeometryMultiPoint( const QVariantMap &geometryData, QgsWkbTypes::Type pointType )
 {
   // {"points" : [[ <x1>, <y1>, <z1>, <m1> ] , [ <x2>, <y2>, <z2>, <m2> ], ... ]}
-  QVariantList coordsList = geometryData[QStringLiteral( "points" )].toList();
-  if ( coordsList.isEmpty() )
-    return nullptr;
+  const QVariantList coordsList = geometryData[QStringLiteral( "points" )].toList();
 
   std::unique_ptr< QgsMultiPoint > multiPoint = qgis::make_unique< QgsMultiPoint >();
-  Q_FOREACH ( const QVariant &coordData, coordsList )
+  for ( const QVariant &coordData : coordsList )
   {
-    QVariantList coordList = coordData.toList();
+    const QVariantList coordList = coordData.toList();
     std::unique_ptr< QgsPoint > p = parsePoint( coordList, pointType );
     if ( !p )
     {
-      return nullptr;
+      continue;
     }
     multiPoint->addGeometry( p.release() );
   }
+
+  // second chance -- sometimes layers are reported as multipoint but features have single
+  // point geometries. Silently handle this and upgrade to multipoint.
+  std::unique_ptr< QgsPoint > p = parseEsriGeometryPoint( geometryData, pointType );
+  if ( p )
+    multiPoint->addGeometry( p.release() );
+
+  if ( multiPoint->numGeometries() == 0 )
+  {
+    // didn't find any points, so reset geometry to null
+    multiPoint.reset();
+  }
   return multiPoint;
 }
 
diff --git a/tests/src/python/test_provider_afs.py b/tests/src/python/test_provider_afs.py
@@ -710,6 +710,92 @@ def testBboxRestriction(self):
         self.assertEqual(vl.featureCount(), 2)
         self.assertEqual([f['pk'] for f in vl.getFeatures()], [2, 4])
 
+    def testBadMultiPoints(self):
+        """
+        Test invalid server response where a layer's type is multipoint but single point geometries
+        are returned. Thanks Jack. Thack.
+        """
+        endpoint = self.basetestpath + '/multipoint_fake_qgis_http_endpoint'
+        with open(sanitize(endpoint, '?f=json'), 'wb') as f:
+            f.write("""
+        {"currentVersion":10.22,"id":1,"name":"QGIS Test","type":"Feature Layer","description":
+        "QGIS Provider Test Layer.\n","geometryType":"esriGeometryMultipoint","copyrightText":"","parentLayer":{"id":0,"name":"QGIS Tests"},"subLayers":[],
+        "minScale":72225,"maxScale":0,
+        "defaultVisibility":true,
+        "extent":{"xmin":-71.123,"ymin":66.33,"xmax":-65.32,"ymax":78.3,
+        "spatialReference":{"wkid":4326,"latestWkid":4326}},
+        "hasAttachments":false,"htmlPopupType":"esriServerHTMLPopupTypeAsHTMLText",
+        "displayField":"LABEL","typeIdField":null,
+        "fields":[{"name":"OBJECTID","type":"esriFieldTypeOID","alias":"OBJECTID","domain":null}],
+        "relationships":[],"canModifyLayer":false,"canScaleSymbols":false,"hasLabels":false,
+        "capabilities":"Map,Query,Data","maxRecordCount":1000,"supportsStatistics":true,
+        "supportsAdvancedQueries":true,"supportedQueryFormats":"JSON, AMF",
+        "ownershipBasedAccessControlForFeatures":{"allowOthersToQuery":true},"useStandardizedQueries":true}""".encode(
+                'UTF-8'))
+
+        with open(sanitize(endpoint, '/query?f=json_where=OBJECTID=OBJECTID_returnIdsOnly=true'), 'wb') as f:
+            f.write("""
+        {
+         "objectIdFieldName": "OBJECTID",
+         "objectIds": [
+          1,
+          2,
+          3
+         ]
+        }
+        """.encode('UTF-8'))
+
+        # Create test layer
+        vl = QgsVectorLayer("url='http://" + endpoint + "' crs='epsg:4326'", 'test', 'arcgisfeatureserver')
+
+        self.assertTrue(vl.isValid())
+        with open(sanitize(endpoint,
+                           '/query?f=json&objectIds=1,2,3&inSR=4326&outSR=4326&returnGeometry=true&outFields=OBJECTID&returnM=false&returnZ=false'), 'wb') as f:
+            f.write("""
+        {
+         "displayFieldName": "name",
+         "fieldAliases": {
+          "name": "name"
+         },
+         "geometryType": "esriGeometryMultipoint",
+         "spatialReference": {
+          "wkid": 4326,
+          "latestWkid": 4326
+         },
+         "fields":[{"name":"OBJECTID","type":"esriFieldTypeOID","alias":"OBJECTID","domain":null},
+        {"name":"Shape","type":"esriFieldTypeGeometry","alias":"Shape","domain":null}],
+         "features": [
+          {
+           "attributes": {
+            "OBJECTID": 1
+           },
+           "geometry": {
+            "x": -70,
+            "y": 66
+           }
+          },
+          {
+           "attributes": {
+            "OBJECTID": 2
+           },
+           "geometry": null
+          },
+          {
+           "attributes": {
+            "OBJECTID": 3
+           },
+           "geometry":
+           {"points" :[[-68,70],
+           [-22,21]]
+           }
+          }
+         ]
+        }""".encode('UTF-8'))
+
+        features = [f for f in vl.getFeatures()]
+        self.assertEqual(len(features), 3)
+        self.assertEqual([f.geometry().asWkt() for f in features], ['MultiPoint ((-70 66))', '', 'MultiPoint ((-68 70),(-22 21))'])
+
 
 if __name__ == '__main__':
     unittest.main()
