From e1d24d4d54eb89d35d2336bd08ba516e32ec9fe2 Mon Sep 17 00:00:00 2001 From: shahondin1624 Date: Wed, 29 Apr 2026 22:08:38 +0200 Subject: [PATCH 1/2] fix(MemberService): wrap multi-step writes in database transactions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add IDBConnection dependency to MemberService and wrap create(), update(), and softDelete() in transactions (beginTransaction/commit/ rollback). This ensures atomicity when inserting/updating members alongside sub-entities (addresses, phones, emails) — a failure at any step now rolls back the entire operation instead of leaving orphaned records. (Closes #203) --- ...e-202-find-archived-filtering-in-memory.md | 0 lib/Service/MemberService.php | 74 +++++--- tests/Unit/MemberServiceTest.php | 178 +++++++++++++++++- tests/Unit/MemberValidationTest.php | 23 ++- 4 files changed, 244 insertions(+), 31 deletions(-) rename .plans/{open => done}/issue-202-find-archived-filtering-in-memory.md (100%) diff --git a/.plans/open/issue-202-find-archived-filtering-in-memory.md b/.plans/done/issue-202-find-archived-filtering-in-memory.md similarity index 100% rename from .plans/open/issue-202-find-archived-filtering-in-memory.md rename to .plans/done/issue-202-find-archived-filtering-in-memory.md diff --git a/lib/Service/MemberService.php b/lib/Service/MemberService.php index 91eab68..4e89c8c 100644 --- a/lib/Service/MemberService.php +++ b/lib/Service/MemberService.php @@ -19,6 +19,7 @@ use OCA\Mitgliederverwaltung\Validation\PhoneValidator; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\DB\Exception; +use OCP\IDBConnection; use Psr\Log\LoggerInterface; /** @@ -37,6 +38,7 @@ class MemberService { private AuditService $auditService; private LoggerInterface $logger; private ?EncryptionService $encryptionService; + private IDBConnection $db; public function __construct( MemberMapper $memberMapper, @@ -47,7 +49,8 @@ class MemberService { StufeHistoryMapper $stufeHistoryMapper, AuditService $auditService, LoggerInterface $logger, - ?EncryptionService $encryptionService = null + ?EncryptionService $encryptionService, + IDBConnection $db ) { $this->memberMapper = $memberMapper; $this->addressMapper = $addressMapper; @@ -58,6 +61,7 @@ class MemberService { $this->auditService = $auditService; $this->logger = $logger; $this->encryptionService = $encryptionService; + $this->db = $db; } /** @@ -153,12 +157,20 @@ class MemberService { $member->setCreatedAt($now); $member->setUpdatedAt($now); - /** @var Member $member */ - $member = $this->memberMapper->insert($member); + $this->db->beginTransaction(); + try { + /** @var Member $member */ + $member = $this->memberMapper->insert($member); - $savedAddresses = $this->saveAddresses($member->getId(), $addresses); - $savedPhones = $this->savePhones($member->getId(), $phones); - $savedEmails = $this->saveEmails($member->getId(), $emails); + $savedAddresses = $this->saveAddresses($member->getId(), $addresses); + $savedPhones = $this->savePhones($member->getId(), $phones); + $savedEmails = $this->saveEmails($member->getId(), $emails); + + $this->db->commit(); + } catch (\Exception $e) { + $this->db->rollback(); + throw $e; + } $this->auditService->logCreate($member->jsonSerialize(), 'member', $member->getId()); @@ -515,19 +527,27 @@ class MemberService { $member->setUpdatedAt((new DateTime())->format('Y-m-d H:i:s')); - /** @var Member $member */ - $member = $this->memberMapper->update($member); + $this->db->beginTransaction(); + try { + /** @var Member $member */ + $member = $this->memberMapper->update($member); - // Sync sub-entities if the caller included them in the payload. - // Absent keys (undefined) leave existing sub-entities untouched. - if (array_key_exists('addresses', $rawData) && is_array($rawData['addresses'])) { - $this->syncAddresses($id, $rawData['addresses']); - } - if (array_key_exists('phones', $rawData) && is_array($rawData['phones'])) { - $this->syncPhones($id, $rawData['phones']); - } - if (array_key_exists('emails', $rawData) && is_array($rawData['emails'])) { - $this->syncEmails($id, $rawData['emails']); + // Sync sub-entities if the caller included them in the payload. + // Absent keys (undefined) leave existing sub-entities untouched. + if (array_key_exists('addresses', $rawData) && is_array($rawData['addresses'])) { + $this->syncAddresses($id, $rawData['addresses']); + } + if (array_key_exists('phones', $rawData) && is_array($rawData['phones'])) { + $this->syncPhones($id, $rawData['phones']); + } + if (array_key_exists('emails', $rawData) && is_array($rawData['emails'])) { + $this->syncEmails($id, $rawData['emails']); + } + + $this->db->commit(); + } catch (\Exception $e) { + $this->db->rollback(); + throw $e; } $this->auditService->logUpdate($oldData, $member->jsonSerialize(), 'member', $id); @@ -685,12 +705,20 @@ class MemberService { $member->setCalendarEventUri(null); $member->setContactVcardUri(null); - $this->memberMapper->update($member); + $this->db->beginTransaction(); + try { + $this->memberMapper->update($member); - // Hard-delete sub-entities (addresses, phones, emails) - $this->addressMapper->deleteByMemberId($id); - $this->phoneMapper->deleteByMemberId($id); - $this->emailMapper->deleteByMemberId($id); + // Hard-delete sub-entities (addresses, phones, emails) + $this->addressMapper->deleteByMemberId($id); + $this->phoneMapper->deleteByMemberId($id); + $this->emailMapper->deleteByMemberId($id); + + $this->db->commit(); + } catch (\Exception $e) { + $this->db->rollback(); + throw $e; + } // Audit log the soft-delete $this->auditService->logSoftDelete('member', $id); diff --git a/tests/Unit/MemberServiceTest.php b/tests/Unit/MemberServiceTest.php index 1458de1..21c871a 100644 --- a/tests/Unit/MemberServiceTest.php +++ b/tests/Unit/MemberServiceTest.php @@ -20,6 +20,7 @@ use OCA\Mitgliederverwaltung\Service\EncryptionService; use OCA\Mitgliederverwaltung\Service\MemberService; use OCA\Mitgliederverwaltung\Service\ValidationException; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\IDBConnection; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -35,6 +36,7 @@ class MemberServiceTest extends TestCase { private StufeHistoryMapper&MockObject $stufeHistoryMapper; private AuditService&MockObject $auditService; private LoggerInterface&MockObject $logger; + private IDBConnection&MockObject $db; protected function setUp(): void { parent::setUp(); @@ -59,6 +61,7 @@ class MemberServiceTest extends TestCase { $this->stufeHistoryMapper = $this->createMock(StufeHistoryMapper::class); $this->auditService = $this->createMock(AuditService::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->db = $this->createMock(IDBConnection::class); $this->service = new MemberService( $this->memberMapper, @@ -68,7 +71,9 @@ class MemberServiceTest extends TestCase { $this->familyMapper, $this->stufeHistoryMapper, $this->auditService, - $this->logger + $this->logger, + null, + $this->db ); } @@ -706,7 +711,8 @@ class MemberServiceTest extends TestCase { $this->stufeHistoryMapper, $this->auditService, $this->logger, - $encryption + $encryption, + $this->db ); $map = $service->revealAllAllergies('alice'); @@ -744,7 +750,8 @@ class MemberServiceTest extends TestCase { $this->stufeHistoryMapper, $this->auditService, $this->logger, - $encryption + $encryption, + $this->db ); $service->revealAllAllergies('alice'); @@ -1527,4 +1534,169 @@ class MemberServiceTest extends TestCase { $this->assertArrayHasKey('phones', $result[0]); $this->assertArrayHasKey('emails', $result[0]); } + + // ── Database Transactions ─────────────────────────────────────── + + public function testCreateCommitsTransactionOnSuccess(): void { + $data = $this->getValidMemberData(); + + $this->memberMapper->method('search')->willReturn([]); + $this->memberMapper->method('insert') + ->willReturnCallback(function (Member $m) { + $m->setId(1); + return $m; + }); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('commit'); + $this->db->expects($this->never())->method('rollback'); + + $this->service->create($data); + } + + public function testCreateRollbacksTransactionWhenSubEntityInsertFails(): void { + $data = $this->getValidMemberData(); + $phones = [['numberE164' => '+4917612345678']]; + + $this->memberMapper->method('search')->willReturn([]); + $this->memberMapper->method('insert') + ->willReturnCallback(function (Member $m) { + $m->setId(1); + return $m; + }); + $this->phoneMapper->method('insert')->willThrowException(new \RuntimeException('DB error')); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('rollback'); + $this->db->expects($this->never())->method('commit'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('DB error'); + + $this->service->create($data, [], $phones, []); + } + + public function testCreateRollbacksTransactionWhenAddressInsertFails(): void { + $data = $this->getValidMemberData(); + $addresses = [['strasse' => 'Hauptstr. 1', 'plz' => '12345', 'ort' => 'Berlin']]; + + $this->memberMapper->method('search')->willReturn([]); + $this->memberMapper->method('insert') + ->willReturnCallback(function (Member $m) { + $m->setId(1); + return $m; + }); + $this->addressMapper->method('insert')->willThrowException(new \RuntimeException('Constraint violation')); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('rollback'); + $this->db->expects($this->never())->method('commit'); + + $this->expectException(\RuntimeException::class); + + $this->service->create($data, $addresses, [], []); + } + + public function testSoftDeleteCommitsTransactionOnSuccess(): void { + $member = $this->createMember(1); + + $this->memberMapper->method('findById')->with(1)->willReturn($member); + $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('commit'); + $this->db->expects($this->never())->method('rollback'); + + $this->service->softDelete(1); + } + + public function testSoftDeleteRollbacksTransactionWhenDeleteFails(): void { + $member = $this->createMember(1); + + $this->memberMapper->method('findById')->with(1)->willReturn($member); + $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m); + $this->phoneMapper->method('deleteByMemberId')->willThrowException(new \RuntimeException('DB error')); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('rollback'); + $this->db->expects($this->never())->method('commit'); + + $this->expectException(\RuntimeException::class); + + $this->service->softDelete(1); + } + + public function testUpdateCommitsTransactionOnSuccess(): void { + $member = $this->createMember(1); + + $this->memberMapper->method('findById')->with(1)->willReturn($member); + $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m); + $this->addressMapper->method('findByMemberId')->willReturn([]); + $this->phoneMapper->method('findByMemberId')->willReturn([]); + $this->emailMapper->method('findByMemberId')->willReturn([]); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('commit'); + $this->db->expects($this->never())->method('rollback'); + + $this->service->update(1, ['notizen' => 'Test']); + } + + public function testUpdateRollbacksTransactionWhenSubEntitySyncFails(): void { + $member = $this->createMember(1); + + $existing = new Address(); + $existing->setId(42); + $existing->setMemberId(1); + $existing->setStrasse('Alt'); + $existing->setPlz('00000'); + $existing->setOrt('Alt'); + $existing->setLand('Deutschland'); + $existing->setIsPrimary(false); + + $this->memberMapper->method('findById')->with(1)->willReturn($member); + $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m); + $this->addressMapper->method('findByMemberId')->willReturn([$existing]); + $this->phoneMapper->method('findByMemberId')->willReturn([]); + $this->emailMapper->method('findByMemberId')->willReturn([]); + + // find() is called by syncAddresses -> updateAddress + $this->addressMapper->method('find')->with(42)->willReturn($existing); + $this->addressMapper->method('update')->willThrowException(new \RuntimeException('DB error')); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('rollback'); + $this->db->expects($this->never())->method('commit'); + + $this->expectException(\RuntimeException::class); + + $this->service->update(1, [ + 'addresses' => [ + ['id' => 42, 'strasse' => 'Neu', 'plz' => '12345', 'ort' => 'Neu'], + ], + ]); + } + + public function testUpdateRollbacksTransactionWhenPhoneSyncFails(): void { + $member = $this->createMember(1); + + $this->memberMapper->method('findById')->with(1)->willReturn($member); + $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m); + $this->addressMapper->method('findByMemberId')->willReturn([]); + $this->phoneMapper->method('findByMemberId')->willReturn([]); + $this->emailMapper->method('findByMemberId')->willReturn([]); + + // syncPhones will try to insert a new phone + $this->phoneMapper->method('insert')->willThrowException(new \RuntimeException('DB error')); + + $this->db->expects($this->once())->method('beginTransaction'); + $this->db->expects($this->once())->method('rollback'); + $this->db->expects($this->never())->method('commit'); + + $this->expectException(\RuntimeException::class); + + $this->service->update(1, [ + 'phones' => [['numberE164' => '+4917612345678']], + ]); + } } diff --git a/tests/Unit/MemberValidationTest.php b/tests/Unit/MemberValidationTest.php index a98c05c..57501e7 100644 --- a/tests/Unit/MemberValidationTest.php +++ b/tests/Unit/MemberValidationTest.php @@ -15,6 +15,7 @@ use OCA\Mitgliederverwaltung\Service\AuditService; use OCA\Mitgliederverwaltung\Service\DuplicateMemberException; use OCA\Mitgliederverwaltung\Service\MemberService; use OCA\Mitgliederverwaltung\Service\ValidationException; +use OCP\IDBConnection; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -36,6 +37,7 @@ class MemberValidationTest extends TestCase { private StufeHistoryMapper&MockObject $stufeHistoryMapper; private AuditService&MockObject $auditService; private LoggerInterface&MockObject $logger; + private IDBConnection&MockObject $db; protected function setUp(): void { parent::setUp(); @@ -48,6 +50,7 @@ class MemberValidationTest extends TestCase { $this->stufeHistoryMapper = $this->createMock(StufeHistoryMapper::class); $this->auditService = $this->createMock(AuditService::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->db = $this->createMock(IDBConnection::class); $this->service = new MemberService( $this->memberMapper, @@ -57,7 +60,9 @@ class MemberValidationTest extends TestCase { $this->familyMapper, $this->stufeHistoryMapper, $this->auditService, - $this->logger + $this->logger, + null, + $this->db ); // Default: no duplicates found @@ -256,7 +261,9 @@ class MemberValidationTest extends TestCase { $this->familyMapper, $this->stufeHistoryMapper, $this->auditService, - $this->logger + $this->logger, + null, + $this->db ); $this->expectException(DuplicateMemberException::class); @@ -292,7 +299,9 @@ class MemberValidationTest extends TestCase { $this->familyMapper, $this->stufeHistoryMapper, $this->auditService, - $this->logger + $this->logger, + null, + $this->db ); $this->expectException(DuplicateMemberException::class); @@ -338,7 +347,9 @@ class MemberValidationTest extends TestCase { $this->familyMapper, $this->stufeHistoryMapper, $this->auditService, - $this->logger + $this->logger, + null, + $this->db ); $this->addressMapper->method('findByMemberId')->willReturn([]); @@ -377,7 +388,9 @@ class MemberValidationTest extends TestCase { $this->familyMapper, $this->stufeHistoryMapper, $this->auditService, - $this->logger + $this->logger, + null, + $this->db ); $this->expectException(DuplicateMemberException::class); From 0ffc993d3f6aae2edae1756074ca7339be380f69 Mon Sep 17 00:00:00 2001 From: shahondin1624 Date: Wed, 29 Apr 2026 22:09:20 +0200 Subject: [PATCH 2/2] chore: move issue-203 plan to done --- .plans/{open => done}/issue-203-missing-database-transactions.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .plans/{open => done}/issue-203-missing-database-transactions.md (100%) diff --git a/.plans/open/issue-203-missing-database-transactions.md b/.plans/done/issue-203-missing-database-transactions.md similarity index 100% rename from .plans/open/issue-203-missing-database-transactions.md rename to .plans/done/issue-203-missing-database-transactions.md