Avoid double-evaluating postgres default values

nyalldawson · m-kuhn · commit 60bbd0933952 · 2016-11-16T14:19:24.000+01:00
diff --git a/python/core/qgsvectordataprovider.sip b/python/core/qgsvectordataprovider.sip
@@ -239,7 +239,13 @@ class QgsVectorDataProvider : QgsDataProvider
 
     /**
      * Returns any literal default values which are present at the provider for a specified
-     * field index.
+     * field index. Important - this should ONLY be called when creating an attribute to insert
+     * directly into the database. Do not call this method for non-feature creation or modification,
+     * eg when validating an attribute or to compare it against an existing value, as calling it
+     * can cause changes to the underlying data source (eg Postgres provider where the default value
+     * is calculated as a result of a sequence). It is recommended that you instead use the methods
+     * in QgsVectorLayerUtils such as QgsVectorLayerUtils::createFeature()
+     * so that default value handling and validation is automatically carried out.
      * @see defaultValueClause()
      */
     virtual QVariant defaultValue( int fieldIndex ) const;
@@ -257,9 +263,19 @@ class QgsVectorDataProvider : QgsDataProvider
      * Returns any constraints which are present at the provider for a specified
      * field index.
      * @note added in QGIS 3.0
+     * @see skipConstraintCheck()
      */
     QgsFieldConstraints::Constraints fieldConstraints( int fieldIndex ) const;
 
+    /**
+     * Returns true if a constraint check should be skipped for a specified field (eg if
+     * the value returned by defaultValue() is trusted implicitly. An optional attribute value can be
+     * passed which can help refine the skip constraint check.
+     * @note added in QGIS 3.0
+     * @see fieldConstraints()
+     */
+    virtual bool skipConstraintCheck( int fieldIndex, QgsFieldConstraints::Constraint constraint, const QVariant& value = QVariant() ) const;
+
     /**
      * Changes geometries of existing features
      * @param geometry_map   A QgsGeometryMap whose index contains the feature IDs
diff --git a/src/core/qgsvectordataprovider.cpp b/src/core/qgsvectordataprovider.cpp
@@ -114,6 +114,11 @@ QgsFieldConstraints::Constraints QgsVectorDataProvider::fieldConstraints( int fi
   return f.at( fieldIndex ).constraints().constraints();
 }
 
+bool QgsVectorDataProvider::skipConstraintCheck( int, QgsFieldConstraints::Constraint, const QVariant& ) const
+{
+  return false;
+}
+
 bool QgsVectorDataProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
 {
   Q_UNUSED( geometry_map );
diff --git a/src/core/qgsvectordataprovider.h b/src/core/qgsvectordataprovider.h
@@ -293,7 +293,13 @@ class CORE_EXPORT QgsVectorDataProvider : public QgsDataProvider
 
     /**
      * Returns any literal default values which are present at the provider for a specified
-     * field index.
+     * field index. Important - this should ONLY be called when creating an attribute to insert
+     * directly into the database. Do not call this method for non-feature creation or modification,
+     * eg when validating an attribute or to compare it against an existing value, as calling it
+     * can cause changes to the underlying data source (eg Postgres provider where the default value
+     * is calculated as a result of a sequence). It is recommended that you instead use the methods
+     * in QgsVectorLayerUtils such as QgsVectorLayerUtils::createFeature()
+     * so that default value handling and validation is automatically carried out.
      * @see defaultValueClause()
      */
     virtual QVariant defaultValue( int fieldIndex ) const;
@@ -311,9 +317,19 @@ class CORE_EXPORT QgsVectorDataProvider : public QgsDataProvider
      * Returns any constraints which are present at the provider for a specified
      * field index.
      * @note added in QGIS 3.0
+     * @see skipConstraintCheck()
      */
     QgsFieldConstraints::Constraints fieldConstraints( int fieldIndex ) const;
 
+    /**
+     * Returns true if a constraint check should be skipped for a specified field (eg if
+     * the value returned by defaultValue() is trusted implicitly. An optional attribute value can be
+     * passed which can help refine the skip constraint check.
+     * @note added in QGIS 3.0
+     * @see fieldConstraints()
+     */
+    virtual bool skipConstraintCheck( int fieldIndex, QgsFieldConstraints::Constraint constraint, const QVariant& value = QVariant() ) const;
+
     /**
      * Changes geometries of existing features
      * @param geometry_map   A QgsGeometryMap whose index contains the feature IDs
diff --git a/src/core/qgsvectorlayerutils.cpp b/src/core/qgsvectorlayerutils.cpp
@@ -143,7 +143,8 @@ bool QgsVectorLayerUtils::validateAttribute( const QgsVectorLayer* layer, const
   if ( attributeIndex < 0 || attributeIndex >= layer->fields().count() )
     return false;
 
-  QgsField field = layer->fields().at( attributeIndex );
+  QgsFields fields = layer->fields();
+  QgsField field = fields.at( attributeIndex );
   QVariant value = feature.attribute( attributeIndex );
   bool valid = true;
   errors.clear();
@@ -179,24 +180,46 @@ bool QgsVectorLayerUtils::validateAttribute( const QgsVectorLayer* layer, const
        && ( strength == QgsFieldConstraints::ConstraintStrengthNotSet || strength == constraints.constraintStrength( QgsFieldConstraints::ConstraintNotNull ) )
        && ( origin == QgsFieldConstraints::ConstraintOriginNotSet || origin == constraints.constraintOrigin( QgsFieldConstraints::ConstraintNotNull ) ) )
   {
-    valid = valid && !value.isNull();
+    bool exempt = false;
+    if ( fields.fieldOrigin( attributeIndex ) == QgsFields::OriginProvider
+         && constraints.constraintOrigin( QgsFieldConstraints::ConstraintNotNull ) == QgsFieldConstraints::ConstraintOriginProvider )
+    {
+      int providerIdx = fields.fieldOriginIndex( attributeIndex );
+      exempt = layer->dataProvider()->skipConstraintCheck( providerIdx, QgsFieldConstraints::ConstraintNotNull, value );
+    }
 
-    if ( value.isNull() )
+    if ( !exempt )
     {
-      errors << QObject::tr( "value is NULL" );
+      valid = valid && !value.isNull();
+
+      if ( value.isNull() )
+      {
+        errors << QObject::tr( "value is NULL" );
+      }
     }
   }
 
   if ( constraints.constraints() & QgsFieldConstraints::ConstraintUnique
        && ( strength == QgsFieldConstraints::ConstraintStrengthNotSet || strength == constraints.constraintStrength( QgsFieldConstraints::ConstraintUnique ) )
        && ( origin == QgsFieldConstraints::ConstraintOriginNotSet || origin == constraints.constraintOrigin( QgsFieldConstraints::ConstraintUnique ) ) )
   {
-    bool alreadyExists = QgsVectorLayerUtils::valueExists( layer, attributeIndex, value, QgsFeatureIds() << feature.id() );
-    valid = valid && !alreadyExists;
+    bool exempt = false;
+    if ( fields.fieldOrigin( attributeIndex ) == QgsFields::OriginProvider
+         && constraints.constraintOrigin( QgsFieldConstraints::ConstraintNotNull ) == QgsFieldConstraints::ConstraintOriginProvider )
+    {
+      int providerIdx = fields.fieldOriginIndex( attributeIndex );
+      exempt = layer->dataProvider()->skipConstraintCheck( providerIdx, QgsFieldConstraints::ConstraintUnique, value );
+    }
 
-    if ( alreadyExists )
+    if ( !exempt )
     {
-      errors << QObject::tr( "value is not unique" );
+      bool alreadyExists = QgsVectorLayerUtils::valueExists( layer, attributeIndex, value, QgsFeatureIds() << feature.id() );
+      valid = valid && !alreadyExists;
+
+      if ( alreadyExists )
+      {
+        errors << QObject::tr( "value is not unique" );
+      }
     }
   }
 
diff --git a/src/providers/postgres/qgspostgresprovider.cpp b/src/providers/postgres/qgspostgresprovider.cpp
@@ -1806,6 +1806,20 @@ QVariant QgsPostgresProvider::defaultValue( int fieldId ) const
   return QVariant();
 }
 
+bool QgsPostgresProvider::skipConstraintCheck( int fieldIndex, QgsFieldConstraints::Constraint, const QVariant& value ) const
+{
+  if ( providerProperty( EvaluateDefaultValues, false ).toBool() )
+  {
+    return mDefaultValues.contains( fieldIndex );
+  }
+  else
+  {
+    // stricter check - if we are evaluating default values only on commit then we can only bypass the check
+    // if the attribute values matches the original default clause
+    return mDefaultValues.contains( fieldIndex ) && mDefaultValues.value( fieldIndex ) == value.toString();
+  }
+}
+
 QString QgsPostgresProvider::paramValue( const QString& fieldValue, const QString &defaultValue ) const
 {
   if ( fieldValue.isNull() )
diff --git a/src/providers/postgres/qgspostgresprovider.h b/src/providers/postgres/qgspostgresprovider.h
@@ -164,6 +164,7 @@ class QgsPostgresProvider : public QgsVectorDataProvider
     QgsAttributeList pkAttributeIndexes() const override { return mPrimaryKeyAttrs; }
     QString defaultValueClause( int fieldId ) const override;
     QVariant defaultValue( int fieldId ) const override;
+    bool skipConstraintCheck( int fieldIndex, QgsFieldConstraints::Constraint constraint, const QVariant& value = QVariant() ) const override;
 
     /** Adds a list of features
       @return true in case of success and false in case of failure*/
diff --git a/tests/src/python/test_provider_postgres.py b/tests/src/python/test_provider_postgres.py
@@ -520,14 +520,28 @@ def testConstraintOverwrite(self):
     def testVectorLayerUtilsUniqueWithProviderDefault(self):
         vl = QgsVectorLayer('%s table="qgis_test"."someData" sql=' % (self.dbconn), "someData", "postgres")
         default_clause = 'nextval(\'qgis_test."someData_pk_seq"\'::regclass)'
+        vl.dataProvider().setProviderProperty(QgsDataProvider.EvaluateDefaultValues, False)
         self.assertEqual(vl.dataProvider().defaultValueClause(0), default_clause)
         self.assertTrue(QgsVectorLayerUtils.valueExists(vl, 0, 4))
 
         vl.startEditing()
         f = QgsFeature(vl.fields())
         f.setAttribute(0, default_clause)
-        self.assertTrue(vl.addFeatures([f]))
         self.assertFalse(QgsVectorLayerUtils.valueExists(vl, 0, default_clause))
+        self.assertTrue(vl.addFeatures([f]))
+
+        # the default value clause should exist...
+        self.assertTrue(QgsVectorLayerUtils.valueExists(vl, 0, default_clause))
+        # but it should not prevent the attribute being validated
+        self.assertTrue(QgsVectorLayerUtils.validateAttribute(vl, f, 0))
+        vl.rollBack()
+
+    def testSkipConstraintCheck(self):
+        vl = QgsVectorLayer('%s table="qgis_test"."someData" sql=' % (self.dbconn), "someData", "postgres")
+        default_clause = 'nextval(\'qgis_test."someData_pk_seq"\'::regclass)'
+        vl.dataProvider().setProviderProperty(QgsDataProvider.EvaluateDefaultValues, False)
+        self.assertTrue(vl.dataProvider().skipConstraintCheck(0, QgsFieldConstraints.ConstraintUnique, default_clause))
+        self.assertFalse(vl.dataProvider().skipConstraintCheck(0, QgsFieldConstraints.ConstraintUnique, 59))
 
     def testVectorLayerUtilsCreateFeatureWithProviderDefault(self):
         vl = QgsVectorLayer('%s table="qgis_test"."someData" sql=' % (self.dbconn), "someData", "postgres")

Original file line number	Diff line number	Diff line change
`@@ -114,6 +114,11 @@ QgsFieldConstraints::Constraints QgsVectorDataProvider::fieldConstraints( int fi`
`114`	`114`	`return f.at( fieldIndex ).constraints().constraints();`
`115`	`115`	`}`
`116`	`116`
	`117`	`+bool QgsVectorDataProvider::skipConstraintCheck( int, QgsFieldConstraints::Constraint, const QVariant& ) const`
	`118`	`+{`
	`119`	`+ return false;`
	`120`	`+}`
	`121`	`+`
`117`	`122`	`bool QgsVectorDataProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )`
`118`	`123`	`{`
`119`	`124`	`Q_UNUSED( geometry_map );`