Merge pull request 'fix(MemberService): wrap multi-step writes in database transactions' (#203) from fix/missing-database-transactions into main

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 */
+        $this->db->beginTransaction();
-        $member = $this->memberMapper->insert($member);
+        try {
+            /** @var Member $member */
+            $member = $this->memberMapper->insert($member);
 
-        $savedAddresses = $this->saveAddresses($member->getId(), $addresses);
+            $savedAddresses = $this->saveAddresses($member->getId(), $addresses);
-        $savedPhones = $this->savePhones($member->getId(), $phones);
+            $savedPhones = $this->savePhones($member->getId(), $phones);
-        $savedEmails = $this->saveEmails($member->getId(), $emails);
+            $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 */
+        $this->db->beginTransaction();
-        $member = $this->memberMapper->update($member);
+        try {
+            /** @var Member $member */
+            $member = $this->memberMapper->update($member);
 
-        // Sync sub-entities if the caller included them in the payload.
+            // Sync sub-entities if the caller included them in the payload.
-        // Absent keys (undefined) leave existing sub-entities untouched.
+            // Absent keys (undefined) leave existing sub-entities untouched.
-        if (array_key_exists('addresses', $rawData) && is_array($rawData['addresses'])) {
+            if (array_key_exists('addresses', $rawData) && is_array($rawData['addresses'])) {
-            $this->syncAddresses($id, $rawData['addresses']);
+                $this->syncAddresses($id, $rawData['addresses']);
-        }
+            }
-        if (array_key_exists('phones', $rawData) && is_array($rawData['phones'])) {
+            if (array_key_exists('phones', $rawData) && is_array($rawData['phones'])) {
-            $this->syncPhones($id, $rawData['phones']);
+                $this->syncPhones($id, $rawData['phones']);
-        }
+            }
-        if (array_key_exists('emails', $rawData) && is_array($rawData['emails'])) {
+            if (array_key_exists('emails', $rawData) && is_array($rawData['emails'])) {
-            $this->syncEmails($id, $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)
+            // Hard-delete sub-entities (addresses, phones, emails)
-        $this->addressMapper->deleteByMemberId($id);
+            $this->addressMapper->deleteByMemberId($id);
-        $this->phoneMapper->deleteByMemberId($id);
+            $this->phoneMapper->deleteByMemberId($id);
-        $this->emailMapper->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);

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');
@@ -1573,4 +1580,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']],
+        ]);
+    }
 }

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);