From 52bdbdf332536cca7c0077deef5852584fb02360 Mon Sep 17 00:00:00 2001 From: Sandro Mani Date: Mon, 25 Sep 2017 13:19:09 +0200 Subject: [PATCH 1/4] [OGR] Followup: Add orig_ogc_fid as last field to avoid changing field order --- src/providers/ogr/qgsogrfeatureiterator.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp index 2dfd500b47a7..1857b67f08f8 100644 --- a/src/providers/ogr/qgsogrfeatureiterator.cpp +++ b/src/providers/ogr/qgsogrfeatureiterator.cpp @@ -327,11 +327,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 { From d02cca95363c273044a49e6c39b377a9794691de Mon Sep 17 00:00:00 2001 From: Sandro Mani Date: Mon, 25 Sep 2017 13:20:01 +0200 Subject: [PATCH 2/4] [OGR] Ensure orig_ogc_fid is never set as ignored field --- src/providers/ogr/qgsogrfeatureiterator.cpp | 1 + src/providers/ogr/qgsogrprovider.cpp | 6 +++++- tests/src/python/test_provider_ogr_sqlite.py | 12 ++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp index 1857b67f08f8..f49acb65ef30 100644 --- 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(); } diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index a8fe35dcec56..ff35342e77f1 100644 --- a/src/providers/ogr/qgsogrprovider.cpp +++ b/src/providers/ogr/qgsogrprovider.cpp @@ -1030,7 +1030,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 ); + } } } diff --git a/tests/src/python/test_provider_ogr_sqlite.py b/tests/src/python/test_provider_ogr_sqlite.py index beddd200a513..4267face4012 100644 --- 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") From 758ae63dcd10ba4b6962fd9bef9bfdce59f8d4cb Mon Sep 17 00:00:00 2001 From: Sandro Mani Date: Mon, 25 Sep 2017 13:47:54 +0200 Subject: [PATCH 3/4] [OGR] Use a ORIG_OGC_FID constant instead of hard-coding orig_ogc_fid --- src/providers/ogr/qgsogrprovider.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index ff35342e77f1..46298bf3a72a 100644 --- 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 ) { @@ -1031,7 +1033,7 @@ void QgsOgrProviderUtils::setRelevantFields( OGRLayerH ogrLayer, int fieldCount, { // add to ignored fields const char *fieldName = OGR_Fld_GetNameRef( OGR_FD_GetFieldDefn( featDefn, firstAttrIsFid ? i - 1 : i ) ); - if ( qstrcmp( fieldName, "orig_ogc_fid" ) != 0 ) + if ( qstrcmp( fieldName, ORIG_OGC_FID ) != 0 ) { ignoredFields.append( fieldName ); } @@ -3470,7 +3472,7 @@ 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 = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr ); @@ -3478,7 +3480,7 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH // 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 ); } @@ -3500,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; } } From 2c3cbcf200ae4941e875c8fdeba6fe525350770f Mon Sep 17 00:00:00 2001 From: Sandro Mani Date: Mon, 25 Sep 2017 18:55:00 +0200 Subject: [PATCH 4/4] Update server tests for OGR orig_ogc_fid changes --- tests/testdata/qgis_server/wms_getfeatureinfo_filter.txt | 1 + tests/testdata/qgis_server/wms_getfeatureinfo_filter_or.txt | 2 ++ .../testdata/qgis_server/wms_getfeatureinfo_filter_or_utf8.txt | 2 ++ 3 files changed, 5 insertions(+) diff --git a/tests/testdata/qgis_server/wms_getfeatureinfo_filter.txt b/tests/testdata/qgis_server/wms_getfeatureinfo_filter.txt index 9fa1cb1b1111..49cabb4e71ee 100644 --- 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 + diff --git a/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or.txt b/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or.txt index 9fba9719b63d..d1afc764b769 100644 --- a/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or.txt +++ b/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or.txt @@ -8,6 +8,7 @@ Content-Type: text/xml; charset=utf-8 + @@ -15,6 +16,7 @@ Content-Type: text/xml; charset=utf-8 + diff --git a/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or_utf8.txt b/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or_utf8.txt index 9fba9719b63d..d1afc764b769 100644 --- a/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or_utf8.txt +++ b/tests/testdata/qgis_server/wms_getfeatureinfo_filter_or_utf8.txt @@ -8,6 +8,7 @@ Content-Type: text/xml; charset=utf-8 + @@ -15,6 +16,7 @@ Content-Type: text/xml; charset=utf-8 +