Add test to ensure that orig_ogc_fid field is only ever used internally

We don't want to expose this field to users, or include it in layer exports or copies And rename internal field to __orig_ogc_fid to avoid clashes with existing datasets which have been exported before this fix and which now contain a orig_ogc_fid field
qgis · Jun 1, 2018 · e6e54de · e6e54de
1 parent bd65fc6
commit e6e54de
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 25 deletions.
diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -92,7 +92,7 @@ static OGRwkbGeometryType ogrWkbGeometryTypeFromName( const QString &typeName );
 
 static bool IsLocalFile( const QString &path );
 
-static const QByteArray ORIG_OGC_FID = "orig_ogc_fid";
+static const QByteArray ORIG_OGC_FID = "__orig_ogc_fid";
 
 QMutex QgsOgrProviderUtils::sGlobalMutex( QMutex::Recursive );
 
@@ -898,6 +898,8 @@ void QgsOgrProvider::loadFields()
   QByteArray fidColumn( mOgrLayer->GetFIDColumn() );
   mFirstFieldIsFid = !fidColumn.isEmpty() &&
                      fdef.GetFieldIndex( fidColumn ) < 0;
+
+  int createdFields = 0;
   if ( mFirstFieldIsFid )
   {
     QgsField fidField(
@@ -914,6 +916,7 @@ void QgsOgrProvider::loadFields()
       fidField
     );
     mDefaultValues.insert( 0, tr( "Autogenerate" ) );
+    createdFields++;
   }
 
   for ( int i = 0; i < fdef.GetFieldCount(); ++i )
@@ -961,6 +964,12 @@ void QgsOgrProvider::loadFields()
     QString name = textEncoding()->toUnicode( OGR_Fld_GetNameRef( fldDef ) );
 #endif
 
+    if ( name == ORIG_OGC_FID )
+    {
+      // don't ever report this field, it's for internal purposes only!
+      continue;
+    }
+
     if ( mAttributeFields.indexFromName( name ) != -1 )
     {
 
@@ -1006,10 +1015,11 @@ void QgsOgrProvider::loadFields()
     QString defaultValue = textEncoding()->toUnicode( OGR_Fld_GetDefault( fldDef ) );
     if ( !defaultValue.isEmpty() && !OGR_Fld_IsDefaultDriverSpecific( fldDef ) )
     {
-      mDefaultValues.insert( i + ( mFirstFieldIsFid ? 1 : 0 ), defaultValue );
+      mDefaultValues.insert( createdFields, defaultValue );
     }
 
     mAttributeFields.append( newField );
+    createdFields++;
   }
 
 }
@@ -3859,15 +3869,15 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, GDALDatasetH ds
       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 = GDALDatasetExecuteSQL( 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 = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr );
     }

diff --git a/tests/src/python/test_provider_ogr_sqlite.py b/tests/src/python/test_provider_ogr_sqlite.py
@@ -245,30 +245,53 @@ def testSubsetStringFids(self):
 
         vl = QgsVectorLayer(tmpfile + "|subset=type=2", 'test', 'ogr')
         self.assertTrue(vl.isValid())
-        self.assertTrue(vl.fields().at(vl.fields().count() - 1).name() == "orig_ogc_fid")
-
-        req = QgsFeatureRequest()
-        req.setFilterExpression("value=16")
-        it = vl.getFeatures(req)
-        f = QgsFeature()
-        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)
 
+        def run_checks():
+            self.assertEqual([f.name() for f in vl.fields()], ['fid', 'GEOMETRY', 'type', 'value'])
+
+            req = QgsFeatureRequest()
+            req.setFilterExpression("value=16")
+            it = vl.getFeatures(req)
+            f = QgsFeature()
+            self.assertTrue(it.nextFeature(f))
+            self.assertTrue(f.id() == 5)
+            self.assertEqual(f.attributes(), [5, NULL, 2, 16])
+            self.assertEqual([field.name() for field in f.fields()], ['fid', 'GEOMETRY', 'type', 'value'])
+
+            req = QgsFeatureRequest()
+            req.setFilterFid(5)
+            it = vl.getFeatures(req)
+            f = QgsFeature()
+            self.assertTrue(it.nextFeature(f))
+            self.assertTrue(f.id() == 5)
+            self.assertEqual(f.attributes(), [5, NULL, 2, 16])
+            self.assertEqual([field.name() for field in f.fields()], ['fid', 'GEOMETRY', 'type', 'value'])
+
+            req = QgsFeatureRequest()
+            req.setFilterFids([5])
+            it = vl.getFeatures(req)
+            f = QgsFeature()
+            self.assertTrue(it.nextFeature(f))
+            self.assertTrue(f.id() == 5)
+            self.assertEqual(f.attributes(), [5, NULL, 2, 16])
+            self.assertEqual([field.name() for field in f.fields()], ['fid', 'GEOMETRY', 'type', 'value'])
+
+            # 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)
+
+        run_checks()
         # Check that subset string is correctly set on reload
         vl.reload()
-        self.assertTrue(vl.fields().at(vl.fields().count() - 1).name() == "orig_ogc_fid")
+        run_checks()
 
     def test_SplitFeature(self):
         """Test sqlite feature can be split"""