Skip to content

Commit

Permalink
Improvements in QgsPostgresProvider::changeFeatures
Browse files Browse the repository at this point in the history 
Included support for json/jsonb in this method, as there already exists
for QgsPostgresProvider::changeAttributeValues; fixed how the provider
deals with bigint primary keys in this case; added a few tests that
exercise changing of attributes, geometries, and primary keys of
features.
  • Loading branch information
@espinafre
espinafre committed Jun 17, 2020
1 parent 76946dd commit 3f59379
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 6 deletions.
47 changes: 41 additions & 6 deletions src/providers/postgres/qgspostgresprovider.cpp
View file
Expand Up @@ -3013,9 +3013,14 @@ bool QgsPostgresProvider::changeAttributeValues( const QgsChangedAttributesMap &
if ( result.PQresultStatus() != PGRES_COMMAND_OK && result.PQresultStatus() != PGRES_TUPLES_OK )
throw PGException( result );
}
else // let the user know that no field was actually changed
{
QgsLogger::warning( tr( "No fields were updated on the database." ) );
}

// update feature id map if key was changed
if ( pkChanged && mPrimaryKeyType == PktFidMap )
// PktInt64 also uses a fid map even if it is a stand alone field.
if ( pkChanged && ( mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktInt64 ) )
{
QVariantList k = mShared->removeFid( fid );

Expand Down Expand Up @@ -3305,6 +3310,8 @@ bool QgsPostgresProvider::changeFeatures( const QgsChangedAttributesMap &attr_ma

// cycle through the changed attributes of the feature
QString delim;
int numChangedFields = 0;

for ( QgsAttributeMap::const_iterator siter = attrs.constBegin(); siter != attrs.constEnd(); ++siter )
{
try
Expand All @@ -3313,6 +3320,14 @@ bool QgsPostgresProvider::changeFeatures( const QgsChangedAttributesMap &attr_ma

pkChanged = pkChanged || mPrimaryKeyAttrs.contains( siter.key() );

if ( mGeneratedValues.contains( siter.key() ) && !mGeneratedValues.value( siter.key(), QString() ).isEmpty() )
{
QgsLogger::warning( tr( "Changing the value of GENERATED field %1 is not allowed." ).arg( fld.name() ) );
continue;
}

numChangedFields++;

sql += delim + QStringLiteral( "%1=" ).arg( quotedIdentifier( fld.name() ) );
delim = ',';

Expand All @@ -3327,6 +3342,16 @@ bool QgsPostgresProvider::changeFeatures( const QgsChangedAttributesMap &attr_ma
sql += QStringLiteral( "st_geographyfromewkt(%1)" )
.arg( quotedValue( siter->toString() ) );
}
else if ( fld.typeName() == QLatin1String( "jsonb" ) )
{
sql += QStringLiteral( "%1::jsonb" )
.arg( quotedJsonValue( siter.value() ) );
}
else if ( fld.typeName() == QLatin1String( "json" ) )
{
sql += QStringLiteral( "%1::json" )
.arg( quotedJsonValue( siter.value() ) );
}
else if ( fld.typeName() == QLatin1String( "bytea" ) )
{
sql += quotedByteaValue( siter.value() );
Expand All @@ -3344,11 +3369,20 @@ bool QgsPostgresProvider::changeFeatures( const QgsChangedAttributesMap &attr_ma

if ( !geometry_map.contains( fid ) )
{
sql += QStringLiteral( " WHERE %1" ).arg( whereClause( fid ) );
// Don't try to UPDATE an empty set of values (might happen if the table only has GENERATED fields,
// or if the user only changed GENERATED fields in the form/attribute table.
if ( numChangedFields > 0 )
{
sql += QStringLiteral( " WHERE %1" ).arg( whereClause( fid ) );

QgsPostgresResult result( conn->PQexec( sql ) );
if ( result.PQresultStatus() != PGRES_COMMAND_OK && result.PQresultStatus() != PGRES_TUPLES_OK )
throw PGException( result );
QgsPostgresResult result( conn->PQexec( sql ) );
if ( result.PQresultStatus() != PGRES_COMMAND_OK && result.PQresultStatus() != PGRES_TUPLES_OK )
throw PGException( result );
}
else // let the user know that nothing has actually changed
{
QgsLogger::warning( tr( "No fields/geometries were updated on the database." ) );
}
}
else
{
Expand All @@ -3375,7 +3409,8 @@ bool QgsPostgresProvider::changeFeatures( const QgsChangedAttributesMap &attr_ma
}

// update feature id map if key was changed
if ( pkChanged && mPrimaryKeyType == PktFidMap )
// PktInt64 also uses a fid map even though it is a single field.
if ( pkChanged && ( mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktInt64 ) )
{
QVariantList k = mShared->removeFid( fid );

Expand Down
111 changes: 111 additions & 0 deletions tests/src/python/test_provider_postgres.py
View file
Expand Up @@ -1123,8 +1123,21 @@ def testJson(self):
{'a': 123, 'b': 123.34, 'c': 'a string', 'd': [
1, 2, 3], 'e': {'a': 123, 'b': 123.45}}
)
attrs2 = (
246,
2466.91,
None,
True,
False,
r"Yet another string literal with \"quotes\" 'and' other funny chars: π []{};#/èé*",
[2, 4, 3.14159, None],
[True, False],
{'a': 246, 'b': 246.68, 'c': 'a rounded area: π × r²', 'd': [
1, 2, 3], 'e': {'a': 246, 'b': 246.91}}
)
json_idx = vl.fields().lookupField('jvalue')
jsonb_idx = vl.fields().lookupField('jbvalue')

for attr in attrs:
# Add a new feature
vl2 = QgsVectorLayer('%s table="qgis_test"."json" sql=' % (
Expand Down Expand Up @@ -1154,6 +1167,21 @@ def testJson(self):
f = vl2.getFeature(fid)
self.assertEqual(f.attributes(), [fid, attr, attr])

# Let's check changeFeatures:
for attr in attrs2:
vl2 = QgsVectorLayer('%s table="qgis_test"."json" sql=' % (
self.dbconn), "testjson", "postgres")
fid = [f.id() for f in vl2.getFeatures()][-1]
self.assertTrue(vl2.startEditing())
self.assertTrue(vl2.dataProvider().changeFeatures({fid: {json_idx: attr, jsonb_idx: attr}}, {}))
self.assertTrue(vl2.commitChanges())

# Read back again
vl2 = QgsVectorLayer('%s table="qgis_test"."json" sql=' % (
self.dbconn), "testjson", "postgres")
f = vl2.getFeature(fid)
self.assertEqual(f.attributes(), [fid, attr, attr])

def testStringArray(self):
vl = QgsVectorLayer('%s table="qgis_test"."string_array" sql=' % (
self.dbconn), "teststringarray", "postgres")
Expand Down Expand Up @@ -2215,6 +2243,67 @@ def testConstraints(self):
self.assertFalse(self.vl.dataProvider().fieldConstraints(
idx) & QgsFieldConstraints.ConstraintUnique)

def testCompoundPkChanges(self):
""" Check if fields with compound primary keys can be changed """
vl = self.vl

self.assertTrue(vl.isValid())
idx_key1 = vl.fields().lookupField('key1')
idx_key2 = vl.fields().lookupField('key2')
# the name "pk" for this datasource is misleading;
# the primary key is actually composed by the fields key1 and key2
idx_pk = vl.fields().lookupField('pk')
idx_name = vl.fields().lookupField('name')
idx_name2 = vl.fields().lookupField('name2')

geomwkt = 'Point(-47.945 -15.812)'

# start editing ordinary attribute.
ft1 = next(vl.getFeatures(QgsFeatureRequest().setFilterExpression("key1 = 2 AND key2 = 2")))
self.assertTrue(ft1.isValid())
original_geometry = ft1.geometry().asWkt()

vl.startEditing()
self.assertTrue(vl.changeAttributeValues(ft1.id(), {idx_name: 'Rose'}))
self.assertTrue(vl.commitChanges())

# check change
ft2 = next(vl.getFeatures(QgsFeatureRequest().setFilterExpression("key1 = 2 AND key2 = 2")))
self.assertEqual(ft2['name'], 'Rose')
self.assertEqual(ft2['name2'], 'Apple')
self.assertEqual(ft2['pk'], 2)

# now, start editing one of the PK field components
vl.startEditing()

self.assertTrue(vl.dataProvider().changeFeatures({ft2.id(): {idx_key2: 42, idx_name: 'Orchid', idx_name2: 'Daisy'}}, {ft2.id(): QgsGeometry.fromWkt(geomwkt)}))
self.assertTrue(vl.commitChanges())

# let's check if we still have the same fid...
ft2 = next(vl.getFeatures(QgsFeatureRequest().setFilterFid(ft2.id())))
self.assertEqual(ft2['key2'], 42)
self.assertEqual(ft2['name'], 'Orchid')
self.assertEqual(ft2['name2'], 'Daisy')
self.assertTrue(vl.startEditing())
vl.changeAttributeValues(ft2.id(), {idx_key1: 21, idx_name2: 'Hibiscus'})
self.assertTrue(vl.commitChanges())
ft2 = next(vl.getFeatures(QgsFeatureRequest().setFilterFid(ft2.id())))
self.assertEqual(ft2['key1'], 21)
self.assertEqual(ft2['name2'], 'Hibiscus')

# lets get a brand new feature and check how it went...
ft3 = next(vl.getFeatures(QgsFeatureRequest().setFilterExpression('pk = 2')))
self.assertEqual(ft3['name'], 'Orchid')
self.assertEqual(ft3['key1'], 21)
self.assertEqual(ft3['key2'], 42)

assert compareWkt(ft3.geometry().asWkt(), geomwkt), "Geometry mismatch. Expected: {} Got: {}\n".format(ft3.geometry().asWkt(), geomwkt)

# Now, we leave the record as we found it, so further tests can proceed
vl.startEditing()
self.assertTrue(vl.dataProvider().changeFeatures({ft3.id(): {idx_key1: 2, idx_key2: 2, idx_pk: 2, idx_name: 'Apple', idx_name2: 'Apple'}}, {ft3.id(): QgsGeometry.fromWkt(original_geometry)}))
self.assertTrue(vl.commitChanges())


class TestPyQgsPostgresProviderBigintSinglePk(unittest.TestCase, ProviderTestCase):

Expand Down Expand Up @@ -2494,6 +2583,28 @@ def testAddFeature(self):
self.assertFalse(l.dataProvider().addFeatures([f1, f2]),
'Provider reported no AddFeatures capability, but returned true to addFeatures')

def testModifyPk(self):
""" Check if we can modify a primary key value. Since this PK is bigint, we also exercise the mapping between fid and values """

vl = self.getEditableLayer()
self.assertTrue(vl.isValid())
geomwkt = 'Point(-47.945 -15.812)'

feature = next(vl.getFeatures(QgsFeatureRequest().setFilterExpression('pk = 4')))
self.assertTrue(feature.isValid())

self.assertTrue(vl.startEditing())
idxpk = vl.fields().lookupField('pk')

self.assertTrue(vl.dataProvider().changeFeatures({feature.id(): {idxpk: 42}}, {feature.id(): QgsGeometry.fromWkt(geomwkt)}))
self.assertTrue(vl.commitChanges())

# read back
ft = next(vl.getFeatures(QgsFeatureRequest().setFilterExpression('pk = 42')))
self.assertTrue(ft.isValid())
self.assertEqual(ft['name'], 'Honey')
assert compareWkt(ft.geometry().asWkt(), geomwkt), "Geometry mismatch. Expected: {} Got: {}\n".format(ft.geometry().asWkt(), geomwkt)

def testDuplicatedFieldNamesInQueryLayers(self):
"""Test regresssion GH #36205"""

Expand Down

0 comments on commit 3f59379

Please sign in to comment.