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..f0a2be2e 100644 --- a/tests/Stubs/Db/ObjectEntity.php +++ b/tests/Stubs/Db/ObjectEntity.php @@ -8,6 +8,40 @@ * 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 `__call()` serves the accessor exactly as it does in + * production. `tests/Unit/Service/MergeOrganisatieServiceTest::entity()` is + * the worked example. + * + * 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 * * @category Test @@ -18,12 +52,88 @@ namespace OCA\OpenRegister\Db; +use BadFunctionCallException; + /** * Stub for ObjectEntity with the surface used by SoftwareCatalog tests. */ abstract class ObjectEntity { + /** + * 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; + + /** + * 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(); @@ -39,15 +149,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..b6e7cc5f 100644 --- a/tests/Unit/Service/MergeOrganisatieServiceTest.php +++ b/tests/Unit/Service/MergeOrganisatieServiceTest.php @@ -211,6 +211,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 +760,22 @@ 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, 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 + * `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.