Better handling of errors.

app/Api/V1/Controllers/TransactionController.php

@@ -29,6 +29,7 @@ use FireflyIII\Api\V1\Requests\TransactionUpdateRequest;
 use FireflyIII\Events\StoredTransactionGroup;
 use FireflyIII\Events\UpdatedTransactionGroup;
 use FireflyIII\Exceptions\DuplicateTransactionException;
+use FireflyIII\Exceptions\FireflyException;
 use FireflyIII\Helpers\Collector\GroupCollectorInterface;
 use FireflyIII\Models\TransactionGroup;
 use FireflyIII\Models\TransactionJournal;
@@ -294,6 +295,19 @@ class TransactionController extends Controller
                 ],
             ];
 
+            return response()->json($response, 422);
+        } catch(FireflyException $e) {
+            Log::warning('Caught an exception. Return error message.');
+            Log::error($e->getMessage());
+            // return bad validation message.
+            // TODO use Laravel's internal validation thing to do this.
+            $response = [
+                'message' => 'The given data was invalid.',
+                'errors'  => [
+                    'transactions.0.description' => [sprintf('Internal exception: %s',$e->getMessage())],
+                ],
+            ];
+
             return response()->json($response, 422);
         }
         app('preferences')->mark();

app/Factory/TransactionFactory.php

@@ -25,6 +25,7 @@ declare(strict_types=1);
 namespace FireflyIII\Factory;
 
 
+use FireflyIII\Exceptions\FireflyException;
 use FireflyIII\Models\Account;
 use FireflyIII\Models\Transaction;
 use FireflyIII\Models\TransactionCurrency;
@@ -104,9 +105,11 @@ class TransactionFactory
      *
      * @param string      $amount
      * @param string|null $foreignAmount
-     * @return Transaction|null
+     *
+     * @return Transaction
+     * @throws FireflyException
      */
-    public function createNegative(string $amount, ?string $foreignAmount): ?Transaction
+    public function createNegative(string $amount, ?string $foreignAmount): Transaction
     {
         if ('' === $foreignAmount) {
             $foreignAmount = null;
@@ -123,9 +126,11 @@ class TransactionFactory
      *
      * @param string      $amount
      * @param string|null $foreignAmount
-     * @return Transaction|null
+     *
+     * @return Transaction
+     * @throws FireflyException
      */
-    public function createPositive(string $amount, ?string $foreignAmount): ?Transaction
+    public function createPositive(string $amount, ?string $foreignAmount): Transaction
     {
         if ('' === $foreignAmount) {
             $foreignAmount = null;
@@ -133,7 +138,6 @@ class TransactionFactory
         if (null !== $foreignAmount) {
             $foreignAmount = app('steam')->positive($foreignAmount);
         }
-
         return $this->create(app('steam')->positive($amount), $foreignAmount);
     }
 
@@ -159,9 +163,11 @@ class TransactionFactory
     /**
      * @param string      $amount
      * @param string|null $foreignAmount
-     * @return Transaction|null
+     *
+     * @return Transaction
+     * @throws FireflyException
      */
-    private function create(string $amount, ?string $foreignAmount): ?Transaction
+    private function create(string $amount, ?string $foreignAmount): Transaction
     {
         $result = null;
         if ('' === $foreignAmount) {
@@ -183,6 +189,12 @@ class TransactionFactory
             // @codeCoverageIgnoreStart
         } catch (QueryException $e) {
             Log::error(sprintf('Could not create transaction: %s', $e->getMessage()), $data);
+            Log::error($e->getMessage());
+            Log::error($e->getTraceAsString());
+            throw new FireflyException('Query exception when creating transaction.');
+        }
+        if (null === $result) {
+            throw new FireflyException('Transaction is NULL.');
         }
         // @codeCoverageIgnoreEnd
         if (null !== $result) {

app/Factory/TransactionGroupFactory.php

@@ -24,6 +24,7 @@ declare(strict_types=1);
 namespace FireflyIII\Factory;
 
 use FireflyIII\Exceptions\DuplicateTransactionException;
+use FireflyIII\Exceptions\FireflyException;
 use FireflyIII\Models\TransactionGroup;
 use FireflyIII\User;
 use Log;
@@ -60,7 +61,6 @@ class TransactionGroupFactory
     {
         $this->journalFactory->setUser($this->user);
         $this->journalFactory->setErrorOnHash($data['error_if_duplicate_hash'] ?? false);
-
         try {
             $collection = $this->journalFactory->create($data);
         } catch(DuplicateTransactionException $e) {

app/Factory/TransactionJournalFactory.php

@@ -29,6 +29,7 @@ use Exception;
 use FireflyIII\Exceptions\DuplicateTransactionException;
 use FireflyIII\Exceptions\FireflyException;
 use FireflyIII\Models\Account;
+use FireflyIII\Models\Transaction;
 use FireflyIII\Models\TransactionCurrency;
 use FireflyIII\Models\TransactionJournal;
 use FireflyIII\Models\TransactionJournalMeta;
@@ -40,6 +41,7 @@ use FireflyIII\Repositories\Category\CategoryRepositoryInterface;
 use FireflyIII\Repositories\Currency\CurrencyRepositoryInterface;
 use FireflyIII\Repositories\PiggyBank\PiggyBankRepositoryInterface;
 use FireflyIII\Repositories\TransactionType\TransactionTypeRepositoryInterface;
+use FireflyIII\Services\Internal\Destroy\JournalDestroyService;
 use FireflyIII\Services\Internal\Support\JournalServiceTrait;
 use FireflyIII\Support\NullArrayObject;
 use FireflyIII\User;
@@ -125,6 +127,7 @@ class TransactionJournalFactory
      *
      * @return Collection
      * @throws DuplicateTransactionException
+     * @throws FireflyException
      */
     public function create(array $data): Collection
     {
@@ -139,18 +142,11 @@ class TransactionJournalFactory
 
             return new Collection;
         }
-
+        try {
             /** @var array $row */
             foreach ($transactions as $index => $row) {
                 Log::debug(sprintf('Now creating journal %d/%d', $index + 1, count($transactions)));
-
-            Log::debug('Going to call createJournal', $row);
-            try {
                 $journal = $this->createJournal(new NullArrayObject($row));
-            } catch (DuplicateTransactionException|Exception $e) {
-                Log::warning('TransactionJournalFactory::create() caught a duplicate journal in createJournal()');
-                throw new DuplicateTransactionException($e->getMessage());
-            }
                 if (null !== $journal) {
                     $collection->push($journal);
                 }
@@ -158,6 +154,19 @@ class TransactionJournalFactory
                     Log::error('The createJournal() method returned NULL. This may indicate an error.');
                 }
             }
+        } catch (DuplicateTransactionException $e) {
+            Log::warning('TransactionJournalFactory::create() caught a duplicate journal in createJournal()');
+            Log::error($e->getMessage());
+            Log::error($e->getTraceAsString());
+            $this->forceDeleteOnError($collection);
+            throw new DuplicateTransactionException($e->getMessage());
+        } catch (FireflyException $e) {
+            Log::warning('TransactionJournalFactory::create() caught an exception.');
+            Log::error($e->getMessage());
+            Log::error($e->getTraceAsString());
+            $this->forceDeleteOnError($collection);
+            throw new FireflyException($e->getMessage());
+        }
 
         return $collection;
     }
@@ -204,7 +213,7 @@ class TransactionJournalFactory
      * @param NullArrayObject $row
      *
      * @return TransactionJournal|null
-     * @throws Exception
+     * @throws FireflyException
      * @throws DuplicateTransactionException
      */
     private function createJournal(NullArrayObject $row): ?TransactionJournal
@@ -232,8 +241,13 @@ class TransactionJournalFactory
         try {
             // validate source and destination using a new Validator.
             $this->validateAccounts($row);
-            /** create or get source and destination accounts  */
+        } catch (FireflyException $e) {
+            Log::error('Could not validate source or destination.');
+            Log::error($e->getMessage());
 
+            return null;
+        }
+        /** create or get source and destination accounts  */
         $sourceInfo = [
             'id'     => (int)$row['source_id'],
             'name'   => $row['source_name'],
@@ -254,14 +268,6 @@ class TransactionJournalFactory
 
         $sourceAccount      = $this->getAccount($type->type, 'source', $sourceInfo);
         $destinationAccount = $this->getAccount($type->type, 'destination', $destInfo);
-            // @codeCoverageIgnoreStart
-        } catch (FireflyException $e) {
-            Log::error('Could not validate source or destination.');
-            Log::error($e->getMessage());
-
-            return null;
-        }
-        // @codeCoverageIgnoreEnd
 
         // TODO AFTER 4.8,0 better handling below:
 
@@ -337,7 +343,15 @@ class TransactionJournalFactory
         $transactionFactory->setCurrency($sourceCurrency);
         $transactionFactory->setForeignCurrency($sourceForeignCurrency);
         $transactionFactory->setReconciled($row['reconciled'] ?? false);
-        $transactionFactory->createNegative((string)$row['amount'], (string)$row['foreign_amount']);
+        try {
+            $negative = $transactionFactory->createNegative((string)$row['amount'], (string)$row['foreign_amount']);
+        } catch (FireflyException $e) {
+            Log::error('Exception creating negative transaction.');
+            Log::error($e->getMessage());
+            Log::error($e->getTraceAsString());
+            $this->forceDeleteOnError(new Collection([$journal]));
+            throw new FireflyException($e->getMessage());
+        }
 
         // and the destination one:
         /** @var TransactionFactory $transactionFactory */
@@ -348,7 +362,18 @@ class TransactionJournalFactory
         $transactionFactory->setCurrency($destCurrency);
         $transactionFactory->setForeignCurrency($destForeignCurrency);
         $transactionFactory->setReconciled($row['reconciled'] ?? false);
+        try {
             $transactionFactory->createPositive((string)$row['amount'], (string)$row['foreign_amount']);
+        } catch (FireflyException $e) {
+            Log::error('Exception creating positive transaction.');
+            Log::error($e->getMessage());
+            Log::error($e->getTraceAsString());
+            Log::warning('Delete negative transaction.');
+            $this->forceTrDelete($negative);
+            $this->forceDeleteOnError(new Collection([$journal]));
+            throw new FireflyException($e->getMessage());
+        }
+
 
         // verify that journal has two transactions. Otherwise, delete and cancel.
         // TODO this can't be faked so it can't be tested.
@@ -418,6 +443,37 @@ class TransactionJournalFactory
         }
     }
 
+    /**
+     * Force the deletion of an entire set of transaction journals and their meta object in case of
+     * an error creating a group.
+     *
+     * @param Collection $collection
+     */
+    private function forceDeleteOnError(Collection $collection): void
+    {
+        Log::debug(sprintf('forceDeleteOnError on collection size %d item(s)', $collection->count()));
+        $service = app(JournalDestroyService::class);
+        /** @var TransactionJournal $journal */
+        foreach ($collection as $journal) {
+            Log::debug(sprintf('forceDeleteOnError on journal #%d', $journal->id));
+            $service->destroy($journal);
+        }
+    }
+
+    /**
+     * @param Transaction $transaction
+     */
+    private function forceTrDelete(Transaction $transaction): void
+    {
+        try {
+            $transaction->delete();
+        } catch (Exception $e) {
+            Log::error($e->getMessage());
+            Log::error($e->getTraceAsString());
+            Log::error('Could not delete negative transaction.');
+        }
+    }
+
     /**
      * @param TransactionCurrency|null $currency
      * @param Account                  $account

app/Repositories/TransactionGroup/TransactionGroupRepository.php

@@ -332,6 +332,7 @@ class TransactionGroupRepository implements TransactionGroupRepositoryInterface
      *
      * @return TransactionGroup
      * @throws DuplicateTransactionException
+     * @throws FireflyException
      */
     public function store(array $data): TransactionGroup
     {
@@ -343,6 +344,10 @@ class TransactionGroupRepository implements TransactionGroupRepositoryInterface
         } catch (DuplicateTransactionException $e) {
             Log::warning('Group repository caught group factory with a duplicate exception!');
             throw new DuplicateTransactionException($e->getMessage());
+        } catch(FireflyException $e) {
+            Log::error($e->getMessage());
+            Log::error($e->getTraceAsString());
+            throw new FireflyException($e->getMessage());
         }