Add our own DB exception abstraction #25091
Conversation
ChristophWurst
added
enhancement
3. to review
ChristophWurst
requested review from
rullzer,
nickvergessen,
MorrisJobke,
blizzz,
juliusknorr and
kesselb
| @@ -25,12 +25,15 @@ | |||
|
|
|||
| namespace OC\DB; | |||
|
|
|||
| use Doctrine\DBAL\Exception; | |||
There was a problem hiding this comment.
something complains on the imports
There was a problem hiding this comment.
there was one unused below
ChristophWurst
force-pushed
the
enhancement/ocp-db-exception-abstraction
branch
from
b1e7305
to
6e2410f
Compare
Oh yes. But to be fair not only because of this PR. It will already hit hard since the original doctrine exception vanished. So it's just a matter of which new exception to listen for. And some of the now thrown exceptions were thrown before too. They were just not documented in our abstraction, but very present in the Doctrine signature. |
|
Well all the |
|
Right. I hadn't factored that in. But then these special exceptions were never documented to be thrown … 😛 |
|
Psalm says no |
ChristophWurst
force-pushed
the
enhancement/ocp-db-exception-abstraction
branch
from
6e2410f
to
01afbba
Compare
Look again. It's unrelated. |
Right now our API exports the Doctrine/dbal exception. As we've seen with the dbal 3 upgrade, the leakage of 3rdparty types is problematic as a dependency update means lots of work in apps, due to the direct dependency of what Nextcloud ships. This breaks this dependency so that apps only need to depend on our public API. That API can then be vendor (db lib) agnostic and we can work around future deprecations/removals in dbal more easily. Right now the type of exception thrown is transported as "reason". For the more popular types of errors we can extend the new exception class and allow apps to catch specific errors only. Right now they have to catch-check-rethrow. This is not ideal, but better than the dependnecy on dbal. Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
force-pushed
the
enhancement/ocp-db-exception-abstraction
branch
from
01afbba
to
2c9cdc1
Compare
| * | ||
| * @since 21.0.0 | ||
| */ | ||
| public const REASON_CONSTRAINT_VIOLATION = 2; |
There was a problem hiding this comment.
so the plan is first to have this generic and then maybe at a later stage have constraint violation exceptions? Or just document it properly and have the callers check the reason?
There was a problem hiding this comment.
Yes and yes. The generic way is that the just catch this exception and optonally compare the reason to a constant. And later we can have specific subclasses that can be caught
ChristophWurst
removed
the
pending documentation
Right now our API exports the Doctrine/dbal exception. As we've seen
with the dbal 3 upgrade, the leakage of 3rdparty types is problematic as
a dependency update means lots of work in apps, due to the direct
dependency of what Nextcloud ships. This breaks this dependency so that
apps only need to depend on our public API. That API can then be vendor
(db lib) agnostic and we can work around future deprecations/removals in
dbal more easily.
Right now the type of exception thrown is transported as "reason". For
the more popular types of errors we can extend the new exception class
and allow apps to catch specific errors only. Right now they have to
catch-check-rethrow. This is not ideal, but better than the dependnecy
on dbal.
Follow-up to #24948