Allow adding new features including the PK value on update duplicate mode

[signedav]

Especially on synchronizing and migration it's important that the pk is used from external dataset as well.

Means it should not touch the pk on update (as before) but add new ones from the source (although they are automatically generated in the target).

This will not be the case with no-action. With no-action it will just append (with auto-generated pk's in the target).

This PR introduces the change, that on update-duplicate-mode it uses the PK from the source on adding, but on updating an existing one (duplicate) it does not append the PK to the updated attrs.

Use Case:

I use this algorithm in a model where I import datasets into my main database. The objects in the datasets have IDs (UUIDs), as do the objects in the main database. When I import data, I want existing objects to be updated and new objects to be appended. The new objects should keep the ID from the dataset, meaning they should be inserted with their UUID as the primary key.

[signedav]

@gacarrillor What do you think?

[gacarrillor]

@gacarrillor What do you think?

@signedav, can you add more details, e.g., an example to have more context?

[signedav]

@gacarrillor What do you think?

@signedav, can you add more details, e.g., an example to have more context?

Updated the description.

[gacarrillor]

@signedav, I think this is welcome. Could you add tests for both GPKG and PG? If so, you could take the existing tests in test_pk.py as a basis. For each of those tests, there is an intermediate step (in APPEND mode) that you can ignore for arriving to your scenario.

[signedav]

Looking at the tests now, I am wondering if my use case could have been represented with no-action. Does no-action overwrite existing features or add new ones? I thought no-action meant that it simply adds new ones.

Anyway, this fix makes sense regardless, as you might not want to identify a feature by the PK but still want to adopt the PK during the import.

Edit: Sorry, misinterpreted...
I'll extend the tests.

[gacarrillor]

Regarding automatic PKs, are GPKG / PG smart enough to skip PK values already in use?

In the modified tests, you're adding T_Id=6.
What if a user wants to add a new feature, relying on the automatic value for the PK (e.g., in the PG test, a serial PK). Will GPKG/PG add the new feature with T_Id=7, or eventually, it'll fail due to an already used T_Id value?

[signedav]

Regarding automatic PKs, are GPKG / PG smart enough to skip PK values already in use?

In the modified tests, you're adding T_Id=6. What if a user wants to add a new feature, relying on the automatic value for the PK (e.g., in the PG test, a serial PK). Will GPKG/PG add the new feature with T_Id=7, or eventually, it'll fail due to an already used T_Id value?

Good point. I had to test it. And weirdly GPKG is smart enough but apparently PG is not. But I assumed it gets it, or how would have this worked before https://github.com/gacarrillor/AppendFeaturesToLayer/blob/master/tests/test_pk.py#L190 ? Here would the serial jump to 102?

[gacarrillor]

Good point. I had to test it. And weirdly GPKG is smart enough but apparently PG is not. But I assumed it gets it, or how would have this worked before https://github.com/gacarrillor/AppendFeaturesToLayer/blob/master/tests/test_pk.py#L190 ? Here would the serial jump to 102?

Those PK are not automatic, that's the method for testing non-serial PKs, i.e., PKs that you must always provide in the INSERT clause. BTW, there is a misleading comment in that line of the test file.

On the other hand, that confirms that this comment was consciously added, i.e., this is dangerous.

The only solution I can see is to keep the current behavior (i.e., the one before this PR) for all PK types but one: UUID, and add the new behavior only when the PK type is UUID (after all, in theory we will never get a UNIQUE constraint violation for UUIDs). If that's the case, we'd need to keep your extended tests (before your latest commit 55608ec), and add more lines for testing that we won't set non-UUID PKs on UPDATE mode.

What do you think?

[signedav]

I think you are right. But isn't it highly provider-dependent? Are there not providers (such as GeoPackage) that can use UUIDs as PKs, but these are text fields? Of course, we can also explicitly allow it in Postgres. Or we check whether the PK is not an integer and then allow it (because we can assume that it generates the PKs in some other way).

[gacarrillor]

I think you are right. But isn't it highly provider-dependent? Are there not providers (such as GeoPackage) that can use UUIDs as PKs, but these are text fields? Of course, we can also explicitly allow it in Postgres. Or we check whether the PK is not an integer and then allow it (because we can assume that it generates the PKs in some other way).

Yeah, I meant PG+UUID --> new behavior, for the rest of cases we keep the current behavior (before this PR).
I wouldn't like to assume that non-int PKs are safe here, but rather enable new behavior for PK types that we are sure that are safe (i.e., UUID (+PG)).
Does it make sense for you?

[signedav]

What do you think @gacarrillor ?

[gacarrillor]

One important question in the comments below and we are done.

We could have a new release (AFTL 3.x series) tomorrow Tuesday or the day after tomorrow.
Let me know if this is needed for QGIS 3 as well.

[signedav]

We could have a new release (AFTL 3.x series) tomorrow Tuesday or the day after tomorrow. Let me know if this is needed for QGIS 3 as well.

For this project funded this change I need QGIS 3. If this would be not possible I would need to deploy it separately, what I would like to avoid.

And in general, I think there are lots of users using QGIS 3 for another year or something. Often people start testing the LTR on patch-version 3 and roll it out on version 8 - so I think it makes sense to support QGIS 3 for some more time. I agree that they can use older versions, but for my specific case this does not work :-)

[gacarrillor]

We could have a new release (AFTL 3.x series) tomorrow Tuesday or the day after tomorrow. Let me know if this is needed for QGIS 3 as well.

For this project funded this change I need QGIS 3. If this would be not possible I would need to deploy it separately, what I would like to avoid.

And in general, I think there are lots of users using QGIS 3 for another year or something. Often people start testing the LTR on patch-version 3 and roll it out on version 8 - so I think it makes sense to support QGIS 3 for some more time. I agree that they can use older versions, but for my specific case this does not work :-)

Not sure if I follow. I'm planning to also release new AFTL 2.x versions (for QGIS 3.x) when really needed, like in this case.

To add some consistency, let's allow the new behavior for the rest of providers as well (mainly thinking about GPKG, and hoping string PKs are being always uniquely generated, e.g., as UUIDs). That would mean reverting 03c9ffa and adding a unit test for GPKG.

Can you add tests for GPKG?

[signedav]

Ah. Sorry, haven't noticed that you are shipping multiple versions. So yes, a 2.x Version is much appreciated ✌🏻

[gacarrillor]

After some internal discussion:

I'll merge this as is and take care of some polishing in a follow-up PR.
Namely, I've checked and GPKG does not support non-integer PKs (i.e., the fid should be integer). Therefore, and taking into account our previous discussion in this PR, I'll only enable the new behavior introduced by this PR for the PostgreSQL provider, and only for the UUID field type (i.e., not for strings).

I'll also enhance existing unit tests to check that we never set automatic PKs in UPDATE mode (unless the target PK field's type is UUID and the target layer is a PG one).

@signedav, thanks for your work!