From 63e93d089a82e1b117a836387c4193835561f4c8 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 12 Aug 2026 01:41:42 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(merge):=20organisation=20merge=20re-poi?= =?UTF-8?q?nts=20nothing=20=E2=80=94=20probe=20a=20magic=20accessor=20with?= =?UTF-8?q?=20property=5Fexists?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MergeOrganisatieService::repointBySelfOrganisation() decided whether an object was owned by the source organisation with method_exists($entity, 'getOrganisation'). OpenRegister's ObjectEntity declares that accessor only as an @method docblock tag over protected ?string $organisation, so it is served by OCP\AppFramework\Db\Entity::__call() and the probe is always false. The next line skipped every object, so contract and compliancy were never re-pointed while tombstoneSource() still retired the source organisation — leaving live objects owned by an organisation that no longer exists. Dry-run and execute agreed only because both arms were equally broken. The instrument is property_exists(), which is what Entity::getter() itself decides on. is_callable() is not a membership test on a __call class — it is true for every name, so a probe swap would make the branch unconditionally true and move the failure into a runtime BadFunctionCallException. The accessor call is wrapped and the result type-checked in the same edit. The same probe in ReviewService::entityUuid() and IntakeService::entityUuid() made both return null for every real save, because saveObject() returns an object and the is_array() fallback cannot rescue it — so submit() answered uuid: null to the client and wrote uuid: null to the audit log. Why the suite was green: tests/Stubs/Db/ObjectEntity declared getOrganisation() concretely, which inverted the exact predicate under test. The merge suite now builds a faithful double — a concrete subclass of the stub, which extends the real Entity, with organisation as a property reached through __call — and one test asserts that premise so the fixture cannot drift back. The stub no longer declares getOrganisation()/setOrganisation() and carries a warning about what adding an accessor there costs. Reverting only the merge probe turns 6 tests red; reverting only the two entityUuid probes turns 2 red. Both predictions were written before the revert and matched exactly. 667 unit tests pass; phpcs, phpmd, psalm and phpstan clean. Also corrects a stale class docblock: it credited the @self.organisation write path to SaveObject::applyCallerSuppliedFields(), a method that exists nowhere in OpenRegister. The real acceptance path is SaveObject::setSelfMetadata(). Closes #490 --- lib/Service/IntakeService.php | 25 +- lib/Service/MergeOrganisatieService.php | 74 +++++- lib/Service/ReviewService.php | 25 +- tests/Stubs/Db/ObjectEntity.php | 50 +++- tests/Unit/Service/IntakeModerationTest.php | 40 +++ .../Service/MergeOrganisatieServiceTest.php | 239 +++++++++++++++++- tests/Unit/Service/ReviewServiceTest.php | 52 ++++ 7 files changed, 477 insertions(+), 28 deletions(-) diff --git a/lib/Service/IntakeService.php b/lib/Service/IntakeService.php index 775d6747..523adc1b 100644 --- a/lib/Service/IntakeService.php +++ b/lib/Service/IntakeService.php @@ -269,15 +269,34 @@ private function resolveTarget(): ?array /** * The uuid of a saved entity (handles entity or array result shapes). * + * `ObjectService::saveObject()` returns an `ObjectEntity`, whose + * `getUuid()` is an `@method` docblock served by `Entity::__call()` over + * `protected ?string $uuid`. A bare `method_exists()` probe is therefore + * FALSE, and because an object is not an array the array arm below cannot + * rescue it — so this method used to return `null` for EVERY real save, + * putting `uuid: null` in the submit response and the audit log + * (softwarecatalog#490). `property_exists()` is the instrument + * `Entity::getter()` itself decides on; `method_exists()` is kept as the + * second arm for genuinely concrete accessors, and the call is wrapped + * because neither probe guarantees the other object's shape. + * * @param mixed $entity The saveObject result. * * @return string|null The uuid, or null. */ private function entityUuid(mixed $entity): ?string { - if (is_object($entity) === true && method_exists($entity, 'getUuid') === true) { - $uuid = $entity->getUuid(); - if (is_string($uuid) === true) { + if (is_object($entity) === true + && (property_exists($entity, 'uuid') === true || method_exists($entity, 'getUuid') === true) + ) { + try { + $uuid = $entity->getUuid(); + } catch (\Throwable $e) { + $this->logger->warning('IntakeService: could not read uuid from saved entity', ['exception' => $e->getMessage()]); + return null; + } + + if (is_string($uuid) === true && $uuid !== '') { return $uuid; } diff --git a/lib/Service/MergeOrganisatieService.php b/lib/Service/MergeOrganisatieService.php index 70339dd0..e3f929b7 100644 --- a/lib/Service/MergeOrganisatieService.php +++ b/lib/Service/MergeOrganisatieService.php @@ -35,9 +35,13 @@ * field (no `$ref: organisatie` property), so ownership is carried by * OpenRegister's system-level `@self.organisation` (the same mechanism * design.md documents explicitly for compliancy). Re-pointed via - * `@self.organisation` in the save payload, matching - * `SaveObject::applyCallerSuppliedFields()`'s admin-gated - * `@self.organisation` acceptance path. + * `@self.organisation` in the save payload, matching OpenRegister's + * `SaveObject::setSelfMetadata()` acceptance path, which honours a + * caller-supplied `@self.organisation` when the caller is an admin or a + * verified member of the target organisation; a merge is admin-triggered, so + * the admin arm applies. (An earlier revision of this docblock named + * `SaveObject::applyCallerSuppliedFields()`. No such method exists anywhere + * in OpenRegister — grepped across the whole tree with a positive control.) * - compliancy: `@self.organisation` (system-level owning organisation). * * @category Service @@ -446,10 +450,7 @@ private function repointBySelfOrganisation(string $objectType, string $source, s $count = 0; foreach ($entities as $entity) { - $owningOrganisation = null; - if (method_exists($entity, 'getOrganisation') === true) { - $owningOrganisation = $entity->getOrganisation(); - } + $owningOrganisation = $this->readOwningOrganisation(entity: $entity); if ($owningOrganisation !== $source) { continue; @@ -467,6 +468,65 @@ private function repointBySelfOrganisation(string $objectType, string $source, s return $count; }//end repointBySelfOrganisation() + /** + * Read an OpenRegister object's system-level owning organisation + * (`@self.organisation`). + * + * `ObjectEntity` declares `getOrganisation()` ONLY as an `@method` docblock + * tag over `protected ?string $organisation`, so the accessor is reached + * through `OCP\AppFramework\Db\Entity::__call()`. Two probes are therefore + * wrong here, and both fail silently: + * + * - `method_exists()` is **false** for every such accessor. That was + * softwarecatalog#490: the caller's re-point branch never ran, so a merge + * re-pointed nothing for `contract`/`compliancy` while still tombstoning + * the source organisation. + * - `is_callable()` is **true** for ANY name on a class with `__call()`, so + * swapping the probe would make the branch unconditionally true and move + * the failure into a runtime `BadFunctionCallException`. + * + * `Entity::getter()` itself decides on `property_exists()`, so that is the + * primary instrument below; `method_exists()` is kept as a second arm for + * an entity that genuinely declares the accessor. The call is still + * wrapped, because `$entity` comes from `ObjectService::findAll()` and is + * not type-guaranteed to be an `Entity` subclass. + * + * Deliberately NOT read from `jsonSerialize()`: `ObjectEntity::getObjectArray()` + * types `organisation` as `array|string|null`, so an expanded organisation + * would silently fail the UUID comparison in the caller. The property holds + * the raw `?string`. + * + * @param object $entity The OpenRegister ObjectEntity to read. + * + * @return string|null The owning organisation UUID, or null when the entity carries none. + * + * @spec openspec/specs/organisation-merge/spec.md#requirement-execute-must-re-point-every-relation-type-while-preserving-every-unrelated-field-on-each-object + */ + private function readOwningOrganisation(object $entity): ?string + { + if (property_exists($entity, 'organisation') === false + && method_exists($entity, 'getOrganisation') === false + ) { + return null; + } + + try { + $owningOrganisation = $entity->getOrganisation(); + } catch (\Throwable $e) { + $this->logger->warning( + 'MergeOrganisatieService: could not read @self.organisation from object entity', + ['exception' => $e->getMessage(), 'entity' => $entity::class] + ); + return null; + } + + if (is_string($owningOrganisation) === false) { + return null; + } + + return $owningOrganisation; + }//end readOwningOrganisation() + /** * Save the full existing payload (only the organisation-reference field(s) * mutated) back via OpenRegister's `ObjectService::saveObject()` — diff --git a/lib/Service/ReviewService.php b/lib/Service/ReviewService.php index 1a2bd51c..8b000937 100644 --- a/lib/Service/ReviewService.php +++ b/lib/Service/ReviewService.php @@ -362,15 +362,34 @@ private function resolveTarget(): ?array /** * The uuid of a saved entity (handles entity or array result shapes). * + * `ObjectService::saveObject()` returns an `ObjectEntity`, whose + * `getUuid()` is an `@method` docblock served by `Entity::__call()` over + * `protected ?string $uuid`. A bare `method_exists()` probe is therefore + * FALSE, and because an object is not an array the array arm below cannot + * rescue it — so this method used to return `null` for EVERY real save, + * putting `uuid: null` in the submit response and the audit log + * (softwarecatalog#490). `property_exists()` is the instrument + * `Entity::getter()` itself decides on; `method_exists()` is kept as the + * second arm for genuinely concrete accessors, and the call is wrapped + * because neither probe guarantees the other object's shape. + * * @param mixed $entity The saveObject result. * * @return string|null The uuid, or null. */ private function entityUuid(mixed $entity): ?string { - if (is_object($entity) === true && method_exists($entity, 'getUuid') === true) { - $uuid = $entity->getUuid(); - if (is_string($uuid) === true) { + if (is_object($entity) === true + && (property_exists($entity, 'uuid') === true || method_exists($entity, 'getUuid') === true) + ) { + try { + $uuid = $entity->getUuid(); + } catch (\Throwable $e) { + $this->logger->warning('ReviewService: could not read uuid from saved entity', ['exception' => $e->getMessage()]); + return null; + } + + if (is_string($uuid) === true && $uuid !== '') { return $uuid; } diff --git a/tests/Stubs/Db/ObjectEntity.php b/tests/Stubs/Db/ObjectEntity.php index d1f0042b..d36c8473 100644 --- a/tests/Stubs/Db/ObjectEntity.php +++ b/tests/Stubs/Db/ObjectEntity.php @@ -8,6 +8,30 @@ * stub declares the getters/setters the unit tests stub explicitly. Resolved * via the `OCA\OpenRegister\ => tests/Stubs/` autoload-dev mapping. * + * ⚠️ KNOWN UNFAITHFULNESS — read before adding a declaration here. + * Every accessor below is magic on the REAL ObjectEntity, so declaring it here + * makes `method_exists()` TRUE in the suite and FALSE in production. A test + * built on this stub therefore CANNOT detect a `method_exists()` probe against + * an OpenRegister entity — that is exactly how softwarecatalog#490 (the + * organisation merge re-pointing nothing while still tombstoning the source) + * stayed green for its entire life. `getOrganisation()`/`setOrganisation()` + * were removed from this stub for that reason. + * + * If your subject probes for an accessor, do NOT add it here. Declare the + * attribute as a `protected` PROPERTY instead (as `organisation` is below) and + * build the double as a concrete subclass of this stub rather than a + * `createMock()`, so `Entity::__call()` serves the accessor exactly as it does + * in production. `tests/Unit/Service/MergeOrganisatieServiceTest::entity()` is + * the worked example. + * + * This stub extends the real `OCP\AppFramework\Db\Entity` so that a faithful + * subclass double is still type-compatible with + * `ObjectService::find(): ?ObjectEntity`. That matters: a double that is not + * type-compatible raises a `TypeError` which + * `MergeOrganisatieService::findOrganisatie()` swallows in a + * `catch (\Throwable)`, turning a wiring mistake into a plausible + * `source-not-found` blocker. + * * SPDX-License-Identifier: EUPL-1.2 * * @category Test @@ -18,12 +42,27 @@ namespace OCA\OpenRegister\Db; +use OCP\AppFramework\Db\Entity; + /** * Stub for ObjectEntity with the surface used by SoftwareCatalog tests. */ -abstract class ObjectEntity +abstract class ObjectEntity extends Entity { + /** + * The system-level owning organisation (`@self.organisation`). + * + * A PROPERTY, not a declared accessor — on the real ObjectEntity this is + * `protected ?string $organisation` reached through `Entity::__call()`, so + * `method_exists($entity, 'getOrganisation')` is FALSE and + * `property_exists($entity, 'organisation')` is TRUE. Declaring it this way + * is what lets a test tell the two apart. See softwarecatalog#490. + * + * @var string|null + */ + protected ?string $organisation = null; + /** @return int */ abstract public function getId(); @@ -39,15 +78,6 @@ abstract public function getRegister(); /** @return mixed */ abstract public function getSchema(); - /** @return string|null */ - abstract public function getOrganisation(); - - /** - * @param string|null $organisation - * @return void - */ - abstract public function setOrganisation($organisation=null); - /** * @param array|null $object * @return self diff --git a/tests/Unit/Service/IntakeModerationTest.php b/tests/Unit/Service/IntakeModerationTest.php index 86aa73d3..499a1569 100644 --- a/tests/Unit/Service/IntakeModerationTest.php +++ b/tests/Unit/Service/IntakeModerationTest.php @@ -35,6 +35,7 @@ use OCA\SoftwareCatalog\Service\IntakeService; use OCA\SoftwareCatalog\Service\ModerationService; use OCA\SoftwareCatalog\Service\SettingsService; +use OCP\AppFramework\Db\Entity; use PHPUnit\Framework\TestCase; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -123,6 +124,45 @@ public function testMissingRequiredFieldRejected(): void $this->assertSame([], $this->saved); }//end testMissingRequiredFieldRejected() + /** + * `entityUuid()` must read the uuid off a saved entity whose `getUuid()` + * is reached through `Entity::__call()` — which is what every real + * OpenRegister `ObjectEntity` returned by `saveObject()` does. + * + * With the old `method_exists()` probe this returned `null` for EVERY real + * save (the `is_array()` arm cannot rescue an object), so `submit()` + * answered `uuid: null` to the client and wrote `['uuid' => null]` to the + * audit log — softwarecatalog#490. See the twin test in ReviewServiceTest; + * the two services carry byte-identical copies of this helper. + * + * @return void + */ + public function testEntityUuidReadsAMagicAccessorUuid(): void + { + $entity = new class extends Entity { + + /** + * The uuid — a property reached via __call, as on ObjectEntity. + * + * @var string|null + */ + protected ?string $uuid = null; + }; + $entity->setUuid('intake-uuid-1'); + + $this->assertFalse( + method_exists($entity, 'getUuid'), + 'the double must reach getUuid() through __call, like the real ObjectEntity' + ); + + $intake = new IntakeService($this->container($this->objectService([])), $this->settings(), $this->logger()); + + $method = new \ReflectionMethod($intake, 'entityUuid'); + $method->setAccessible(true); + + $this->assertSame('intake-uuid-1', $method->invoke($intake, $entity)); + }//end testEntityUuidReadsAMagicAccessorUuid() + /** * Anti-spam validation: oversized value is rejected. * diff --git a/tests/Unit/Service/MergeOrganisatieServiceTest.php b/tests/Unit/Service/MergeOrganisatieServiceTest.php index 9c39fd6e..41673988 100644 --- a/tests/Unit/Service/MergeOrganisatieServiceTest.php +++ b/tests/Unit/Service/MergeOrganisatieServiceTest.php @@ -32,6 +32,7 @@ use OCA\SoftwareCatalog\Service\SettingsService; use OCA\SoftwareCatalog\Service\SoftwareCatalogue\OrganizationHandler; use OCP\App\IAppManager; +use OCP\AppFramework\Db\Entity; use OCP\EventDispatcher\IEventDispatcher; use OCP\IGroup; use OCP\IGroupManager; @@ -211,6 +212,130 @@ public function testUntouchedContractFieldsSurviveRepointing(): void $this->assertSame('doc-ref', $contractSave['object']['documentReferentie']); }//end testUntouchedContractFieldsSurviveRepointing() + /** + * The `entity()` double asserts its own premise: the real + * OpenRegister `ObjectEntity` declares `getOrganisation()` only as an + * `@method` docblock tag over `protected ?string $organisation`, so it is + * reached through `Entity::__call()`. That makes `method_exists()` FALSE + * and `property_exists()` TRUE on the property the framework itself keys + * on (`Entity::getter()` does exactly this check). + * + * Without this test the fixture could silently drift back to the concrete + * shape of `tests/Stubs/Db/ObjectEntity.php` — which is precisely how the + * defect this file now covers stayed green for its entire life. + * + * @return void + * + * @spec openspec/specs/organisation-merge/spec.md#requirement-execute-must-re-point-every-relation-type-while-preserving-every-unrelated-field-on-each-object + */ + public function testTheMagicEntityDoubleMatchesTheRealObjectEntityAccessorShape(): void + { + $entity = $this->entity(['id' => 'c1'], uuid: 'c1', organisation: 'org-a'); + + $this->assertFalse( + method_exists($entity, 'getOrganisation'), + 'the double must reach getOrganisation() through __call, like the real ObjectEntity' + ); + $this->assertTrue( + property_exists($entity, 'organisation'), + 'property_exists() is the instrument Entity::getter() itself uses' + ); + $this->assertSame('org-a', $entity->getOrganisation()); + }//end testTheMagicEntityDoubleMatchesTheRealObjectEntityAccessorShape() + + /** + * Objects owned through the system-level `@self.organisation` field + * (`contract`, `compliancy`) are re-pointed when the entity reaches + * `getOrganisation()` through `Entity::__call()` — which is what every + * real OpenRegister `ObjectEntity` does. + * + * This is the regression test for #490: with a `method_exists()` probe the + * branch below never ran, so `execute()` re-pointed NOTHING for these two + * relation types while still tombstoning the source organisation — leaving + * live objects owned by a retired organisation. + * + * @return void + * + * @spec openspec/specs/organisation-merge/spec.md#requirement-execute-must-re-point-every-relation-type-while-preserving-every-unrelated-field-on-each-object + */ + public function testSelfOrganisationRelationsAreRepointedForMagicAccessorEntities(): void + { + $service = $this->makeService( + organisations: [ + 'org-a' => $this->entity(['id' => 'org-a', 'status' => 'Actief']), + 'org-b' => $this->entity(['id' => 'org-b', 'status' => 'Actief']), + ], + typedFixtures: [ + 'contract' => [ + $this->entity( + ['id' => 'c1', 'contractNummer' => 'C-100', 'kosten' => 5000], + uuid: 'c1', + organisation: 'org-a' + ), + $this->entity(['id' => 'c2'], uuid: 'c2', organisation: 'org-b'), + ], + 'compliancy' => [ + $this->entity(['id' => 'cp1'], uuid: 'cp1', organisation: 'org-a'), + ], + ], + groupMembers: [] + ); + + $result = $service->execute(sourceUuid: 'org-a', targetUuid: 'org-b'); + + $this->assertSame(1, $result['counts']['contract'], 'the contract owned by the source MUST be counted'); + $this->assertSame(1, $result['counts']['compliancy'], 'the compliancy owned by the source MUST be counted'); + + $contractSave = $this->findSave(schemaId: self::SCHEMA_IDS['contract'], uuid: 'c1'); + $this->assertNotNull($contractSave, 'the contract MUST be re-pointed, not silently skipped'); + $this->assertSame('org-b', $contractSave['object']['@self']['organisation']); + // PUT-semantics: every unrelated field is carried forward. + $this->assertSame('C-100', $contractSave['object']['contractNummer']); + $this->assertSame(5000, $contractSave['object']['kosten']); + + $this->assertNotNull($this->findSave(schemaId: self::SCHEMA_IDS['compliancy'], uuid: 'cp1')); + $this->assertNull( + $this->findSave(schemaId: self::SCHEMA_IDS['contract'], uuid: 'c2'), + 'a contract already owned by the target MUST NOT be re-saved' + ); + + // The data-loss half of #490: the source is tombstoned either way, so a + // silent zero here leaves live objects owned by a retired organisation. + $tombstone = $this->findSave(schemaId: self::SCHEMA_IDS['organisatie'], uuid: 'org-a'); + $this->assertNotNull($tombstone); + $this->assertSame('samengevoegd', $tombstone['object']['status']); + }//end testSelfOrganisationRelationsAreRepointedForMagicAccessorEntities() + + /** + * Dry-run and execute agree for magic-accessor entities too. Before #490 + * they agreed only because BOTH were equally broken (0 == 0), so the + * parity assertion could not detect the defect on its own. + * + * @return void + * + * @spec openspec/specs/organisation-merge/spec.md#requirement-dry-run-and-execute-must-report-structurally-identical-counts-for-the-same-unchanged-input + */ + public function testDryRunCountsSelfOrganisationRelationsForMagicAccessorEntities(): void + { + $service = $this->makeService( + organisations: [ + 'org-a' => $this->entity(['id' => 'org-a', 'status' => 'Actief']), + 'org-b' => $this->entity(['id' => 'org-b', 'status' => 'Actief']), + ], + typedFixtures: [ + 'contract' => [$this->entity(['id' => 'c1'], uuid: 'c1', organisation: 'org-a')], + 'compliancy' => [$this->entity(['id' => 'cp1'], uuid: 'cp1', organisation: 'org-a')], + ], + groupMembers: [] + ); + + $result = $service->dryRun(sourceUuid: 'org-a', targetUuid: 'org-b'); + + $this->assertSame(1, $result['counts']['contract']); + $this->assertSame(1, $result['counts']['compliancy']); + $this->assertSame([], $this->savedCalls, 'dry-run MUST NOT write any object'); + }//end testDryRunCountsSelfOrganisationRelationsForMagicAccessorEntities() + /** * A gebruik object with the source as one of several deelnemers only * replaces the matching entry. @@ -636,7 +761,21 @@ function (string $gid) use ($groupMembers) { }//end makeService() /** - * Build an ObjectEntity mock returning $data / $uuid / $organisation. + * Build a FAITHFUL OpenRegister ObjectEntity double: a concrete subclass of + * the `ObjectEntity` stub (which extends `OCP\AppFramework\Db\Entity`), + * whose `organisation` and `uuid` attributes are reached through + * `Entity::__call()`, exactly as the real `ObjectEntity` reaches them. + * + * This used to return `createMock(ObjectEntity::class)` over a stub that + * declared `getOrganisation()` CONCRETELY — PHPUnit 10 removed + * `addMethods()`, so a mock cannot configure a magic accessor. That double + * made `method_exists($entity, 'getOrganisation')` TRUE in the suite and + * FALSE in production, i.e. it inverted the exact predicate under test, and + * is why softwarecatalog#490 was green here for its entire life. + * + * It must be a SUBCLASS, not an arbitrary `Entity`: `ObjectService::find()` + * declares `?ObjectEntity`, and an incompatible return raises a `TypeError` + * that `findOrganisatie()` swallows into a `source-not-found` blocker. * * @param array $data The object payload (getObject()). * @param string|null $uuid The uuid (defaults to $data['id']). @@ -646,10 +785,100 @@ function (string $gid) use ($groupMembers) { */ private function entity(array $data, ?string $uuid=null, ?string $organisation=null): ObjectEntity { - $entity = $this->createMock(ObjectEntity::class); - $entity->method('getObject')->willReturn($data); - $entity->method('getUuid')->willReturn($uuid ?? (string) ($data['id'] ?? '')); - $entity->method('getOrganisation')->willReturn($organisation); + $entity = new class extends ObjectEntity { + + /** + * The object uuid — a property, reached via __call as on the real entity. + * + * @var string|null + */ + protected ?string $uuid = null; + + /** + * The object payload. + * + * @var array|null + */ + protected ?array $object = null; + + /** + * The numeric database id. + * + * @return int + */ + public function getId() + { + return 0; + }//end getId() + + /** + * The object uuid. + * + * @return string + */ + public function getUuid() + { + return (string) $this->uuid; + }//end getUuid() + + /** + * Mirrors ObjectEntity::getObject(), which is explicitly declared + * (not magic) on the real entity because it injects the uuid as `id`. + * + * @return array + */ + public function getObject() + { + return array_merge(['id' => $this->uuid], ($this->object ?? [])); + }//end getObject() + + /** + * The register id — unused by these tests. + * + * @return mixed + */ + public function getRegister() + { + return null; + }//end getRegister() + + /** + * The schema id — unused by these tests. + * + * @return mixed + */ + public function getSchema() + { + return null; + }//end getSchema() + + /** + * Set the object payload. + * + * @param array|null $object The payload. + * + * @return self + */ + public function setObject($object=null) + { + $this->object = $object; + return $this; + }//end setObject() + + /** + * Serialise the payload. + * + * @return array + */ + public function jsonSerialize() + { + return $this->getObject(); + }//end jsonSerialize() + }; + + $entity->setUuid($uuid ?? (string) ($data['id'] ?? '')); + $entity->setObject($data); + $entity->setOrganisation($organisation); return $entity; }//end entity() diff --git a/tests/Unit/Service/ReviewServiceTest.php b/tests/Unit/Service/ReviewServiceTest.php index a1da5d1b..eb40bffe 100644 --- a/tests/Unit/Service/ReviewServiceTest.php +++ b/tests/Unit/Service/ReviewServiceTest.php @@ -30,6 +30,7 @@ use OCA\OpenRegister\Service\ObjectService; use OCA\SoftwareCatalog\Service\ReviewService; use OCA\SoftwareCatalog\Service\SettingsService; +use OCP\AppFramework\Db\Entity; use OCP\IUser; use OCP\IUserSession; use PHPUnit\Framework\TestCase; @@ -228,6 +229,57 @@ public function testInvalidSubjectTypeRejected(): void $this->assertSame([], $this->saved); }//end testInvalidSubjectTypeRejected() + /** + * `entityUuid()` must read the uuid off a saved entity whose `getUuid()` + * is reached through `Entity::__call()` — which is what every real + * OpenRegister `ObjectEntity` returned by `saveObject()` does. + * + * With the old `method_exists()` probe this returned `null` for EVERY real + * save (the `is_array()` arm cannot rescue an object), so `submit()` + * answered `uuid: null` to the client and wrote `['uuid' => null]` to the + * audit log — softwarecatalog#490. + * + * The private method is exercised directly because the shared + * `tests/Stubs/Db/ObjectEntity` still declares `getUuid()` concretely (8 + * other test files configure it on a mock), so a double routed through + * `saveObject()`'s `: ObjectEntity` return type cannot express the magic + * shape. Recorded as remaining debt rather than hidden. + * + * @return void + * + * @spec openspec/specs/catalog-ratings/spec.md#requirement-a-newly-submitted-review-must-require-moderation-approval-before-becoming-public + */ + public function testEntityUuidReadsAMagicAccessorUuid(): void + { + $entity = new class extends Entity { + + /** + * The uuid — a property reached via __call, as on ObjectEntity. + * + * @var string|null + */ + protected ?string $uuid = null; + }; + $entity->setUuid('review-uuid-1'); + + $this->assertFalse( + method_exists($entity, 'getUuid'), + 'the double must reach getUuid() through __call, like the real ObjectEntity' + ); + + $service = new ReviewService( + $this->container($this->objectService([])), + $this->settings(), + $this->userSession($this->user('jan.jansen', 'Jan Jansen')), + $this->logger() + ); + + $method = new \ReflectionMethod($service, 'entityUuid'); + $method->setAccessible(true); + + $this->assertSame('review-uuid-1', $method->invoke($service, $entity)); + }//end testEntityUuidReadsAMagicAccessorUuid() + /** * Build an ObjectService mock whose searchObjects returns $found and whose * saveObject captures the data bag. From c9fd937c52237dd26db32935d2e767cb3690f720 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 12 Aug 2026 01:51:39 +0200 Subject: [PATCH 2/2] fix(tests): keep the ObjectEntity stub free-standing so it loads under both bootstraps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit made the stub extend OCP\AppFramework\Db\Entity. That is fine under tests/bootstrap-unit.php, which registers an OCP autoloader, but tests/bootstrap.php require_once's every file in tests/Stubs/ BEFORE Nextcloud's lib/base.php — deliberately, so the stub wins over the real OpenRegister class during mock generation. At that point no OCP class is resolvable, so the whole suite died in the bootstrap with Error in bootstrap script: Class "OCP\AppFramework\Db\Entity" not found on both PHPUnit cells. The local unit run could not see it because phpunit-unit.xml uses the other bootstrap. The stub now mirrors Entity's __call/getter/setter triple instead of inheriting it, so it has no load-time dependency at all. The semantics that the fix turns on are reproduced exactly: get*/set* resolve through property_exists(), anything else raises BadFunctionCallException. Verified by replaying the exact failing bootstrap step — vendor/autoload.php plus the tests/Stubs glob, with no Nextcloud and no OCP autoloader. The committed version fatals there; this version loads clean. The revert prediction is unchanged: reverting the merge probe still turns exactly the same 6 tests red. --- tests/Stubs/Db/ObjectEntity.php | 93 ++++++++++++++++--- .../Service/MergeOrganisatieServiceTest.php | 8 +- 2 files changed, 86 insertions(+), 15 deletions(-) diff --git a/tests/Stubs/Db/ObjectEntity.php b/tests/Stubs/Db/ObjectEntity.php index d36c8473..f0a2be2e 100644 --- a/tests/Stubs/Db/ObjectEntity.php +++ b/tests/Stubs/Db/ObjectEntity.php @@ -20,17 +20,27 @@ * If your subject probes for an accessor, do NOT add it here. Declare the * attribute as a `protected` PROPERTY instead (as `organisation` is below) and * build the double as a concrete subclass of this stub rather than a - * `createMock()`, so `Entity::__call()` serves the accessor exactly as it does - * in production. `tests/Unit/Service/MergeOrganisatieServiceTest::entity()` is + * `createMock()`, so `__call()` serves the accessor exactly as it does in + * production. `tests/Unit/Service/MergeOrganisatieServiceTest::entity()` is * the worked example. * - * This stub extends the real `OCP\AppFramework\Db\Entity` so that a faithful - * subclass double is still type-compatible with - * `ObjectService::find(): ?ObjectEntity`. That matters: a double that is not - * type-compatible raises a `TypeError` which - * `MergeOrganisatieService::findOrganisatie()` swallows in a - * `catch (\Throwable)`, turning a wiring mistake into a plausible - * `source-not-found` blocker. + * A faithful double must be a SUBCLASS of this stub, not of some other base: + * `ObjectService::find()` declares `?ObjectEntity`, and an incompatible return + * raises a `TypeError` that `MergeOrganisatieService::findOrganisatie()` + * swallows in a `catch (\Throwable)`, turning a wiring mistake into a + * plausible-looking `source-not-found` blocker. + * + * ⚠️ The `__call`/`getter`/`setter` triple below MIRRORS + * `OCP\AppFramework\Db\Entity` (`:159`, `:175`) rather than inheriting it, and + * that is deliberate. `tests/bootstrap.php` `require_once`s every file in + * `tests/Stubs/` BEFORE Nextcloud's `lib/base.php`, precisely so this stub wins + * over the real OpenRegister class during mock generation — so at load time no + * `OCP\` class is resolvable yet, and extending one makes the whole suite die + * in the bootstrap with `Class "OCP\AppFramework\Db\Entity" not found`. + * Keeping the stub free-standing is what lets it load under BOTH + * `tests/bootstrap.php` and `tests/bootstrap-unit.php`. The semantics that + * matter are reproduced exactly: `get*`/`set*` resolve through + * `property_exists()`, anything else raises `BadFunctionCallException`. * * SPDX-License-Identifier: EUPL-1.2 * @@ -42,12 +52,12 @@ namespace OCA\OpenRegister\Db; -use OCP\AppFramework\Db\Entity; +use BadFunctionCallException; /** * Stub for ObjectEntity with the surface used by SoftwareCatalog tests. */ -abstract class ObjectEntity extends Entity +abstract class ObjectEntity { /** @@ -63,6 +73,67 @@ abstract class ObjectEntity extends Entity */ protected ?string $organisation = null; + /** + * Magic accessor dispatch, mirroring `OCP\AppFramework\Db\Entity::__call()`. + * + * @param string $method The called method name. + * @param array $args The call arguments. + * + * @return mixed + * + * @throws BadFunctionCallException When the name maps to no attribute. + */ + public function __call(string $method, array $args) + { + if (str_starts_with($method, 'get') === true) { + return $this->getter(lcfirst(substr($method, 3))); + } + + if (str_starts_with($method, 'set') === true) { + $this->setter(lcfirst(substr($method, 3)), $args); + return $this; + } + + throw new BadFunctionCallException($method.' does not exist'); + }//end __call() + + /** + * Generic attribute read, mirroring `Entity::getter()`. + * + * @param string $name The attribute name. + * + * @return mixed + * + * @throws BadFunctionCallException When no such property exists. + */ + protected function getter(string $name) + { + if (property_exists($this, $name) === false) { + throw new BadFunctionCallException($name.' is not a valid attribute'); + } + + return $this->$name; + }//end getter() + + /** + * Generic attribute write, mirroring `Entity::setter()`. + * + * @param string $name The attribute name. + * @param array $args The call arguments. + * + * @return void + * + * @throws BadFunctionCallException When no such property exists. + */ + protected function setter(string $name, array $args): void + { + if (property_exists($this, $name) === false) { + throw new BadFunctionCallException($name.' is not a valid attribute'); + } + + $this->$name = ($args[0] ?? null); + }//end setter() + /** @return int */ abstract public function getId(); diff --git a/tests/Unit/Service/MergeOrganisatieServiceTest.php b/tests/Unit/Service/MergeOrganisatieServiceTest.php index 41673988..b6e7cc5f 100644 --- a/tests/Unit/Service/MergeOrganisatieServiceTest.php +++ b/tests/Unit/Service/MergeOrganisatieServiceTest.php @@ -32,7 +32,6 @@ use OCA\SoftwareCatalog\Service\SettingsService; use OCA\SoftwareCatalog\Service\SoftwareCatalogue\OrganizationHandler; use OCP\App\IAppManager; -use OCP\AppFramework\Db\Entity; use OCP\EventDispatcher\IEventDispatcher; use OCP\IGroup; use OCP\IGroupManager; @@ -762,9 +761,10 @@ function (string $gid) use ($groupMembers) { /** * Build a FAITHFUL OpenRegister ObjectEntity double: a concrete subclass of - * the `ObjectEntity` stub (which extends `OCP\AppFramework\Db\Entity`), - * whose `organisation` and `uuid` attributes are reached through - * `Entity::__call()`, exactly as the real `ObjectEntity` reaches them. + * the `ObjectEntity` stub, whose `organisation` and `uuid` attributes are + * reached through the stub's `__call()` — which mirrors + * `OCP\AppFramework\Db\Entity::__call()`, exactly as the real + * `ObjectEntity` reaches them. * * This used to return `createMock(ObjectEntity::class)` over a stub that * declared `getOrganisation()` CONCRETELY — PHPUnit 10 removed