Organisation merge repoints nothing — repointBySelfOrganisation() probes a magic accessor, so every object is skipped while the source org is tombstoned

[rubenvdlinde]

Summary

MergeOrganisatieService::repointBySelfOrganisation() reads the owning organisation with a method_exists() probe:

// lib/Service/MergeOrganisatieService.php:450
if (method_exists($entity, 'getOrganisation') === true) { $owningOrganisation = ...; }
...
if ($owningOrganisation !== $source) { continue; }

$entity is an OCA\OpenRegister\Db\ObjectEntity. getOrganisation() is an @method docblock served by Entity::__call(), so the probe is false, $owningOrganisation stays null, and the next line continues every object.

Measured

Live, against the server's own Entity.php:

subject	method_exists	is_callable
ObjectEntity::getOrganisation	false	true
ObjectEntity::getObject — concrete control	true	true

Consequence

repointBySelfOrganisation() always returns 0. Meanwhile tombstoneSource() still tombstones the source organisation, so contract and compliancy objects are left pointing at @self.organisation = <tombstoned org>. The merge reports success having moved nothing.

The parity check between dry-run and execute cannot catch this: both arms are equally broken, so they agree. This is the same shape as an A/B whose two arms share the defect.

Also in this repo, lower rank

ReviewService.php:371 and IntakeService.php:278 — same probe on getUuid, no working fallback (is_array() is false for an ObjectEntity), so both return null. submit() returns uuid: null to the client and writes ['uuid' => null] to the audit log.

Seven further getUuid probes in this repo (OrganizationContactSyncJob:221, MigrateContactsToNc:386, ModerationService:344, ReviewAggregateService:279, FederationService:659, EolSyncService:463) are dead code with no runtime effect — ObjectEntity::getObject() always injects the uuid under id, so their $data['id'] fallback already holds. Worth tidying, not worth prioritising.

Fix

is_callable() is not a membership test on a __call class — measured, it is true for any name and the call then raises BadFunctionCallException. So swapping the probe yields an always-true guard, and the call must be made exception-safe in the same edit.

The most robust route here is to read @self.organisation from jsonSerialize(), which is genuinely concrete.

Please reshape the double in the same PR

Whatever test currently covers the merge will be green because its double declares the accessor concretely — that is what happened in every other confirmed instance of this bug. A fix without reshaping the double proves nothing. Write down how many tests you expect to go red when the fix is reverted, then check.

decidesk/tests/Stubs/Db/ObjectEntity.php is the fleet's reference double: it extends Entity, declares only genuinely concrete methods, and documents why.

Fleet context and the full 18-repo table: fleet-board/findings/method-exists-sweep.md.

[rubenvdlinde]

Fixed in #491, squash-merged to development as d401ab72. Verified by reading the merged tree, not by trusting the merge.

What shipped

repointBySelfOrganisation() now reads the owning organisation through readOwningOrganisation(), which decides on property_exists($entity, 'organisation') — the instrument Entity::getter() itself uses — keeps method_exists as a second arm for a genuinely concrete accessor, and wraps the call in try/catch (\Throwable) with an is_string check in the same edit. is_callable() was deliberately not used: it is true for every name on a __call class, so it would have made the branch unconditionally true and moved the failure into a runtime BadFunctionCallException.

The two sibling sites you flagged are fixed the same way: ReviewService::entityUuid() and IntakeService::entityUuid() returned null for every real save, because saveObject() returns an object and the is_array() arm below cannot rescue it.

The seven other getUuid probes were read and left alone — ObjectEntity::getObject() always injects id => uuid, so their $data['id'] fallback already holds. Your triage of them matches mine.

The double, which was the harder half

You were right that the existing test would be green. It was worse than that: tests/Stubs/Db/ObjectEntity declared getOrganisation() and had no organisation property, so it disagreed with production on both probes, in opposite directions — method_exists true where production is false, property_exists false where production is true. Landing the prescribed fix alone turns four previously-green tests red, which reads exactly like a regression.

The stub now declares organisation as a property, no longer declares the accessor, mirrors Entity's __call/getter/setter, and carries a warning about what adding an accessor there costs. The merge suite's double is a concrete subclass, and one test asserts its premise (method_exists false, property_exists true) so the fixture cannot drift back.

Revert predictions, written before reverting and reverted with the editor, not a script: reverting only the merge probe was predicted to redden six named tests — observed six, exactly those, with a grep confirming which line carried the revert. Reverting only the two entityUuid probes was predicted to redden two and nothing else — observed exactly that over the full 667-test suite.

Correction to the suggested fix

The issue proposes reading @self.organisation off jsonSerialize(). That works, but ObjectEntity::getObjectArray() types organisation as array|string|null, so an expanded organisation would silently fail the UUID comparison in the caller. The raw property is ?string. The docblock also credited the write path to SaveObject::applyCallerSuppliedFields() — that method exists nowhere in OpenRegister (grepped whole-tree with a positive control). The real path is SaveObject::setSelfMetadata(). Corrected in place.

What I could NOT prove, and it matters

This is not a case where the correctly-probed read returns a value the guard can never match — checked, because that failure mode was live elsewhere in the fleet tonight. The column is Types::STRING(255); getOrganisationForNewEntity() returns a UUID; and OrganisatieService creates the OpenRegister-core Organisation with the organisatie object's own UUID (live: 7 of 9 core organisations are UUID-aligned with an organisatie object). So both sides of $owningOrganisation !== $source are organisatie UUIDs. No slug or numeric id is involved anywhere on this path.

But the end-to-end effect is not demonstrable on the shared dev instance. Every contract/compliancy shard, counted:

shard	rows	owned by a merge-able organisatie
..._11_5034 contract	0	0
..._11_51 compliancy	10	0
..._13_5034 / ..._13_51 / ..._16_5334	0	0

All ten compliancy rows carry the same _organisation, which is an OpenRegister-core organisation with no organisatie object — the default org, which cannot be a merge source. A live before/after merge therefore returns 0 → 0 under the old code and the new code alike, so running one would be a control that could not have moved.

What is established: the branch executes, the save payload carries @self.organisation = target (and that is the key SaveObject::setSelfMetadata() actually reads, checked in source), and the two operands are provably in one identifier space. What is not established is a row that changed owner. Anyone with a populated instance should confirm that before treating this as verified end-to-end.

Closing on the code fix. Reopen if a populated instance shows otherwise.