add PRIMARY KEY and AUTOINCREMENT for spatialite offlineediting

[lbartoletti]

Description

fixes #5938 (https://issues.qgis.org/issues/5938)

cc @yjacolin

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

• Commit messages are descriptive and explain the rationale for changes
• Commits which fix bugs include fixes #11111 in the commit message next to the description
• Commits which add new features are tagged with [FEATURE] in the commit message
• Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
• I have read the QGIS Coding Standards and this PR complies with them
• This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
• New unit tests have been added for core changes
• I have run the scripts/prepare-commit.sh script before each commit

[m-kuhn]

Does this work if

• the primary key is not an autoincrementing integer (e.g. string, uuid, ...)
• the offline project is used on several devices
• the source database is edited in parallel while someone is adding offline data

[lbartoletti]

Thanks.
I corrected to add the autoincrement only on integers.
For the modifications on both sides, I tested it and I didn't have any problems.

[m-kuhn]

For the modifications on both sides, I tested it and I didn't have any problems.

For my understanding, how does it work?

Let's say you have one feature with an id 1, and you go offline.
Then you add a new feature in the db (id 2) and in the spatialite (also id 2). Will this not give a unique constraint error on sync?

[lbartoletti]

Then you add a new feature in the db (id 2) and in the spatialite (also id 2). Will this not give a unique constraint error on sync?

Yes. db keep id 2 and the feature from spatialite with id 2 up to id 3. I discover this plugin.

[m-kuhn]

Ah yes, that might be taken care of with QgsVectorLayerUtils::createFeature()?

In this case, what's the advantage of having a primary key set if it's not stable? Isn't the original problem (which were sync problems I guess) already solved by the current createFeature() implementation?

What I am worried a bit is about stability of primary keys in relation scenarios. But I must admit I also don't have the ultimate solution yet.

Another thing to verify is that it's compatible with #8035 (but I assume it is).

[lbartoletti]

In this case, what's the advantage of having a primary key set if it's not stable? Isn't the original problem (which were sync problems I guess) already solved by the current createFeature() implementation?

As I understood (and already seen using), you can have relationships with IDs on your offline base.

What I am worried a bit is about stability of primary keys in relation scenarios. But I must admit I also don't have the ultimate solution yet.

Sadly, me too.

[m-kuhn]

As I understood (and already seen using), you can have relationships with IDs on your offline base.

Which is a nice thing to support. But if I understand the above statement right, the primary key will be adjusted on sync back, but not the foreign key. But this is is neither more nor less save than before.

What I still wonder: which scenario does this fix?

[lbartoletti]

Which is a nice thing to support. But if I understand the above statement right, the primary key will be adjusted on sync back, but not the foreign key. But this is is neither more nor less save than before.

What I still wonder: which scenario does this fix?

The case I had met was very simple: no modification directly in the database, it still does not go through offline modifications and then an online import, without any possible conflict, since the modifications are successive.

[nyalldawson]

What's the status here? @m-kuhn are you happy?

[m-kuhn]

To understand this right, the proposal here is:

• As soon as an integer field is the primary key, AUTOINCREMENT will be set on this field in the offline DB
• sqlite is in this case expected to start increasing values based on the current max value (I guess)

For databases with no to very little configuration at all and a single user, this will ease maintenance setup time and maintenance I assume.

How does this behave in a scenario with multiple users or partial extracts from the database (only features in a given area). Will this not lead to id collisions and merge conflicts on sync?

[lbartoletti]

I think this is more of a debate about the process than about the tool. According to our tests (see above), there is a rise in the id so there are no conflicts.
I understand this fear, but I think that in a case like the one mentioned, you have to look at the tools specialized in versioning, right?

[m-kuhn]

I think this is more of a debate about the process than about the tool.

We are talking about a tool that is already in use in processes

According to our tests (see above), there is a rise in the id so there are no conflicts.

... in the processes and workflows that you are targeting in this test

I understand this fear, but I think that in a case like the one mentioned, you have to look at the tools specialized in versioning, right?

I know of several organizations using this tool for parallel data acquisition, they avoid conflicts by working in clearly defined spatially distributed areas. So far they can use offline editing for their workflow and I am afraid (but not sure since I don't have all the system setups in mind) that this might break the workflow.

On the other hand, the big advantage of this is also still a bit unclear to me. Without this change: there is no primary key generated (so it's NULL) and the database will assign one on commit, no?

[lbartoletti]

I can understand your point of view. So, it could be a "not be fixed" and we will close the issue?

@yjacolin what is your opinion?

[m-kuhn]

I'm not totally opposed to it if there is an ultimate benefit of it. I just need to have that one presented here first ;)

It could also be an option (although I don't like options too much) if it's worth it

And there is now also gpkg as offline format which by definition gets a fid, not sure if that also solves some of the issues

[stale]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[stale]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.