Skip to content
Permalink
Browse files

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
  • Loading branch information
nyalldawson committed Jun 1, 2018
1 parent bd65fc6 commit e6e54de383168900aec2d06a228e4c43c3fdd041
Showing with 58 additions and 25 deletions.
  1. +14 −4 src/providers/ogr/qgsogrprovider.cpp
  2. +44 −21 tests/src/python/test_provider_ogr_sqlite.py
@@ -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 );
}
@@ -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"""

0 comments on commit e6e54de

Please sign in to comment.
You can’t perform that action at this time.