QGIS » QGIS Application

It occurs also with other providers and is very annoying.

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.

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 ?

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.

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).

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.

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.

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 ?

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).

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) ?

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?

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

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.

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 ?

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

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