Merge pull request #5251 from manisandro/orig_ocg_fid_218

[2.18][OGR] orig_ogc_fid followups
qgis · Sep 25, 2017 · ce11ebb · ce11ebb
2 parents b2b494d + 2c3cbcf
commit ce11ebb
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 6 deletions.
diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -152,6 +152,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
     OGR_L_SetAttributeFilter( ogrLayer, nullptr );
   }
 
+
   //start with first feature
   rewind();
 }
@@ -327,11 +328,16 @@ bool QgsOgrFeatureIterator::readFeature( OGRFeatureH fet, QgsFeature& feature )
 {
   if ( mOrigFidAdded )
   {
+    OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( ogrLayer );
+    int lastField = OGR_FD_GetFieldCount( fdef ) - 1;
+    if ( lastField >= 0 )
 #if defined(GDAL_VERSION_NUM) && GDAL_VERSION_NUM >= 2000000
-    feature.setFeatureId( OGR_F_GetFieldAsInteger64( fet, 0 ) );
+      feature.setFeatureId( OGR_F_GetFieldAsInteger64( fet, lastField ) );
 #else
-    feature.setFeatureId( OGR_F_GetFieldAsInteger( fet, 0 ) );
+      feature.setFeatureId( OGR_F_GetFieldAsInteger( fet, lastField ) );
 #endif
+    else
+      feature.setFeatureId( OGR_F_GetFID( fet ) );
   }
   else
   {

diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -95,6 +95,8 @@ class QgsCPLErrorHandler
     }
 };
 
+static const QByteArray ORIG_OGC_FID = "orig_ogc_fid";
+
 
 bool QgsOgrProvider::convertField( QgsField &field, const QTextCodec &encoding )
 {
@@ -1030,7 +1032,11 @@ void QgsOgrProviderUtils::setRelevantFields( OGRLayerH ogrLayer, int fieldCount,
       if ( !fetchAttributes.contains( i ) )
       {
         // add to ignored fields
-        ignoredFields.append( OGR_Fld_GetNameRef( OGR_FD_GetFieldDefn( featDefn, firstAttrIsFid ? i - 1 : i ) ) );
+        const char *fieldName = OGR_Fld_GetNameRef( OGR_FD_GetFieldDefn( featDefn, firstAttrIsFid ? i - 1 : i ) );
+        if ( qstrcmp( fieldName, ORIG_OGC_FID ) != 0 )
+        {
+          ignoredFields.append( fieldName );
+        }
       }
     }
 
@@ -3466,15 +3472,15 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH
       fidColumn = "FID";
     }
 
-    QByteArray sql = sqlPart1 + ", " + fidColumn + " as orig_ogc_fid" + sqlPart3;
+    QByteArray sql = sqlPart1 + ", " + fidColumn + " as " + ORIG_OGC_FID + sqlPart3;
     QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
     subsetLayer = OGR_DS_ExecuteSQL( ds, sql.constData(), nullptr, nullptr );
 
     // See https://lists.osgeo.org/pipermail/qgis-developer/2017-September/049802.html
     // If execute SQL fails because it did not find the fidColumn, retry with hardcoded FID
     if ( !subsetLayer )
     {
-      QByteArray sql = sqlPart1 + ", " + "FID as orig_ogc_fid" + sqlPart3;
+      QByteArray sql = sqlPart1 + ", " + "FID as " + ORIG_OGC_FID + sqlPart3;
       QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
       subsetLayer = OGR_DS_ExecuteSQL( ds, sql.constData(), nullptr, nullptr );
     }
@@ -3496,7 +3502,7 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH
     if ( fieldCount > 0 )
     {
       OGRFieldDefnH fldDef = OGR_FD_GetFieldDefn( fdef, fieldCount - 1 );
-      origFidAdded = qstrcmp( OGR_Fld_GetNameRef( fldDef ), "orig_ogc_fid" ) == 0;
+      origFidAdded = qstrcmp( OGR_Fld_GetNameRef( fldDef ), ORIG_OGC_FID ) == 0;
     }
   }
 

diff --git a/tests/src/python/test_provider_ogr_sqlite.py b/tests/src/python/test_provider_ogr_sqlite.py
@@ -184,6 +184,18 @@ def testSubsetStringFids(self):
         self.assertTrue(it.nextFeature(f))
         self.assertTrue(f.id() == 5)
 
+        # Ensure that orig_ogc_fid is still retrieved even if attribute subset is passed
+        req = QgsFeatureRequest()
+        req.setSubsetOfAttributes([])
+        it = vl.getFeatures(req)
+        ids = []
+        while it.nextFeature(f):
+            ids.append(f.id())
+        self.assertTrue(len(ids) == 3)
+        self.assertTrue(3 in ids)
+        self.assertTrue(4 in ids)
+        self.assertTrue(5 in ids)
+
         # Check that subset string is correctly set on reload
         vl.reload()
         self.assertTrue(vl.fields().at(vl.fields().count() - 1).name() == "orig_ogc_fid")

diff --git a/tests/testdata/qgis_server/wms_getfeatureinfo_filter.txt b/tests/testdata/qgis_server/wms_getfeatureinfo_filter.txt
@@ -8,6 +8,7 @@ Content-Type: text/xml; charset=utf-8
    <Attribute value="2" name="id"/>
    <Attribute value="two" name="name"/>
    <Attribute value="two àò" name="utf8nameè"/>
+   <Attribute value="1" name="orig_ogc_fid"/>
    <BoundingBox CRS="EPSG:3857" maxx="913214.6741" minx="913214.6741" maxy="5606017.8743" miny="5606017.8743"/>
    <Attribute value="Point (913214.6741 5606017.8743)" type="derived" name="geometry"/>
   </Feature>

diff --git a/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or.txt b/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or.txt
@@ -8,13 +8,15 @@ Content-Type: text/xml; charset=utf-8
    <Attribute value="2" name="id"/>
    <Attribute value="two" name="name"/>
    <Attribute value="two àò" name="utf8nameè"/>
+   <Attribute value="1" name="orig_ogc_fid"/>
    <BoundingBox CRS="EPSG:3857" maxx="913214.6741" minx="913214.6741" maxy="5606017.8743" miny="5606017.8743"/>
    <Attribute value="Point (913214.6741 5606017.8743)" type="derived" name="geometry"/>
   </Feature>
   <Feature id="2">
    <Attribute value="3" name="id"/>
    <Attribute value="three" name="name"/>
    <Attribute value="three èé↓" name="utf8nameè"/>
+   <Attribute value="2" name="orig_ogc_fid"/>
    <BoundingBox CRS="EPSG:3857" maxx="913204.9128" minx="913204.9128" maxy="5606011.4565" miny="5606011.4565"/>
    <Attribute value="Point (913204.9128 5606011.4565)" type="derived" name="geometry"/>
   </Feature>

diff --git a/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or_utf8.txt b/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or_utf8.txt
@@ -8,13 +8,15 @@ Content-Type: text/xml; charset=utf-8
    <Attribute value="2" name="id"/>
    <Attribute value="two" name="name"/>
    <Attribute value="two àò" name="utf8nameè"/>
+   <Attribute value="1" name="orig_ogc_fid"/>
    <BoundingBox CRS="EPSG:3857" maxx="913214.6741" minx="913214.6741" maxy="5606017.8743" miny="5606017.8743"/>
    <Attribute value="Point (913214.6741 5606017.8743)" type="derived" name="geometry"/>
   </Feature>
   <Feature id="2">
    <Attribute value="3" name="id"/>
    <Attribute value="three" name="name"/>
    <Attribute value="three èé↓" name="utf8nameè"/>
+   <Attribute value="2" name="orig_ogc_fid"/>
    <BoundingBox CRS="EPSG:3857" maxx="913204.9128" minx="913204.9128" maxy="5606011.4565" miny="5606011.4565"/>
    <Attribute value="Point (913204.9128 5606011.4565)" type="derived" name="geometry"/>
   </Feature>