Skip to content

Commit

Permalink
[ogr] Fix broken stringlist field implementation
Browse files Browse the repository at this point in the history
  • Loading branch information
@nirvn @nyalldawson
nirvn authored and nyalldawson committed May 10, 2021
1 parent f616be2 commit f191404
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 28 deletions.
12 changes: 6 additions & 6 deletions src/core/providers/ogr/qgsogrprovider.cpp
View file
Expand Up @@ -1980,7 +1980,7 @@ bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags, QgsFeatureId
{
OGR_F_UnsetField( feature.get(), ogrAttributeId );
}
else if ( attrVal.isNull() || ( type != OFTString && ( ( attrVal.type() != QVariant::List && attrVal.toString().isEmpty() ) || ( attrVal.type() == QVariant::List && attrVal.toList().empty() ) ) ) )
else if ( attrVal.isNull() || ( type != OFTString && ( ( attrVal.type() != QVariant::List && attrVal.toString().isEmpty() && attrVal.type() != QVariant::StringList && attrVal.toStringList().isEmpty() ) || ( attrVal.type() == QVariant::List && attrVal.toList().empty() ) ) ) )
{
// Starting with GDAL 2.2, there are 2 concepts: unset fields and null fields
// whereas previously there was only unset fields. For a GeoJSON output,
Expand Down Expand Up @@ -2075,12 +2075,13 @@ bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags, QgsFeatureId
int pos = 0;
for ( const QString &string : list )
{
lst[pos] = textEncoding()->fromUnicode( string ).data();
lst[pos] = CPLStrdup( textEncoding()->fromUnicode( string ).data() );
pos++;
}
}
lst[count] = nullptr;
OGR_F_SetFieldStringList( feature.get(), ogrAttributeId, lst );
CSLDestroy( lst );
break;
}

Expand Down Expand Up @@ -2778,8 +2779,7 @@ bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_
}

OGRFieldType type = OGR_Fld_GetType( fd );

if ( it2->isNull() || ( type != OFTString && ( ( it2->type() != QVariant::List && it2->toString().isEmpty() ) || ( it2->type() == QVariant::List && it2->toList().empty() ) ) ) )
if ( it2->isNull() || ( type != OFTString && ( ( it2->type() != QVariant::List && it2->type() != QVariant::StringList && it2->toString().isEmpty() ) || ( it2->type() == QVariant::List && it2->toList().empty() ) || ( it2->type() == QVariant::StringList && it2->toStringList().empty() ) ) ) )
{
// Starting with GDAL 2.2, there are 2 concepts: unset fields and null fields
// whereas previously there was only unset fields. For a GeoJSON output,
Expand All @@ -2796,7 +2796,6 @@ bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_
}
else
{

switch ( type )
{
case OFTInteger:
Expand Down Expand Up @@ -2862,12 +2861,13 @@ bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_
int pos = 0;
for ( const QString &string : list )
{
lst[pos] = textEncoding()->fromUnicode( string ).data();
lst[pos] = CPLStrdup( textEncoding()->fromUnicode( string ).data() );
pos++;
}
}
lst[count] = nullptr;
OGR_F_SetFieldStringList( of.get(), f, lst );
CSLDestroy( lst );
break;
}

Expand Down
6 changes: 4 additions & 2 deletions src/core/qgsvectorfilewriter.cpp
View file
Expand Up @@ -2564,12 +2564,13 @@ gdal::ogr_feature_unique_ptr QgsVectorFileWriter::createFeature( const QgsFeatur
int pos = 0;
for ( const QString &string : list )
{
lst[pos] = mCodec->fromUnicode( string ).data();
lst[pos] = CPLStrdup( mCodec->fromUnicode( string ).data() );
pos++;
}
}
lst[count] = nullptr;
OGR_F_SetFieldStringList( poFeature.get(), ogrField, lst );
CSLDestroy( lst );
}
else
{
Expand All @@ -2592,12 +2593,13 @@ gdal::ogr_feature_unique_ptr QgsVectorFileWriter::createFeature( const QgsFeatur
int pos = 0;
for ( const QString &string : list )
{
lst[pos] = mCodec->fromUnicode( string ).data();
lst[pos] = CPLStrdup( mCodec->fromUnicode( string ).data() );
pos++;
}
}
lst[count] = nullptr;
OGR_F_SetFieldStringList( poFeature.get(), ogrField, lst );
CSLDestroy( lst );
}
else
{
Expand Down
83 changes: 82 additions & 1 deletion tests/src/python/test_provider_ogr.py
View file
Expand Up @@ -559,7 +559,7 @@ def testBinaryField(self):
self.assertIsInstance(features[2]['DATA'], QByteArray)
self.assertEqual(hashlib.md5(features[2]['DATA'].data()).hexdigest(), '4b952b80e4288ca5111be2f6dd5d6809')

def testStringListField(self):
def testGmlStringListField(self):
source = os.path.join(TEST_DATA_DIR, 'stringlist.gml')
vl = QgsVectorLayer(source)
self.assertTrue(vl.isValid())
Expand Down Expand Up @@ -592,6 +592,87 @@ def testStringListField(self):
self.assertEqual(list_field.typeName(), 'StringList')
self.assertEqual(list_field.subType(), QVariant.String)

def testStringListField(self):
tmpfile = os.path.join(self.basetestpath, 'newstringlistfield.geojson')
ds = ogr.GetDriverByName('GeoJSON').CreateDataSource(tmpfile)
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
lyr.CreateField(ogr.FieldDefn('strfield', ogr.OFTString))
lyr.CreateField(ogr.FieldDefn('intfield', ogr.OFTInteger))
lyr.CreateField(ogr.FieldDefn('stringlistfield', ogr.OFTStringList))

f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometry(ogr.CreateGeometryFromWkt('POINT (1 1)'))
f.SetField('strfield', 'one')
f.SetField('intfield', 1)
f.SetFieldStringList(2, ['a', 'b', 'c'])
lyr.CreateFeature(f)

lyr = None
ds = None

vl = QgsVectorLayer(tmpfile)
self.assertTrue(vl.isValid())

dp = vl.dataProvider()
fields = dp.fields()
list_field = fields[fields.lookupField('stringlistfield')]
self.assertEqual(list_field.type(), QVariant.StringList)
self.assertEqual(list_field.typeName(), 'StringList')
self.assertEqual(list_field.subType(), QVariant.String)

f = next(vl.getFeatures())
self.assertEqual(f.attributes(), ['one', 1, ['a', 'b', 'c']])

# add features
f = QgsFeature()
f.setAttributes(['two', 2, ['z', 'y', 'x']])
self.assertTrue(vl.dataProvider().addFeature(f))
f.setAttributes(['three', 3, NULL])
self.assertTrue(vl.dataProvider().addFeature(f))
f.setAttributes(['four', 4, []])
self.assertTrue(vl.dataProvider().addFeature(f))

vl = None

vl = QgsVectorLayer(tmpfile)
self.assertTrue(vl.isValid())

self.assertEqual([f.attributes() for f in vl.getFeatures()],
[['one', 1, ['a', 'b', 'c']],
['two', 2, ['z', 'y', 'x']],
['three', 3, NULL],
['four', 4, NULL]])

# change attribute values
f1_id = [f.id() for f in vl.getFeatures() if f.attributes()[1] == 1][0]
f3_id = [f.id() for f in vl.getFeatures() if f.attributes()[1] == 3][0]
self.assertTrue(vl.dataProvider().changeAttributeValues({f1_id: {2: NULL}, f3_id: {2: ['m', 'n', 'o']}}))

vl = QgsVectorLayer(tmpfile)
self.assertTrue(vl.isValid())

self.assertEqual([f.attributes() for f in vl.getFeatures()],
[['one', 1, NULL],
['two', 2, ['z', 'y', 'x']],
['three', 3, ['m', 'n', 'o']],
['four', 4, NULL]])

# add attribute
self.assertTrue(
vl.dataProvider().addAttributes([QgsField('new_list', type=QVariant.StringList, subType=QVariant.String)]))
f1_id = [f.id() for f in vl.getFeatures() if f.attributes()[1] == 1][0]
f3_id = [f.id() for f in vl.getFeatures() if f.attributes()[1] == 3][0]
self.assertTrue(vl.dataProvider().changeAttributeValues({f1_id: {3: ['111', '222']}, f3_id: {3: ['121', '122', '123']}}))

vl = QgsVectorLayer(tmpfile)
self.assertTrue(vl.isValid())

self.assertEqual([f.attributes() for f in vl.getFeatures()],
[['one', 1, NULL, ['111', '222']],
['two', 2, ['z', 'y', 'x'], NULL],
['three', 3, ['m', 'n', 'o'], ['121', '122', '123']],
['four', 4, NULL, NULL]])

def testIntListField(self):
tmpfile = os.path.join(self.basetestpath, 'newintlistfield.geojson')
ds = ogr.GetDriverByName('GeoJSON').CreateDataSource(tmpfile)
Expand Down
34 changes: 15 additions & 19 deletions tests/src/python/test_qgsvectorfilewriter.py
View file
Expand Up @@ -1060,39 +1060,35 @@ def testWriteWithStringListField(self):
Test writing with a string list field
:return:
"""
basetestpath = tempfile.mkdtemp()
tmpfile = os.path.join(basetestpath, 'newstringlistfield.gml')
ds = ogr.GetDriverByName('GML').CreateDataSource(tmpfile)
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
lyr.CreateField(ogr.FieldDefn('strfield', ogr.OFTString))
lyr.CreateField(ogr.FieldDefn('intfield', ogr.OFTInteger))
lyr.CreateField(ogr.FieldDefn('strlistfield', ogr.OFTStringList))
ds = None

vl = QgsVectorLayer(tmpfile)
self.assertTrue(vl.isValid())
source_fields = QgsFields()
source_fields.append(QgsField('int', QVariant.Int))
source_fields.append(QgsField('stringlist', QVariant.StringList, subType=QVariant.String))
vl = QgsMemoryProviderUtils.createMemoryLayer('test', source_fields)
f = QgsFeature()
f.setAttributes([1, ['ab', 'cd']])
vl.dataProvider().addFeature(f)

# write a gml dataset with a string list field
filename = os.path.join(str(QDir.tempPath()), 'with_stringlist_field.gml')
filename = os.path.join(str(QDir.tempPath()), 'with_stringlist_field.geojson')
rc, errmsg = QgsVectorFileWriter.writeAsVectorFormat(vl,
filename,
'utf-8',
vl.crs(),
'GML')
'GeoJSON')

self.assertEqual(rc, QgsVectorFileWriter.NoError)

# open the resulting gml
# open the resulting geojson
vl = QgsVectorLayer(filename, '', 'ogr')
self.assertTrue(vl.isValid())
fields = vl.fields()

# test type of converted field
idx = fields.indexFromName('strlistfield')
idx = fields.indexFromName('stringlist')
self.assertEqual(fields.at(idx).type(), QVariant.StringList)
self.assertEqual(fields.at(idx).subType(), QVariant.String)

del vl
self.assertEqual([f.attributes() for f in vl.getFeatures()], [[1, ['ab', 'cd']]])

os.unlink(filename)

def testWriteWithIntegerListField(self):
Expand All @@ -1117,7 +1113,7 @@ def testWriteWithIntegerListField(self):

self.assertEqual(rc, QgsVectorFileWriter.NoError)

# open the resulting gml
# open the resulting geojson
vl = QgsVectorLayer(filename, '', 'ogr')
self.assertTrue(vl.isValid())
fields = vl.fields()
Expand Down Expand Up @@ -1153,7 +1149,7 @@ def testWriteWithDoubleListField(self):

self.assertEqual(rc, QgsVectorFileWriter.NoError)

# open the resulting gml
# open the resulting geojson
vl = QgsVectorLayer(filename, '', 'ogr')
self.assertTrue(vl.isValid())
fields = vl.fields()
Expand Down

0 comments on commit f191404

Please sign in to comment.