- solves issue #7550

- the feature modification stops (in edit buffer) once an error is found - the test has been modified to refelect the real problem: the data corruption, and not the commit failure if primary key are not unique
qgis · Jul 11, 2013 · 827ed31 · 827ed31
1 parent 184bf41
commit 827ed31
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 23 deletions.
diff --git a/src/core/qgsvectorlayereditbuffer.cpp b/src/core/qgsvectorlayereditbuffer.cpp
@@ -340,7 +340,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList& commitErrors )
     //
     // delete features
     //
-    if ( !mDeletedFeatureIds.isEmpty() )
+    if ( success && !mDeletedFeatureIds.isEmpty() )
     {
       if (( cap & QgsVectorDataProvider::DeleteFeatures ) && provider->deleteFeatures( mDeletedFeatureIds ) )
       {
@@ -366,7 +366,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList& commitErrors )
     //
     //  add features
     //
-    if ( !mAddedFeatures.isEmpty() )
+    if ( success && !mAddedFeatures.isEmpty() )
     {
       if ( cap & QgsVectorDataProvider::AddFeatures )
       {
@@ -418,7 +418,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList& commitErrors )
   //
   // update geometries
   //
-  if ( !mChangedGeometries.isEmpty() )
+  if ( success && !mChangedGeometries.isEmpty() )
   {
     if (( cap & QgsVectorDataProvider::ChangeGeometries ) && provider->changeGeometryValues( mChangedGeometries ) )
     {

diff --git a/src/core/qgsvectorlayereditutils.cpp b/src/core/qgsvectorlayereditutils.cpp
@@ -244,15 +244,15 @@ int QgsVectorLayerEditUtils::splitFeatures( const QList<QgsPoint>& splitLine, bo
         QgsFeature newFeature;
         newFeature.setGeometry( newGeometry );
 
-        //use default value where possible (primary key issue), otherwise the value from the original (split) feature
+        //use default value where possible for primary key (e.g. autoincrement),
+        //and use the value from the original (split) feature if not primary key
         QgsAttributes newAttributes = select_it->attributes();
-        QVariant defaultValue;
-        for ( int j = 0; j < newAttributes.count(); ++j )
+        foreach ( int pkIdx, L->dataProvider()->pkAttributeIndexes() )
         {
-          defaultValue = L->dataProvider()->defaultValue( j );
+          const QVariant defaultValue = L->dataProvider()->defaultValue( pkIdx );
           if ( !defaultValue.isNull() )
           {
-            newAttributes[ j ] = defaultValue;
+            newAttributes[ pkIdx ] = defaultValue;
           }
         }
 

diff --git a/src/providers/spatialite/qgsspatialiteprovider.cpp b/src/providers/spatialite/qgsspatialiteprovider.cpp
@@ -597,6 +597,7 @@ void QgsSpatiaLiteProvider::loadFieldsAbstractInterface( gaiaVectorLayerPtr lyr
 
   attributeFields.clear();
   mPrimaryKey.clear(); // cazzo cazzo cazzo
+  mPrimaryKeyAttrs.clear();
 
   gaiaLayerAttributeFieldPtr fld = lyr->First;
   if ( fld == NULL )
@@ -692,6 +693,7 @@ void QgsSpatiaLiteProvider::loadFields()
   if ( !isQuery )
   {
     mPrimaryKey.clear();
+    mPrimaryKeyAttrs.clear();
 
     sql = QString( "PRAGMA table_info(%1)" ).arg( quotedIdentifier( mTableName ) );
 
@@ -712,6 +714,8 @@ void QgsSpatiaLiteProvider::loadFields()
           // found a Primary Key column
           pkCount++;
           pkName = name;
+          mPrimaryKeyAttrs << i - 1;
+          QgsDebugMsg( "found primaryKey " + name );
         }
 
         if ( name != mGeometryColumn )
@@ -777,6 +781,8 @@ void QgsSpatiaLiteProvider::loadFields()
         {
           pkCount++;
           pkName = name;
+          mPrimaryKeyAttrs << i - 1;
+          QgsDebugMsg( "found primaryKey " + name );
         }
 
         if ( name != mGeometryColumn )
@@ -3506,7 +3512,7 @@ bool QgsSpatiaLiteProvider::addFeatures( QgsFeatureList & flist )
   QString values;
   QString separator;
   int ia, ret;
-  
+
   if ( flist.size() == 0 )
     return true;
   const QgsAttributes & attributevec = flist[0].attributes();
@@ -3605,13 +3611,6 @@ bool QgsSpatiaLiteProvider::addFeatures( QgsFeatureList & flist )
       if ( fieldname.isEmpty() || fieldname == mGeometryColumn )
         continue;
 
-      // replace primary key with NULL so that sqlite will generate one for us
-      if ( mPrimaryKey == fieldname )
-      { 
-        v = QVariant();
-        assert(v.toString().isEmpty());
-      }
-
       QVariant::Type type = attributeFields[i].type();
       if ( v.toString().isEmpty() )
       {
@@ -5205,3 +5204,8 @@ QGISEXTERN bool deleteLayer( const QString& dbPath, const QString& tableName, QS
   return true;
 }
 
+QgsAttributeList QgsSpatiaLiteProvider::pkAttributeIndexes()
+{
+  return mPrimaryKeyAttrs;
+}
+
diff --git a/src/providers/spatialite/qgsspatialiteprovider.h b/src/providers/spatialite/qgsspatialiteprovider.h
@@ -220,6 +220,12 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider
     */
     QString description() const;
 
+    /**
+     * Return list of indexes of fields that make up the primary key
+     * @note added in 2.0
+     */
+    QgsAttributeList pkAttributeIndexes();
+
   signals:
     /**
      *   This is emitted whenever the worker thread has fully calculated the
@@ -297,6 +303,10 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider
        * Name of the primary key column in the table
        */
     QString mPrimaryKey;
+    /**
+       * List of primary key columns in the table
+       */
+    QgsAttributeList mPrimaryKeyAttrs;
     /**
        * Name of the geometry column in the table
        */

diff --git a/tests/src/python/test_qgsspatialiteprovider.py b/tests/src/python/test_qgsspatialiteprovider.py
@@ -35,22 +35,26 @@ class TestQgsSpatialiteProvider(TestCase):
     @classmethod
     def setUpClass(cls):
         """Run before all tests"""
+        # create test db
         if os.path.exists("test.sqlite") :
             os.remove("test.sqlite")
         con = sqlite3.connect("test.sqlite")
         cur = con.cursor()
         sql = "SELECT InitSpatialMetadata()"
         cur.execute(sql)
-        sql = "CREATE TABLE test_pg (id INTEGER NOT NULL, name TEXT NOT NULL, PRIMARY KEY (id, name))"
+
+        # simple table with primary key
+        sql = "CREATE TABLE test_pg (id INTEGER NOT NULL PRIMARY KEY, name TEXT NOT NULL)"
         cur.execute(sql)
         sql = "SELECT AddGeometryColumn('test_pg', 'geometry', 4326, 'POLYGON', 'XY')"
         cur.execute(sql)
         sql = "INSERT INTO test_pg (id, name, geometry) "
-        sql +=    "VALUES (11, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
-        rs = cur.execute(sql)
+        sql +=    "VALUES (1, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
+        cur.execute(sql)
+
         con.commit()
         con.close()
-        
+
     @classmethod
     def tearDownClass(cls):
         """Run after all tests"""
@@ -74,9 +78,19 @@ def test_SplitFeature(self):
         assert(layer.hasGeometryType())
         layer.startEditing()
         layer.splitFeatures([QgsPoint(0.5, -0.5), QgsPoint(0.5, 1.5)], 0)==0 or die("error in split")
-        layer.commitChanges() or die("error in commit")
-
-
+        layer.splitFeatures([QgsPoint(-0.5, 0.5), QgsPoint(1.5, 0.5)], 0)==0 or die("error in split")
+        if layer.commitChanges():
+            die("this commit should fail")
+        layer.rollBack()
+        feat = QgsFeature()
+        it=layer.getFeatures()
+        it.nextFeature(feat)
+        ref = [[(0,0), (1,0), (1,1), (0,1), (0,0)]]
+        res = feat.geometry().asPolygon()
+        for ring1, ring2 in zip(ref ,res):
+            for p1, p2 in zip(ring1, ring2):
+                for c1, c2 in zip(p1,p2):
+                    c1 == c2 or die("polygon has been altered by failed edition")
 
 if __name__ == '__main__':
     unittest.main()