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

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