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

Fixes #19936 by prioritising existing attributes over provider side defaults in some circumstances
qgis · Oct 3, 2018 · e375d1d · e375d1d
1 parent f410b4a
commit e375d1d
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 18 deletions.
diff --git a/src/core/qgsvectorlayerutils.cpp b/src/core/qgsvectorlayerutils.cpp
@@ -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

diff --git a/tests/src/python/test_provider_postgres.py b/tests/src/python/test_provider_postgres.py
@@ -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

diff --git a/tests/src/python/test_qgsvectorlayerutils.py b/tests/src/python/test_qgsvectorlayerutils.py
@@ -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])