From d4296e7de099399c307396e959a8235b50133ada Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 2 Nov 2026 19:10:09 +0100 Subject: [PATCH] refactor(federation): pass the peer to the merge plan as one array FederationMerger::plan took the peer's url, organisation and sync moment as three of its five parameters. They now travel as one peer array; the reconciliation itself is unchanged. FederationService and the merger test pass the same values in the new shape, and the pull test now also checks the organisation and sync moment on the stored mirror. Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Service/Federation/FederationMerger.php | 20 ++++---- lib/Service/Federation/FederationService.php | 13 +++-- tests/Unit/Service/FederationMergerTest.php | 54 +++++++++----------- tests/Unit/Service/FederationServiceTest.php | 5 ++ 4 files changed, 43 insertions(+), 49 deletions(-) diff --git a/lib/Service/Federation/FederationMerger.php b/lib/Service/Federation/FederationMerger.php index 470fc7b57..560d344e5 100644 --- a/lib/Service/Federation/FederationMerger.php +++ b/lib/Service/Federation/FederationMerger.php @@ -55,13 +55,14 @@ class FederationMerger { * locally-mirrored entries no longer in the peer catalog are WITHDRAWs * (marked withdrawn + stale, never silently deleted). * - * @param string $peerUrl The peer base URL (provenance instance). - * @param string $peerOrg The peer organisation name (provenance). + * @param array{url: string, organisation: string, syncedAt: string} $peer The peer: its base URL + * (provenance instance), its + * organisation name (provenance) + * and the ISO-8601 sync moment. * @param array> $peerEntries The peer's published entries (each MUST * carry a stable id under 'id' or * 'uuid'). * @param array> $localMirrors The locally-stored mirrors of THIS peer. - * @param string $syncedAt The ISO-8601 sync moment. * * The reconciliation plan. Each create/update item is a full mirror data bag * (with `_source`); each withdraw item is the existing local mirror data bag, @@ -83,14 +84,11 @@ class FederationMerger { * per-entry guards, each of which exists to keep the plan idempotent and to keep * locally-owned entries out of it. */ - public function plan( - string $peerUrl, - string $peerOrg, - array $peerEntries, - array $localMirrors, - string $syncedAt, - ): array { - $create = []; + public function plan(array $peer, array $peerEntries, array $localMirrors): array { + $peerUrl = $peer['url']; + $peerOrg = $peer['organisation']; + $syncedAt = $peer['syncedAt']; + $create = []; $update = []; $withdraw = []; diff --git a/lib/Service/Federation/FederationService.php b/lib/Service/Federation/FederationService.php index 3bb5174b7..cd49fea85 100644 --- a/lib/Service/Federation/FederationService.php +++ b/lib/Service/Federation/FederationService.php @@ -373,13 +373,12 @@ public function pullPeer(string $peerUrl): array { } $localMirrors = $this->loadPeerMirrors(peerUrl: $peerUrl, target: $target); - $plan = $this->merger->plan( - $peerUrl, - (string)($fetch['organisation'] ?? $peerUrl), - $fetch['entries'], - $localMirrors, - $this->now() - ); + $peer = [ + 'url' => $peerUrl, + 'organisation' => (string)($fetch['organisation'] ?? $peerUrl), + 'syncedAt' => $this->now(), + ]; + $plan = $this->merger->plan(peer: $peer, peerEntries: $fetch['entries'], localMirrors: $localMirrors); $created = $this->applyMirrors( objectService: $objectService, diff --git a/tests/Unit/Service/FederationMergerTest.php b/tests/Unit/Service/FederationMergerTest.php index 6628e904c..4e717d0d7 100644 --- a/tests/Unit/Service/FederationMergerTest.php +++ b/tests/Unit/Service/FederationMergerTest.php @@ -56,11 +56,9 @@ protected function setUp(): void { */ public function testNewPeerEntryIsCreatedWithProvenance(): void { $plan = $this->merger->plan( - self::PEER, - self::ORG, - [['id' => 'p-1', 'name' => 'Service A']], - [], - self::NOW + peer: ['url' => self::PEER, 'organisation' => self::ORG, 'syncedAt' => self::NOW], + peerEntries: [['id' => 'p-1', 'name' => 'Service A']], + localMirrors: [] ); $this->assertCount(1, $plan['create']); @@ -86,11 +84,9 @@ public function testNewPeerEntryIsCreatedWithProvenance(): void { */ public function testInboundSourceCannotBeSpoofed(): void { $plan = $this->merger->plan( - self::PEER, - self::ORG, - [['id' => 'p-1', 'name' => 'X', '_source' => ['instance' => 'https://evil.example']]], - [], - self::NOW + peer: ['url' => self::PEER, 'organisation' => self::ORG, 'syncedAt' => self::NOW], + peerEntries: [['id' => 'p-1', 'name' => 'X', '_source' => ['instance' => 'https://evil.example']]], + localMirrors: [] ); $this->assertSame(self::PEER, $plan['create'][0]['_source']['instance']); @@ -107,11 +103,9 @@ public function testChangedPeerEntryIsUpdated(): void { ]; $plan = $this->merger->plan( - self::PEER, - self::ORG, - [['id' => 'p-1', 'name' => 'New name']], - $local, - self::NOW + peer: ['url' => self::PEER, 'organisation' => self::ORG, 'syncedAt' => self::NOW], + peerEntries: [['id' => 'p-1', 'name' => 'New name']], + localMirrors: $local ); $this->assertCount(0, $plan['create']); @@ -132,11 +126,9 @@ public function testUnchangedRepullIsIdempotent(): void { ]; $plan = $this->merger->plan( - self::PEER, - self::ORG, - [['id' => 'p-1', 'name' => 'Same']], - $local, - '2026-06-15T13:00:00+00:00' + peer: ['url' => self::PEER, 'organisation' => self::ORG, 'syncedAt' => '2026-06-15T13:00:00+00:00'], + peerEntries: [['id' => 'p-1', 'name' => 'Same']], + localMirrors: $local ); $this->assertSame([], $plan['create']); @@ -156,11 +148,9 @@ public function testRemovedPeerEntryIsWithdrawn(): void { ]; $plan = $this->merger->plan( - self::PEER, - self::ORG, - [['id' => 'p-2', 'name' => 'Stays']], - $local, - self::NOW + peer: ['url' => self::PEER, 'organisation' => self::ORG, 'syncedAt' => self::NOW], + peerEntries: [['id' => 'p-2', 'name' => 'Stays']], + localMirrors: $local ); $this->assertCount(0, $plan['create']); @@ -187,7 +177,11 @@ public function testOtherPeersMirrorsAreUntouched(): void { ], ]; - $plan = $this->merger->plan(self::PEER, self::ORG, [], $local, self::NOW); + $plan = $this->merger->plan( + peer: ['url' => self::PEER, 'organisation' => self::ORG, 'syncedAt' => self::NOW], + peerEntries: [], + localMirrors: $local + ); // The other peer's mirror is NOT withdrawn by THIS peer's empty pull. $this->assertSame([], $plan['withdraw']); @@ -202,11 +196,9 @@ public function testOtherPeersMirrorsAreUntouched(): void { */ public function testEntryWithoutStableIdIsSkipped(): void { $plan = $this->merger->plan( - self::PEER, - self::ORG, - [['name' => 'No id']], - [], - self::NOW + peer: ['url' => self::PEER, 'organisation' => self::ORG, 'syncedAt' => self::NOW], + peerEntries: [['name' => 'No id']], + localMirrors: [] ); $this->assertSame([], $plan['create']); diff --git a/tests/Unit/Service/FederationServiceTest.php b/tests/Unit/Service/FederationServiceTest.php index 5a40ff889..afa4bdf8e 100644 --- a/tests/Unit/Service/FederationServiceTest.php +++ b/tests/Unit/Service/FederationServiceTest.php @@ -358,6 +358,11 @@ function (array $object) use (&$saved) { $this->assertNotEmpty($saved); $mirror = $saved[0]; $this->assertSame('https://peer.example', $mirror['_source']['instance']); + $this->assertSame(expected: 'Gemeente Peer', actual: $mirror['_source']['organisation']); + $this->assertMatchesRegularExpression( + pattern: '/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\+00:00$/', + string: $mirror['_source']['syncedAt'] + ); $this->assertTrue($service->isPeerSourced($mirror)); }//end testPullPeerMergesNewEntries()