Skip to content
Permalink
Browse files

[BUG FIX][Split Tool] Bug fix for split features tool

Fixes #19936 by prioritising existing attributes over provider side defaults in some circumstances
  • Loading branch information
phborba authored and nyalldawson committed Oct 3, 2018
1 parent f410b4a commit e375d1ddd70f7ee66a415f3beb21a00844f7b044
@@ -380,17 +380,32 @@ QgsFeature QgsVectorLayerUtils::createFeature( const QgsVectorLayer *layer, cons
bool checkUnique = true;

// in order of priority:
// 1. passed attribute value and if field does not have a unique constraint like primary key
if ( attributes.contains( idx ) )
{
v = attributes.value( idx );
if ( fields.at( idx ).constraints().constraints() & QgsFieldConstraints::ConstraintUnique
&& QgsVectorLayerUtils::valueExists( layer, idx, v ) )
{
// unique constraint violated
QVariant uniqueValue = QgsVectorLayerUtils::createUniqueValue( layer, idx, v );
if ( uniqueValue.isValid() )
v = uniqueValue;
}
checkUnique = false;
}

// 1. client side default expression
if ( layer->defaultValueDefinition( idx ).isValid() )
// 2. client side default expression
// note - deliberately not using else if!
if ( !v.isValid() && layer->defaultValueDefinition( idx ).isValid() )
{
// client side default expression set - takes precedence over all. Why? Well, this is the only default
// which QGIS users have control over, so we assume that they're deliberately overriding any
// provider defaults for some good reason and we should respect that
v = layer->defaultValue( idx, newFeature, evalContext );
}

// 2. provider side default value clause
// 3. provider side default value clause
// note - not an else if deliberately. Users may return null from a default value expression to fallback to provider defaults
if ( !v.isValid() && fields.fieldOrigin( idx ) == QgsFields::OriginProvider )
{
@@ -403,7 +418,7 @@ QgsFeature QgsVectorLayerUtils::createFeature( const QgsVectorLayer *layer, cons
}
}

// 3. provider side default literal
// 4. provider side default literal
// note - deliberately not using else if!
if ( !v.isValid() && fields.fieldOrigin( idx ) == QgsFields::OriginProvider )
{
@@ -416,13 +431,6 @@ QgsFeature QgsVectorLayerUtils::createFeature( const QgsVectorLayer *layer, cons
}
}

// 4. passed attribute value
// note - deliberately not using else if!
if ( !v.isValid() && attributes.contains( idx ) )
{
v = attributes.value( idx );
}

// last of all... check that unique constraints are respected
// we can't handle not null or expression constraints here, since there's no way to pick a sensible
// value if the constraint is violated
@@ -808,14 +808,21 @@ def testVectorLayerUtilsCreateFeatureWithProviderDefault(self):
default_clause = 'nextval(\'qgis_test."someData_pk_seq"\'::regclass)'
self.assertEqual(vl.dataProvider().defaultValueClause(0), default_clause)

# check that provider default clause takes precedence over passed attribute values
# this also checks that the inbuilt unique constraint handling is bypassed in the case of a provider default clause
# If an attribute map is provided, QgsVectorLayerUtils.createFeature must
# respect it, otherwise default values from provider are checked.
# User's choice will not be respected if the value violates unique constraints.
# See https://issues.qgis.org/issues/19936
f = QgsVectorLayerUtils.createFeature(vl, attributes={1: 5, 3: 'map'})
self.assertEqual(f.attributes(), [default_clause, 5, "'qgis'::text", "'qgis'::text", None, None])
# changed so that createFeature respects user choice
self.assertEqual(f.attributes(), [default_clause, 5, "'qgis'::text", 'map', None, None])

# test take vector layer default value expression overrides postgres provider default clause
vl.setDefaultValueDefinition(3, QgsDefaultValue("'mappy'"))
# test ignore vector layer default value expression overrides postgres provider default clause,
# due to user's choice
f = QgsVectorLayerUtils.createFeature(vl, attributes={1: 5, 3: 'map'})
self.assertEqual(f.attributes(), [default_clause, 5, "'qgis'::text", 'map', None, None])
# Since user did not enter a default for field 3, test must return the default value chosen
f = QgsVectorLayerUtils.createFeature(vl, attributes={1: 5})
self.assertEqual(f.attributes(), [default_clause, 5, "'qgis'::text", 'mappy', None, None])

# See https://issues.qgis.org/issues/15188
@@ -258,21 +258,24 @@ def testCreateFeature(self):
# layer with default value expression
layer.setDefaultValueDefinition(2, QgsDefaultValue('3*4'))
f = QgsVectorLayerUtils.createFeature(layer)
self.assertEqual(f.attributes(), [NULL, NULL, 12.0])
# we expect the default value expression to take precedence over the attribute map
self.assertEqual(f.attributes(), [NULL, NULL, 12])
# we do not expect the default value expression to take precedence over the attribute map
f = QgsVectorLayerUtils.createFeature(layer, attributes={0: 'a', 2: 6.0})
self.assertEqual(f.attributes(), ['a', NULL, 12.0])
self.assertEqual(f.attributes(), ['a', NULL, 6.0])
# layer with default value expression based on geometry
layer.setDefaultValueDefinition(2, QgsDefaultValue('3*$x'))
f = QgsVectorLayerUtils.createFeature(layer, g)
#adjusted so that input value and output feature are the same
self.assertEqual(f.attributes(), [NULL, NULL, 300.0])
layer.setDefaultValueDefinition(2, QgsDefaultValue(None))

# test with violated unique constraints
layer.setFieldConstraint(1, QgsFieldConstraints.ConstraintUnique)
f = QgsVectorLayerUtils.createFeature(layer, attributes={0: 'test_1', 1: 123})
# since field 1 has Unique Constraint, it ignores value 123 that already has been set and sets to 128
self.assertEqual(f.attributes(), ['test_1', 128, NULL])
layer.setFieldConstraint(0, QgsFieldConstraints.ConstraintUnique)
# since field 0 and 1 already have values test_1 and 123, the output must be a new unique value
f = QgsVectorLayerUtils.createFeature(layer, attributes={0: 'test_1', 1: 123})
self.assertEqual(f.attributes(), ['test_4', 128, NULL])

0 comments on commit e375d1d

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