diff --git a/app/Api/V1/Controllers/TransactionController.php b/app/Api/V1/Controllers/TransactionController.php index 289cc0a39f..9e25876dcb 100644 --- a/app/Api/V1/Controllers/TransactionController.php +++ b/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(); diff --git a/app/Factory/TransactionFactory.php b/app/Factory/TransactionFactory.php index 1f70c9c69c..a601499a54 100644 --- a/app/Factory/TransactionFactory.php +++ b/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; @@ -102,11 +103,13 @@ class TransactionFactory /** * Create transaction with negative amount (for source accounts). * - * @param string $amount + * @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; @@ -121,11 +124,13 @@ class TransactionFactory /** * Create transaction with positive amount (for destination accounts). * - * @param string $amount + * @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); } @@ -157,11 +161,13 @@ class TransactionFactory } /** - * @param string $amount + * @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) { diff --git a/app/Factory/TransactionGroupFactory.php b/app/Factory/TransactionGroupFactory.php index 27e6b619dc..045cdec5e4 100644 --- a/app/Factory/TransactionGroupFactory.php +++ b/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) { diff --git a/app/Factory/TransactionJournalFactory.php b/app/Factory/TransactionJournalFactory.php index e1db66c70f..b0a855b190 100644 --- a/app/Factory/TransactionJournalFactory.php +++ b/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,24 +142,30 @@ class TransactionJournalFactory return new Collection; } - - /** @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 { + try { + /** @var array $row */ + foreach ($transactions as $index => $row) { + Log::debug(sprintf('Now creating journal %d/%d', $index + 1, count($transactions))); $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); - } - if (null === $journal) { - Log::error('The createJournal() method returned NULL. This may indicate an error.'); + if (null !== $journal) { + $collection->push($journal); + } + if (null === $journal) { + 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,36 +241,33 @@ class TransactionJournalFactory try { // validate source and destination using a new Validator. $this->validateAccounts($row); - /** create or get source and destination accounts */ - - $sourceInfo = [ - 'id' => (int)$row['source_id'], - 'name' => $row['source_name'], - 'iban' => $row['source_iban'], - 'number' => $row['source_number'], - 'bic' => $row['source_bic'], - ]; - - $destInfo = [ - 'id' => (int)$row['destination_id'], - 'name' => $row['destination_name'], - 'iban' => $row['destination_iban'], - 'number' => $row['destination_number'], - 'bic' => $row['destination_bic'], - ]; - Log::debug('Source info:', $sourceInfo); - Log::debug('Destination info:', $destInfo); - - $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 + /** create or get source and destination accounts */ + $sourceInfo = [ + 'id' => (int)$row['source_id'], + 'name' => $row['source_name'], + 'iban' => $row['source_iban'], + 'number' => $row['source_number'], + 'bic' => $row['source_bic'], + ]; + + $destInfo = [ + 'id' => (int)$row['destination_id'], + 'name' => $row['destination_name'], + 'iban' => $row['destination_iban'], + 'number' => $row['destination_number'], + 'bic' => $row['destination_bic'], + ]; + Log::debug('Source info:', $sourceInfo); + Log::debug('Destination info:', $destInfo); + + $sourceAccount = $this->getAccount($type->type, 'source', $sourceInfo); + $destinationAccount = $this->getAccount($type->type, 'destination', $destInfo); // 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); - $transactionFactory->createPositive((string)$row['amount'], (string)$row['foreign_amount']); + 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 diff --git a/app/Repositories/TransactionGroup/TransactionGroupRepository.php b/app/Repositories/TransactionGroup/TransactionGroupRepository.php index 2c000c5c66..15a47653d8 100644 --- a/app/Repositories/TransactionGroup/TransactionGroupRepository.php +++ b/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()); }