Bug report #7550

Split feature on spatialite layers with primary key error

Added by Olivier Dalang almost 11 years ago. Updated over 10 years ago.

Status:Closed
Priority:Severe/Regression
Assignee:-
Category:Digitising
Affected QGIS version:master Regression?:No
Operating System: Easy fix?:No
Pull Request or Patch supplied:Yes Resolution:fixed
Crashes QGIS or corrupts data:Yes Copied to github as #:16493

Description

Hi !

As stated here: #7244

When you split a feature on a spatialite layer that has a primary key (as QGis proposes in the new spatialite layer window), you get the following error:

Errors: ERROR: 2 feature(s) not added.
SUCCESS: 2 geometries were changed.

Provider errors:
  SQLite error: PRIMARY KEY must be unique
SQL: INSERT INTO "teste1"("geometry","pkuid","nome") VALUES (GeomFromWKB(?, 3003),?,?)

This is critical since after exiting edit mode, even if you discard the changes, you loose one part of the splitted feature !

Thanks !

test_qgsspatialiteprovider.py Magnifier (2.52 KB) Vincent Mora, 2013-07-09 06:48 AM


Related issues

Related to QGIS Application - Bug report #5475: Problem to insert splitted geometries in postgis Closed 2012-04-23
Related to QGIS Application - Bug report #7244: Split multipolygon geometry fails Closed 2013-02-27
Related to QGIS Application - Bug report #6060: QGIS 1.8 Split Features doesn't work as in 1.7 Closed 2012-07-18

History

#1 Updated by Giovanni Manghi almost 11 years ago

  • Category set to Vectors
  • Priority changed from High to Severe/Regression

I raise the priority even if I'm not sure if this is a regression, because we cannot ship qgis 2.0 with such critical issue.

#2 Updated by Denis Rouzaud almost 11 years ago

It occurs also with other providers and is very annoying.

#3 Updated by Giovanni Manghi almost 11 years ago

  • Subject changed from Split feature on spatialite layers with primary key error to Split feature on spatialite/postgis layers with primary key error

#4 Updated by Giovanni Manghi almost 11 years ago

  • Status changed from Open to Feedback

it seems fixed with PostGIS, but not with Spatialite, do you confirm?

#5 Updated by Giovanni Manghi almost 11 years ago

Giovanni Manghi wrote:

it seems fixed with PostGIS, but not with Spatialite, do you confirm?

unfortunately I have to correct myself, it is not fixed at all. Moreover when discarding edits features are lost.

#6 Updated by Giovanni Manghi almost 11 years ago

  • Subject changed from Split feature on spatialite/postgis layers with primary key error to Split feature on spatialite layers with primary key error

uff... it is a roller coaster... it seems fixed for PostGIS but still an issue for SL, can you confirm?

#7 Updated by Giovanni Manghi over 10 years ago

  • Category changed from Vectors to Digitising

#8 Updated by Vincent Mora over 10 years ago

  • Assignee set to Vincent Mora

I can confirm that the problem is there with SL and not there with PostGIS.

#9 Updated by Vincent Mora over 10 years ago

Here is a simple test to reproduce the problem.

#10 Updated by Vincent Mora over 10 years ago

There is a flaw in QgsVectorLayerEditBuffer::commitChanges IMO: in the case one of the operations (deleteFeature, addFeature...) fails, the edition cannot be rolled back and the data are corrupted.

COMMIT/ROLLBACK are done after each operation and not globally for QgsVectorLayerEditBuffer::commitChanges.

This is not what this issue is about, but the problem is visible here (if a geometry is split in half, the second half cannot be added because of the non unique key, and the first half replaces the initial geometry).

Maybe another issue ?

#11 Updated by Jürgen Fischer over 10 years ago

Olivier Dalang wrote:

This is critical since after exiting edit mode, even if you discard the changes, you loose one part of the splitted feature!

You don't want to discard the changes, because you already have commited some of the changes (the 2 updates) and discarding the changes means you loose the two pending additions. You can still fix those by assigning a primary key, which they can be committed with. Admittedly that's not very convenient.

Maybe the split operation should bring up a dialog like the merge operation, that lets you set the attributes of the new features - possibly with an option to reset them to a default value or NULL (which might produce a usable primary key).

The provider interface currently has no way to expose to the vector layer, which attributes make up the primary key (or if there even is an attribute based primary key at all) nor to have full transactions, so that you can commit all or nothing.

#12 Updated by Vincent Mora over 10 years ago

For the moment I'm planning to detect primary keys and add new keys for added features (what is done in postgres provider).

This should solve the bug and remain convenient for the user (who can still edit attributes afterward if needed).

#13 Updated by Jürgen Fischer over 10 years ago

Vincent Mora wrote:

For the moment I'm planning to detect primary keys and add new keys for added features (what is done in postgres provider).

The solution for postgres layer was to reset the attributes to their default values in the split tool. If the spatialite provider reported a NULL value as defaultValue for the primary key column, you'd be probably done. In the postgres case that implies that there is a nextval call in default value and for the spatialite case that would imply that the column is auto-incrementing.

#14 Updated by Jürgen Fischer over 10 years ago

Jürgen Fischer wrote:

If the spatialite provider reported a NULL value as defaultValue for the primary key column, you'd be probably done.

Sorry, unfortunately the split tool explicitly ignores defaultValues that are NULL - because that's how columns without a default value are reported.

#15 Updated by Vincent Mora over 10 years ago

OK, so I will start by implementing the getPrimaryKey() for the SL provider,

What you say is that if I set the key to NULL in addFeatures, sqlite generates a unique key for me ?

#16 Updated by Jürgen Fischer over 10 years ago

Vincent Mora wrote:

OK, so I will start by implementing the getPrimaryKey() for the SL provider,

What you say is that if I set the key to NULL in addFeatures, sqlite generates a unique key for me ?

only if the column is set to auto_increment - but that's not mandatory. So there could well be tables where that's not the case (but that wouldn't be handled with postgres layers either).

#17 Updated by Vincent Mora over 10 years ago

Jürgen Fischer wrote:

only if the column is set to auto_increment - but that's not mandatory. So there could well be tables where that's not the case (but that wouldn't be handled with postgres layers either).

So, back to the original plan (generate primary keys for new features) ?

#18 Updated by Jürgen Fischer over 10 years ago

Vincent Mora wrote:

So, back to the original plan (generate primary keys for new features) ?

You have to draw a line somewhere anyway. E.g. what do you do with a primary key that is made up from multiple columns?

#19 Updated by Vincent Mora over 10 years ago

The issue is solved, here is the pull request: https://github.com/qgis/Quantum-GIS/pull/708

#20 Updated by Giovanni Manghi over 10 years ago

  • Pull Request or Patch supplied changed from No to Yes

#21 Updated by Vincent Mora over 10 years ago

The defaultValue is not implemented for SL provider. As a result the pk value of a feature created by split is the same as the original feature (default implementation of defaultValue).

I did implement pkAttributeIndexes for the SL provider.

Since Jürgen suggested the case where PK are not set to autoincrement, I reproduced the problem with a posgres database (primary key value not set to autoincrement).

I'm planning to generate unique values for PK in split (if the field is not set to autoincrement), the user can then edit them manually if need be.

Your feedback is appreciated.

#22 Updated by Vincent Mora over 10 years ago

After trying to generate unique primary key values in split, I realized that this is not an easy endeavor. Moreover, if the user has a primary key that does not auto-increment, I'm not sure it should be done behind his/her back. So I'm back to square one: edition buffer fails to save (because of duplicate keys) and corrupts data even if discarded what should be done.

I tried a very naive approach: in QgsVectorLayerEditBuffer::commitChanges(), stop to touch the database if an error is detected.

It seems to work fine. If the added features (added by split or otherwise) fail to comply to database requirements, the user can discard changes or edit the attribute table to fix the problem. In case of discard, data are not affected.

Is this solution acceptable ?

While editing the feature table (fixing non unique PK) I stubled uppon another problem: if I change the key of the new feature, no problem, but if I change the key of the original feature modified by the split, the geometry modification are lost (I end up with the original geometry).

Should it be considered to be the same issue ?

#23 Updated by Vincent Mora over 10 years ago

Here is a new patch, along the line of what I proposed earlier: https://github.com/qgis/Quantum-GIS/pull/714

#24 Updated by Vincent Mora over 10 years ago

  • Pull Request or Patch supplied changed from Yes to No

Sorry for the PR with several commits in it. I'm going to fix that.

I also have a default behaviour with spatialite that is a bit better for the user, namely, in the split function, if a default value for primary keys is not found, then the value is set to NULL: with spatialite provider it will allow sqlite to generate a unique pk in simple cases (e.g. primary key on single column) and to have obvious things for the user to fix (NULL values) in the attribute table in more complex case (e.g. pk on multiple columns).

#25 Updated by Vincent Mora over 10 years ago

  • Pull Request or Patch supplied changed from No to Yes

#26 Updated by Hugo Mercier over 10 years ago

  • Resolution set to fixed
  • Assignee changed from Vincent Mora to Nathan Woodrow

Nathan, are you ok with the supplied PR ? Can we close ?

#27 Updated by Jürgen Fischer over 10 years ago

  • Assignee deleted (Nathan Woodrow)

#28 Updated by Jürgen Fischer over 10 years ago

  • Status changed from Feedback to Closed

Pull request #715 was merged.

#29 Updated by Vincent Mora over 10 years ago

Bug fixed thanks to fundings from Agence de l'Eau Adour-Garonne.

#30 Updated by Jürgen Fischer over 6 years ago

  • Duplicated by Bug report #17185: Data loss when discarding changes in a PostGIS layer added

#31 Updated by Jürgen Fischer over 6 years ago

  • Duplicated by deleted (Bug report #17185: Data loss when discarding changes in a PostGIS layer)

Also available in: Atom PDF