fix(merge): organisation merge re-points nothing — probe a magic accessor with property_exists (#491)

* fix(merge): organisation merge re-points nothing — probe a magic accessor with property_exists

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

* fix(tests): keep the ObjectEntity stub free-standing so it loads under both bootstraps

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.

Author: rubenvdlinde

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

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()` —

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

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<mixed> $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<mixed> $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<string,mixed>|null $object
      * @return self

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.
      *