Add new `\Civi\Exception\DBQueryException` & throw that rather than a `PEAR_Exception` #25634

eileenmcnaughton · 2023-02-21T02:18:40Z

Overview

Add new \Civi\Core\Exception\DBQueryException & throw that rather than a PEAR_Exception

Before

When a DB error occurs an instance of PEAR_Exception is thrown. CRM_Core_Exception extends PEAR_Exception and is the target of most try-catch blocks but does NOT catch the PEAR_Exception

After

\Civi\Core\Exception\DBQueryException is thrown for DB exceptions, otherwise CRM_Core_Exceptions are thrown

Technical Details

Comments

civibot · 2023-02-21T02:18:43Z

(Standard links)

If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

totten · 2023-02-21T02:38:14Z

With regard to naming, here's a list of existing exceptions in civicrm-core:

CRM/Utils/Cache/CacheException.php
CRM/Utils/Cache/InvalidArgumentException.php
CRM/Extension/Exception/ParseException.php
CRM/Extension/Exception/MissingException.php
CRM/Extension/Exception/DependencyException.php
CRM/Extension/Exception.php
CRM/Core/Exception/ResourceConflictException.php
CRM/Core/Exception/PrematureExitException.php
CRM/Core/Exception.php
CRM/Contribute/Exception/PastContributionPageException.php
CRM/Contribute/Exception/InactiveContributionPageException.php
CRM/Contribute/Exception/FutureContributionPageException.php
CRM/Contribute/Exception/CheckLineItemsException.php
CRM/Contact/Page/DedupeException.php
CRM/Dedupe/DAO/DedupeException.php
CRM/Dedupe/DAO/Exception.php
CRM/Dedupe/BAO/DedupeException.php
CRM/Dedupe/BAO/Exception.php

Civi/Token/TokenException.php
Civi/Api4/DedupeException.php
Civi/Core/Exception/UnknownAssetException.php
Civi/WorkflowMessage/Exception/WorkflowMessageException.php
Civi/Crypto/Exception/CryptoException.php
Civi/Payment/Exception/PaymentProcessorException.php
Civi/API/Exception/UnauthorizedException.php
Civi/API/Exception/NotImplementedException.php
Civi/CCase/Exception/MultipleActivityException.php
Civi/Pipe/JsonRpcMethodException.php

Within Civi namespace, I think that:

✔️ Exception classes are always FooBarException
⚠️ There is always some kind of Civi\Xxx (where Xxx is a subsystem - like Api4 or Token or Core)
🤷 It's quite mixed about whether to have a subordinate namespace just for exceptions ( Civi\Xxx\Exception\)

We often look to Drupal conventions. They also use FooBarException (good). They don't explicitly mention namespacing guidance, but the live examples go fairly deep (e.g. Drupal\Core\Entity\Exception\AmbiguousBundleClassException).

totten · 2023-02-21T02:38:43Z

So I would probably suggest Civi\Core\Exception\DBQueryException as being more consistent.

eileenmcnaughton · 2023-02-21T03:23:28Z

It looks like Civi/Api4/DedupeException.php is actually an Entity API and Civi/Token/TokenException.php is never used so JsonRpcMethodException is the only in-use Exception class in Civi without the Exception folder

In the CRM folder the only ones are

CRM/Utils/Cache/CacheException.php
CRM/Utils/Cache/InvalidArgumentException.php

The others are not actually exceptions

totten · 2023-02-21T03:47:29Z

ext/civiimport/Civi/BAO/Import.php

+    }
+    catch (\PEAR_Exception $e) {
+      throw new CRM_Core_Exception('Import table no longer exists');
+    }


Is this part still necessary, if the CRM_Core_Error::exceptionHandler() now emits CRM_Core_Exception?

@totten - no it is part of one of the PRs to be rebased out once merged first

(merged & up-merged to master)

Ooh, I see. You're aiming for that variant to go into 5.59 - and then this variant goes into master (but it needs to be reconciled with whatever happened in 5.59).

I just rebased out this patch cos I'm trying to deal with another issue in the same file & it will conflict

totten · 2023-02-21T04:02:34Z

CRM/Core/DAO.php

@@ -1644,6 +1644,8 @@ public static function executeUnbufferedQuery(
   *   object that holds the results of the query
   *   NB - if this is defined as just returning a DAO phpstorm keeps pointing
   *   out all the properties that are not part of the DAO
+   *
+   * @throws \Civi\Core\Exception\DBQueryException
   */
  public static function &executeQuery(


Just trying to think this through... I do like having that clearly documented for someone who's reading the docblock.

But... DBQueryException is a "checked" exception (insofar as it's not a \RuntimeException), and PHPStorm will nag you to catch it. The thing about that... when searching universe, there were only 2-3 small places which actually tried to catch an exception like this. But there are.. about 5,000 places in universe/ext/ that call executeQuery(). So won't the tooling expect you to put catch() or @throws on all of ~5,000 of them (plus, for @throws, the same on every downstream call)?

The function actually throws 2 different types of exceptions - it also throws an exception if the validation is invalid.

We kind of have the problem in practice anyway - as all the places that call the api or singleValueQuery are required to catch CRM_Core_Exception or document it (and if you catch or document CRM_Core_Exception that covers this.

I took a look & drupal DOES declare an exception on it's prepare so I guess drupal code has to catch those things.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21Query%21Sql%21Query.php/10

so I guess drupal code has to catch those things...

I started looking at their arrangement, and... it's a bit of a rabbit-hole. For example, here's a basic query:

\Drupal::database()->query("SELECT id, example FROM {mytable}")->fetchAll();

The type-hints have a lot of abstraction there. Both in the immediate call chain (e.g. abstract class Connection, interface StatementInterface) and in the various @throws. On Connection::query(), it @throws one of these:

class DatabaseExceptionWrapper extends \RuntimeException implements DatabaseException {

So from the callers' POV, that's an unchecked exception. But if you want to catch it, then you should probably go for DatabaseException instead of DatabaseExceptionWrapper.

But there's more! In the class you linked, the protected prepare() declares a @throws QueryException but the public execute() doesn't declare any @throws at all. So they just hide that one...

I dunno, after looking at Drupal's example, I feel more confused. 🙄

.all the places that call the api or singleValueQuery are required to catch CRM_Core_Exception or document it (and if you catch or document CRM_Core_Exception that covers this.

Very good point!

eileenmcnaughton · 2023-02-21T06:19:30Z

Hmm - what is happening with the tests - test this please (this is one of 2 prs I keep trying to get tests to run on today)

totten · 2023-02-21T08:02:10Z

This one seems to be an issue with the updated CRM_Core_ErrorTest.

This one seems to be an out-of-memory.

ok 638 - api_v3_ContributionRecurTest::testCreateContributionRecurWithToken with data set "APIv4" (4)
ok 639 - api_v3_ContributionRecurTest::testDeleteContributionRecur with data set "APIv3" (3)
ok 640 - api_v3_ContributionRecurTest::testDeleteContributionRecur with data set "APIv4" (4)
ok 641 - api_v3_ContributionRecurTest::testGetFieldsContributionRecur
ok 642 - api_v3_ContributionRecurTest::testContributionRecurCancel
ok 643 - api_v3_ContributionSoftTest::testGetContributionSoft
ok 644 - api_v3_ContributionSoftTest::testCreateEmptyParamsContributionSoft
ok 645 - api_v3_ContributionSoftTest::testCreateParamsWithoutRequiredKeysContributionSoft

Fatal error: Allowed memory size of 2147483648 bytes exhausted (tried to allocate 1071648768 bytes) in /home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php on line 86
###[ vtime ]##########################################################
## Command: phpunit-each --exclude-group ornery /home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/ /tmp/workspace/CiviCRM-Core-Matrix-PR/BKPROF/dfl/SUITES/phpunit-api3/label/bknix-tmp/junit api_v3_AllTests
## Started: Tue Feb 21 06:21:24 UTC 2023
## Finished: Tue Feb 21 06:26:48 UTC 2023
## Duration: 324

I checked the build-log on this node, and there were two cases of this -- both were on this PR (25634) when testing the api3 suite. (In the interim between runs, the other tests ran OK.) I wonder if this actually causes the test to use more memory?

totten · 2023-02-21T08:15:16Z

@eileenmcnaughton I get the same out-of-memory problem when I run phpunit-api3 with the patch locally.

totten · 2023-02-21T09:16:52Z

  public static function exceptionHandler($pearError) {
    CRM_Core_Error::debug_var('Fatal Error Details', self::getErrorDetails($pearError), TRUE, TRUE, '', PEAR_LOG_ERR);
    CRM_Core_Error::backtrace('backTrace', TRUE);
    if ($pearError instanceof DB_Error) {
      throw new DBQueryException($pearError->getMessage(), $pearError->getCode(), ['exception' => $pearError]);
    }
    throw new CRM_Core_Exception($pearError->getMessage(), $pearError->getCode(), ['exception' => $pearError]);
  }

So I checked out the patch and did some runs of phpunit-api3 with variations on this code:

Straight CRM_Core_Exception: Fail, out-of-memory
Straight PEAR_Exception: OK, no out-of-memory (I didn't wait for the entire test to run - but it got way past the point where the others failed - and then I stopped it)
Hybrid DBQueryException/CRM_Core_Exception: Fail, out-of-memory
Hybrid DBQueryException/PEAR_Exception: Fail, out-of-memory

(ED: I haven't dug any further to figure what or why of the memory usage.)

eileenmcnaughton · 2023-02-21T19:42:31Z

That sucks - the exception is in the error_data array - but as long as that is released it should be fine.

I think we need to get the preliminary PRs merged & then once that is sorted I will dig into where memory is not released

eileenmcnaughton · 2023-02-22T05:08:49Z

Well I don't quite have the fix yet but the issue is apiv3 handling of errors & specifically backtrace..

This line

civicrm-core/Civi/Test/Api3TestTrait.php

Line 86 in 92400d7

    
           $this->assertEquals(1, $apiResult['is_error'], "api call should have failed but it succeeded " . $prefix . (print_r($apiResult, TRUE)));

is having a cow - presumably the way the trace is attached to the $apiResult

because it is now funnelling into the first if not the second...

civicrm-core/Civi/API/Kernel.php

Lines 89 to 97 in ca5063d

    
           if ($e instanceof \CRM_Core_Exception) { 
        
             $err = $this->formatApiException($e, $apiRequest); 
        
           } 
        
           elseif ($e instanceof \PEAR_Exception) { 
        
             $err = $this->formatPearException($e, $apiRequest); 
        
           } 
        
           else { 
        
             $err = $this->formatException($e, $apiRequest); 
        
           }

eileenmcnaughton · 2023-02-23T01:14:54Z

Argh

CRM_Core_ErrorTest::testDBError with data set "invalid_syntax" (array('FROM civicrm_contact', 'DB Error: syntax error', -2, 'Invalid Query syntax error Yo...line 1', 1064))
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Invalid Query syntax error You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'FROM civicrm_contact' at line 1'
+'Invalid Query syntax error You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'FROM civicrm_contact' at line 1'

eileenmcnaughton · 2023-02-23T06:18:50Z

@totten it passed!

…EAR_ERROR Fix api v3 handling of exception to not include the exception (fails on print_r()) m

…rocess Show user sanitised message

eileenmcnaughton · 2023-02-26T21:00:02Z

@totten I think this is mergeable now (I thought we had merged it & got tripped up when I couldn't catch the new exception type :-))

totten · 2023-03-02T22:48:28Z

We just branched RC yesterday. I think that makes it a good time to merge this.

civibot bot added the master label Feb 21, 2023

eileenmcnaughton force-pushed the except branch 2 times, most recently from 7d2a7c4 to 253489a Compare February 21, 2023 03:35

totten reviewed Feb 21, 2023

View reviewed changes

eileenmcnaughton force-pushed the except branch from 33b096c to bfdd5ac Compare February 21, 2023 05:25

eileenmcnaughton force-pushed the except branch from bfdd5ac to 376dc75 Compare February 22, 2023 01:15

eileenmcnaughton mentioned this pull request Feb 22, 2023

Deprecate unused, exception class with non-standard name-spacing #25641

Merged

eileenmcnaughton force-pushed the except branch 2 times, most recently from c2b9a52 to 52013b5 Compare February 23, 2023 04:11

eileenmcnaughton added 2 commits February 27, 2023 09:46

Add new \Civi\Exception\DBQueryException & throw that rather than a P…

ba96490

…EAR_ERROR Fix api v3 handling of exception to not include the exception (fails on print_r()) m

Reply on improved exception handling for handling bad sql in import p…

1527308

…rocess Show user sanitised message

eileenmcnaughton force-pushed the except branch from 52013b5 to 1527308 Compare February 26, 2023 21:01

totten merged commit a653811 into civicrm:master Mar 2, 2023

seamuslee001 deleted the except branch March 2, 2023 22:48

jensschuppe mentioned this pull request Feb 13, 2024

api\v4\EckEntity\EckEntityTest::testTwoEntityTypes() fails with DB Error systopia/de.systopia.eck#115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `\Civi\Exception\DBQueryException` & throw that rather than a `PEAR_Exception` #25634

Add new `\Civi\Exception\DBQueryException` & throw that rather than a `PEAR_Exception` #25634

eileenmcnaughton commented Feb 21, 2023 •

edited

Loading

civibot bot commented Feb 21, 2023

totten commented Feb 21, 2023

totten commented Feb 21, 2023

eileenmcnaughton commented Feb 21, 2023 •

edited

Loading

totten Feb 21, 2023

eileenmcnaughton Feb 21, 2023

eileenmcnaughton Feb 21, 2023

totten Feb 21, 2023

eileenmcnaughton Feb 26, 2023

totten Feb 21, 2023

eileenmcnaughton Feb 21, 2023 •

edited

Loading

totten Feb 21, 2023

eileenmcnaughton commented Feb 21, 2023

totten commented Feb 21, 2023 •

edited

Loading

totten commented Feb 21, 2023

totten commented Feb 21, 2023 •

edited

Loading

eileenmcnaughton commented Feb 21, 2023

eileenmcnaughton commented Feb 22, 2023

eileenmcnaughton commented Feb 23, 2023

eileenmcnaughton commented Feb 23, 2023

eileenmcnaughton commented Feb 26, 2023

totten commented Mar 2, 2023

Add new \Civi\Exception\DBQueryException & throw that rather than a PEAR_Exception #25634

Add new \Civi\Exception\DBQueryException & throw that rather than a PEAR_Exception #25634

Conversation

eileenmcnaughton commented Feb 21, 2023 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Feb 21, 2023

totten commented Feb 21, 2023

totten commented Feb 21, 2023

eileenmcnaughton commented Feb 21, 2023 • edited Loading

totten Feb 21, 2023

Choose a reason for hiding this comment

eileenmcnaughton Feb 21, 2023

Choose a reason for hiding this comment

eileenmcnaughton Feb 21, 2023

Choose a reason for hiding this comment

totten Feb 21, 2023

Choose a reason for hiding this comment

eileenmcnaughton Feb 26, 2023

Choose a reason for hiding this comment

totten Feb 21, 2023

Choose a reason for hiding this comment

eileenmcnaughton Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

totten Feb 21, 2023

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 21, 2023

totten commented Feb 21, 2023 • edited Loading

totten commented Feb 21, 2023

totten commented Feb 21, 2023 • edited Loading

eileenmcnaughton commented Feb 21, 2023

eileenmcnaughton commented Feb 22, 2023

eileenmcnaughton commented Feb 23, 2023

eileenmcnaughton commented Feb 23, 2023

eileenmcnaughton commented Feb 26, 2023

totten commented Mar 2, 2023

Add new `\Civi\Exception\DBQueryException` & throw that rather than a `PEAR_Exception` #25634

Add new `\Civi\Exception\DBQueryException` & throw that rather than a `PEAR_Exception` #25634

eileenmcnaughton commented Feb 21, 2023 •

edited

Loading

eileenmcnaughton commented Feb 21, 2023 •

edited

Loading

eileenmcnaughton Feb 21, 2023 •

edited

Loading

totten commented Feb 21, 2023 •

edited

Loading

totten commented Feb 21, 2023 •

edited

Loading