Catch UniqueConstraintViolationException inside insertIfNotExist

[MorrisJobke]

This is the most common case for the usage of this method.

See also #12369 and the linked tickets.

• supersedes Bug 6160: Deadlock removed #12315, Catch and ignore UniqueConstraintViolationException in file locking init #12366 and Catch and ignore UniqueConstraintViolationException in file cache insert #12368
• fixesTrashbin throws UniqueConstraintViolationException when restoring files #135
• fixesDoctrine\DBAL\Exception\UniqueConstraintViolationException: An exception occurred while executing 'INSERT INTO "oc_filecache"  #6160
• fixesDoctrine\DBAL\Exception\UniqueConstraintViolationException: An exception occurred while executing 'UPDATE oc_filecache #6608
• fixesRace condition with locks in database - unique constraint violation on oc_file_locks #9305
• fixesDoctrine\DBAL\Exception\DriverException: An exception occurred while executing 'INSERT INTO oc_filecache #12343
• see UniqueConstraintViolationException overview ticket #12369

[MorrisJobke]

@obel1x This is basically the more general approach.

[blizzz]

code looks good and makes sense

[ChristophWurst]

Makes sense

[nickvergessen]

Well this basically "breaks" the case when you have a unique key which is not the pattern you are trying to match, but that does not sound like a valid usecase to me, so I'm fine with this.

Let's hope it finally kills these reports.

[MorrisJobke]

Well this basically "breaks" the case when you have a unique key which is not the pattern you are trying to match, but that does not sound like a valid usecase to me, so I'm fine with this.

That's also what I thought, I will check if any existing app does that, but I don't expect it.

[kesselb]

the case when you have a unique key which is not the pattern you are trying to match, but that does not sound like a valid usecase to me

😕 If this is not a valid usecase and unique key are working for all supported dbms why do you still use this "fake" atomic insert approach here?

try {
$connection->insert('blubba');
} catch(UniqueConstraintViolationException $e) {
}

Is the above not working? Do i miss something? (I have to admit, I feel a little stupid right now 🤣)

[MorrisJobke]

Is the above not working? Do i miss something? (I have to admit, I feel a little stupid right now 🤣)

I guess it's fine then. The only problem with this is that the unique index needs to be in place. WIth the other logic it's also detected if the unique index is not in place. So we maybe want to go for the "first select and if no result insert with a wrapping of the insert".

[nickvergessen]

@danielkesselberg well you can use insertIfNotExists without having a unique key on your table.

[MorrisJobke]

@danielkesselberg well you can use insertIfNotExists without having a unique key on your table.

The only problem with this approach seems to be this deadlock somehow. But I have no idea how to reliably trigger this one in a test environment.

[MorrisJobke]

Also thinking about this a bit and the better approach to this would be to deprecate this method and do this just properly in the application logic:

try {
$db->insert(...);
} catch(UniqueConstraintViolationException$e) {
// already in there
}

Then the developer does not trust us to do this correct, but is properly aware of the situation and just does it in the right way.

[MorrisJobke]

I would still go for this PR here and backport it. Then also replace the existing occurrences. I added the deprecation message as well to this PR.

[MorrisJobke]

I fixed the unit test. This is now good to go into 15. I would do this for 15 only and do separate PRs, that replace the insertIntoIfNotExist with a proper handling in the application itself, which then also could be more easily ported to older versions.