Throw our own exception in \OCP\DB\QueryBuilder\IQueryBuilder::execute

[ChristophWurst]

Just as documented. This seems to be a leftover form the dbal changes in
21. We noticed that execute still throws the raw dbal exception and
catches for the documented execption never trigger.

Follow-up to #24948

[ChristophWurst]

/backport to stable21

[ChristophWurst]

This change actually has a lot of potential to break things 😬

[MorrisJobke]

CI says "no" :/

[rullzer]

execute is depratecated anyway. I vote to not break stuff. And just leave it as it. Then when we move stuff over it will just get cleaner over time

[ChristophWurst]

The exception is documented to be thrown in our upgrade docs and in the method docblock. This is confusing for devs and static analysis. Shall we fix the docblock instead? Both are breaking fixes.

[rullzer]

The exception is documented to be thrown in our upgrade docs and in the method docblock. This is confusing for devs and static analysis. Shall we fix the docblock instead? Both are breaking fixes.

Ah ok I missed that part.
But this was only for 21 right?
I guess fixing the doc block is better here.

[nickvergessen]

I vote for doing it with our Exception. We promoted it in the upgrade docs and people released all their apps with it I guess?

[ChristophWurst]

I vote for doing it with our Exception. We promoted it in the upgrade docs and people released all their apps with it I guess?

It will depend on whether people adjusted their code to what the method docs say or what exceptions they found during debugging.

Both changing the thrown exception and changing the documented exception has potential for an ugly breaking change, but I'd rather break towards a consistent API.

[ChristophWurst]

In any case for master we should definitely do this. Only the backport to 21 is up for debate :)

[MorrisJobke]

CI says "no" :/

That hasn't changed yet 😢

[rullzer]

ping

[ChristophWurst]

All code using execute but catching stuff like UniqueConstraintViolationException fails 🤡

[nickvergessen]

So, 🤡 idea:
only fix the exception on executeStatement and executeQuery. They are new in 22 and execute is deprecated and still throws the old exceptions?

[ChristophWurst]

I like your thinking.

[ChristophWurst]

only fix the exception on executeStatement and executeQuery. They are new in 22 and execute is deprecated and still throws the old exceptions?

That is actually our current situation.

[nickvergessen]

Yeah but the methods are newer than this PR? So adjust the docs of execute() instead?
Or we make a new class which inherits from both :-X so SorryUniqueConstraintViolationException is a UniqueConstraintViolationException aswell as a DoctrineException

[ChristophWurst]

Or we make a new class which inherits from both :-X so SorryUniqueConstraintViolationException is a UniqueConstraintViolationException aswell as a DoctrineException

Then I need this for all the ~15 subtypes of doctrine's exception. This isn't practical at all.

[nickvergessen]

So adjust the docs of execute() instead?

So lets adjust the docs on the deprecated method to the reality and the new code is good going forward?

[ChristophWurst]

Let's leave this deprecated method as is.