Merge pull request #535 from ConductionNL/fix/green-softwarecatalog

fix: close the ADR-084 contract drift behind 19 PHPUnit failures and 44 static findings

Author: rubenvdlinde

lib/AppInfo/Application.php

@@ -377,7 +377,6 @@ function ($container) {
 				return new GebruikSyncService(
 					logger: $container->get('Psr\Log\LoggerInterface'),
 					settingsService: $container->get(SettingsService::class),
-					container: $container,
 					objectService: $container->get(ObjectServiceInterface::class),
 				);
 			}
@@ -738,7 +737,6 @@ function ($container) {
 					userManager: $container->get('OCP\IUserManager'),
 					groupManager: $container->get('OCP\IGroupManager'),
 					userSession: $container->get('OCP\IUserSession'),
-					container: $container,
 					secureRandom: $container->get('OCP\Security\ISecureRandom'),
 					logger: $container->get('Psr\Log\LoggerInterface'),
 					// ADR-084 added these two to the constructor; this hand-written

lib/Controller/ContactpersonenController.php

@@ -33,7 +33,6 @@
 use OCP\IUserManager;
 use OCP\IUserSession;
 use OCP\Security\ISecureRandom;
-use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 use OCA\OpenRegister\Contract\ObjectServiceInterface;
 use OCA\OpenRegister\Service\OrganisationService;
@@ -111,13 +110,6 @@ class ContactpersonenController extends Controller {
 	 */
 	private IUserSession $userSession;
 
-	/**
-	 * Container for dependency injection.
-	 *
-	 * @var ContainerInterface
-	 */
-	private ContainerInterface $container;
-
 	/**
 	 * Contactpersoon service for business logic.
 	 *
@@ -136,7 +128,6 @@ class ContactpersonenController extends Controller {
 	 * @param IUserManager $userManager User manager
 	 * @param IGroupManager $groupManager Group manager
 	 * @param IUserSession $userSession User session
-	 * @param ContainerInterface $container Container for DI
 	 * @param ISecureRandom $secureRandom Secure random generator
 	 * @param LoggerInterface $logger Logger instance
 	 * @param ObjectServiceInterface $objectService OpenRegister object access (ADR-022/ADR-084 —
@@ -154,7 +145,6 @@ public function __construct(
 		IUserManager $userManager,
 		IGroupManager $groupManager,
 		IUserSession $userSession,
-		ContainerInterface $container,
 		ISecureRandom $secureRandom,
 		LoggerInterface $logger,
 		private readonly ObjectServiceInterface $objectService,
@@ -167,7 +157,6 @@ public function __construct(
 		$this->userManager = $userManager;
 		$this->groupManager = $groupManager;
 		$this->userSession = $userSession;
-		$this->container = $container;
 		$this->secureRandom = $secureRandom;
 		$this->logger = $logger;
 	}//end __construct()
@@ -515,7 +504,12 @@ public function convertToUser(string $contactPersonId): JSONResponse {
 
 			$contactData = $this->normaliseContactDataForPersist(contactData: $contactData);
 
-			$contactPersonObject->setObject($contactData);
+			// No local `setObject()` on the entity here. `setObject()` is not on
+			// the published ObjectEntityInterface (ADR-084) — it is an
+			// implementation-only accessor reached through Entity::__call() — and
+			// mutating the in-memory copy changed nothing anyway: the save below
+			// sends `$contactData` itself, and the entity is only read again for
+			// its UUID.
 
 			// Debug logging to understand data types before save.
 			$lastNameValue = $contactData['achternaam'] ?? 'not set';
@@ -550,8 +544,7 @@ public function convertToUser(string $contactPersonId): JSONResponse {
 				schema: $schemaId,
 				uuid: $contactPersonObject->getUuid(),
 				silent: true,
-				silent: true,
-			_validation: false
+				_validation: false
 			);
 
 			$this->logger->info(

lib/EventListener/UserProfileUpdatedEventListener.php

@@ -26,9 +26,6 @@
 use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 use OCA\OpenRegister\Contract\ObjectServiceInterface;
-use OCA\OpenRegister\Db\SchemaMapper;
-use OCA\OpenRegister\Db\RegisterMapper;
-use OCA\OpenRegister\Service\Object\SaveObject\MetadataHydrationHandler;
 
 /**
  * Syncs user profile changes to the corresponding contactpersoon object.
@@ -58,16 +55,10 @@ class UserProfileUpdatedEventListener implements IEventListener {
 	 *
 	 * @param ContainerInterface $container DI container for lazy service resolution.
 	 * @param ObjectServiceInterface $objectService OpenRegister object access (ADR-084 contract).
-	 * @param SchemaMapper $schemaMapper Resolves the contactpersoon schema.
-	 * @param RegisterMapper $registerMapper Resolves the register the schema lives in.
-	 * @param MetadataHydrationHandler $metadataHydrationHandler Hydrates `@self` metadata on read.
 	 */
 	public function __construct(
 		private readonly ContainerInterface $container,
 		private readonly ObjectServiceInterface $objectService,
-		private readonly SchemaMapper $schemaMapper,
-		private readonly RegisterMapper $registerMapper,
-		private readonly MetadataHydrationHandler $metadataHydrationHandler,
 	) {
 	}//end __construct()
 
@@ -207,16 +198,16 @@ private function syncToContactPerson(UserProfileUpdatedEvent $event, LoggerInter
 			]
 		);
 
-		// Merge the patch into existing data and save directly via mapper to skip schema validation.
-		// Schema validation can reject existing data with legacy values (e.g. notificaties enum).
+		// Merge the patch into the existing payload. `saveObject()` is
+		// PUT-semantic, so the FULL payload must be carried forward, not only
+		// the changed keys. Validation is skipped on the save because schema
+		// validation can reject pre-existing legacy values (e.g. notificaties
+		// enum) that this listener never touched.
 		$mergedObject = array_merge($contactData, $patch);
-		$contactPerson->setObject($mergedObject);
 
 		$this->persistContactPersonPatch(
 			contactPerson: $contactPerson,
-			register:       (int)$register,
-			schema:         (int)$contactPersonSchema,
-			logger:         $logger
+			object:        $mergedObject
 		);
 
 		$logger->info(
@@ -278,63 +269,39 @@ private function buildContactPatch(
 	}//end buildContactPatch()
 
 	/**
-	 * Persist the patched contactpersoon entity, regenerating `_name`
-	 * metadata first when the schema is loadable.
+	 * Persist the patched contactpersoon payload through the published
+	 * OpenRegister contract.
 	 *
-	 * @param object $contactPerson The contactpersoon entity.
-	 * @param int $register The voorzieningen register id.
-	 * @param int $schema The contactpersoon schema id.
-	 * @param LoggerInterface $logger Logger for hydration warnings.
+	 * `_name` metadata is NOT regenerated here. This method used to load the
+	 * schema through `SchemaMapper` and call
+	 * `MetadataHydrationHandler::hydrateObjectMetadata()` itself — both of them
+	 * OpenRegister internals that ADR-084's published contract deliberately does
+	 * not expose, and neither of which a leaf app can load in its own unit
+	 * tests. It was also redundant: `ObjectService::saveObject()` calls
+	 * `hydrateObjectMetadata()` on both its create and its update path before
+	 * handing the entity to `objectEntityMapper`, so the hydration happens
+	 * exactly once either way — it was simply being done twice, one layer too
+	 * deep.
+	 *
+	 * @param object $contactPerson The contactpersoon entity (read for its coordinates).
+	 * @param array $object The full merged payload to store (PUT-semantic).
 	 *
 	 * @return void
 	 */
 	private function persistContactPersonPatch(
 		object $contactPerson,
-		int $register,
-		int $schema,
-		LoggerInterface $logger,
+		array $object,
 	): void {
-		$schemaEntity = null;
-		$registerEntity = null;
-		try {
-			$schemaEntity = $this->schemaMapper->find(id: $schema, _rbac: false, _multitenancy: false);
-			$registerEntity = $this->registerMapper->find(id: $register, _rbac: false, _multitenancy: false);
-		} catch (\Exception $e) {
-			$logger->warning(
-				'[UserProfileUpdatedEventListener] Could not load schema/register entities for _name hydration',
-				[
-					'error' => $e->getMessage(),
-				]
-			);
-		}
-
-		if ($schemaEntity !== null) {
-			$this->metadataHydrationHandler->hydrateObjectMetadata(entity: $contactPerson, schema: $schemaEntity);
-			$logger->debug(
-				'[UserProfileUpdatedEventListener] Regenerated _name metadata',
-				[
-					'newName' => $contactPerson->getName(),
-				]
-			);
-		}
-
-		// Persist through the published contract rather than OpenRegister's Db
-		// layer. The comment this replaces worried that a plain save would touch
-		// "just the blob table" — it does not: ObjectService::saveObject() calls
-		// metaHydrationHandler->hydrateObjectMetadata() and then
-		// objectEntityMapper->update(entity:, register:, schema:), which IS the
-		// magic-mapper route. This listener was hand-rolling OpenRegister's own
-		// save pipeline, one layer too deep.
 		$this->objectService->saveObject(
-			object: $contactPerson->getObject(),
+			object: $object,
 			register: $contactPerson->getRegister(),
 			schema: $contactPerson->getSchema(),
 			uuid: $contactPerson->getUuid(),
 			silent: true,
 			_validation: false
 		);
 
-	}//end persistContactpersoonPatch()
+	}//end persistContactPersonPatch()
 
 	/**
 	 * Find a contactpersoon by username, falling back to a case-insensitive email search.

lib/Service/AanbodService.php

@@ -374,9 +374,12 @@ public function acceptAanbod(string $aanbodId, array $options = []): array {
 			);
 
 			// Save the updated object with RBAC and multitenancy disabled.
-			$existingAanbod->setObject($aanbodData);
+			// The PAYLOAD is what is sent — `setObject()` on the entity is not on
+			// the published ObjectEntityInterface (ADR-084) and the mutated copy
+			// was never read back anyway; `saveObject()` is PUT-semantic, so
+			// $aanbodData already carries every unchanged field forward.
 			$updatedAanbod = $objectService->saveObject(
-				object: $existingAanbod,
+				object: $aanbodData,
 				register: $existingAanbod->getRegister(),
 				schema: $existingAanbod->getSchema(),
 				uuid: $aanbodId,
@@ -593,12 +596,12 @@ private function resolvePartyId(mixed $partyInfo): ?string {
 	 * @param ObjectServiceInterface $objectService The OpenRegister object service
 	 * @param string $aanbodId The UUID of the aanbod object
 	 *
-	 * @return \OCA\OpenRegister\Db\ObjectEntity|null The found object or null
+	 * @return \OCA\OpenRegister\Contract\ObjectEntityInterface|null The found object or null
 	 */
 	private function findAanbodObject(
 		ObjectServiceInterface $objectService,
 		string $aanbodId,
-	): ?\OCA\OpenRegister\Db\ObjectEntity {
+	): ?\OCA\OpenRegister\Contract\ObjectEntityInterface {
 		$voorzieningenConfig = $this->settingsService->getVoorzieningenConfig();
 		$registerId = $voorzieningenConfig['register'] ?? null;
 

lib/Service/AangebodenGebruikService.php

@@ -992,9 +992,11 @@ public function setGebruikSelfToActiveOrg(string $gebruikId, array $options = []
 
 			// Save the updated object with RBAC and multitenancy disabled.
 			// Use register/schema from the found entity for correct table routing.
-			$existingGebruik->setObject($gebruikData);
+			// The PAYLOAD is what is sent — `setObject()` is not on the published
+			// ObjectEntityInterface (ADR-084), and `saveObject()` is PUT-semantic,
+			// so $gebruikData already carries every unchanged field forward.
 			$updatedGebruik = $objectService->saveObject(
-				object: $existingGebruik,
+				object: $gebruikData,
 				register: $existingGebruik->getRegister(),
 				schema: $existingGebruik->getSchema(),
 				uuid: $gebruikId,
@@ -1049,12 +1051,12 @@ public function setGebruikSelfToActiveOrg(string $gebruikId, array $options = []
 	 * @param ObjectServiceInterface $objectService The OpenRegister object service
 	 * @param string $objectId The UUID of the object to find
 	 *
-	 * @return \OCA\OpenRegister\Db\ObjectEntity|null The found object or null
+	 * @return \OCA\OpenRegister\Contract\ObjectEntityInterface|null The found object or null
 	 */
 	private function findGebruikOrIntegration(
 		ObjectServiceInterface $objectService,
 		string $objectId,
-	): ?\OCA\OpenRegister\Db\ObjectEntity {
+	): ?\OCA\OpenRegister\Contract\ObjectEntityInterface {
 		$voorzieningenConfig = $this->settingsService->getVoorzieningenConfig();
 		$registerId = $voorzieningenConfig['register'] ?? null;
 

lib/Service/ContactpersoonService.php

@@ -25,6 +25,7 @@
 use OCA\SoftwareCatalog\Service\SoftwareCatalogue\GroupHandler;
 use OCA\SoftwareCatalog\Service\SoftwareCatalogue\HierarchyHandler;
 use OCP\App\IAppManager;
+use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\IAppConfig;
 use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
@@ -1026,15 +1027,24 @@ public function getBulkUserInfo(array $contactPersonIds): array {
 					}
 
 					// Find the contactpersoon object with register and schema specified.
-					$contactObject = $objectService->findSilent(
-						id: $contactPersonId,
-						_extend: [],
-						files: false,
-						register: $contactRegister,
-						schema: $contactSchema
-					);
-
-					if ($contactObject === null) {
+					//
+					// A MISS RAISES, it does not return null: `findSilent()` is
+					// declared `ObjectEntityInterface` (non-nullable) on the
+					// published contract and OpenRegister lets the mapper's
+					// DoesNotExistException out. The `=== null` test this replaces
+					// could therefore never be true — the distinct "not found"
+					// entry below was unreachable, and every missing contactpersoon
+					// fell through to the generic error arm and came back carrying
+					// an `error` key instead.
+					try {
+						$contactObject = $objectService->findSilent(
+							id: $contactPersonId,
+							_extend: [],
+							files: false,
+							register: $contactRegister,
+							schema: $contactSchema
+						);
+					} catch (DoesNotExistException $e) {
 						$this->logger->warning(
 							'ContactpersoonService: Contactpersoon not found for bulk user info',
 							['contactpersoonId' => $contactPersonId]
@@ -1045,7 +1055,7 @@ public function getBulkUserInfo(array $contactPersonIds): array {
 							'groups' => [],
 						];
 						continue;
-					}
+					}//end try
 
 					$contactData = $contactObject->getObject();
 					$username = $contactData['username'] ?? null;

lib/Service/ContractApprovalService.php

@@ -38,7 +38,6 @@
 namespace OCA\SoftwareCatalog\Service;
 
 use OCA\OpenRegister\Contract\ObjectServiceInterface;
-use OCA\OpenRegister\Service\ObjectService;
 use OCP\EventDispatcher\IEventDispatcher;
 use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
@@ -501,7 +500,7 @@ private function buildSubjectLabel(array $data): string {
 	 *
 	 * @param string $contractUuid The contract uuid.
 	 *
-	 * @return \OCA\OpenRegister\Db\ObjectEntity|null The object, or null.
+	 * @return \OCA\OpenRegister\Contract\ObjectEntityInterface|null The object, or null.
 	 *
 	 * @spec openspec/specs/contract-decision-delegation/spec.md
 	 */
@@ -540,7 +539,7 @@ private function loadContract(string $contractUuid) {
 	/**
 	 * Persist a mutated contract object back to the OR store.
 	 *
-	 * @param \OCA\OpenRegister\Db\ObjectEntity $contract The contract entity.
+	 * @param \OCA\OpenRegister\Contract\ObjectEntityInterface $contract The contract entity.
 	 * @param array $data The mutated object data.
 	 *
 	 * @return void
@@ -572,8 +571,14 @@ private function persistContract($contract, array $data): void {
 	 */
 	private function getObjectService(): ?ObjectServiceInterface {
 		try {
-			$service = $this->container->get('OCA\OpenRegister\Service\ObjectService');
-			if ($service instanceof ObjectService) {
+			// Ask for the CONTRACT and narrow on the CONTRACT (ADR-084). Asking
+			// for the concrete class and gating on `instanceof ObjectService`
+			// made this method return null for anything that satisfies the
+			// published interface without being that exact class — which is
+			// every double a leaf app can build, so the ownership check silently
+			// fell through to its fail-closed arm and refused an owner.
+			$service = $this->container->get(ObjectServiceInterface::class);
+			if ($service instanceof ObjectServiceInterface) {
 				return $service;
 			}
 		} catch (\Throwable $e) {

lib/Service/ContractStatusService.php

@@ -28,7 +28,6 @@
 
 use DateTimeImmutable;
 use OCA\OpenRegister\Contract\ObjectServiceInterface;
-use OCA\OpenRegister\Service\ObjectService;
 use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 
@@ -189,8 +188,11 @@ public function expirePastContracts(?DateTimeImmutable $now = null): int {
 	 */
 	private function getObjectService(): ?ObjectServiceInterface {
 		try {
-			$service = $this->container->get('OCA\OpenRegister\Service\ObjectService');
-			if ($service instanceof ObjectService) {
+			// Ask for the CONTRACT and narrow on the CONTRACT (ADR-084) — see
+			// ContractApprovalService::getObjectService() for why gating on the
+			// concrete class is a silent fail-closed.
+			$service = $this->container->get(ObjectServiceInterface::class);
+			if ($service instanceof ObjectServiceInterface) {
 				return $service;
 			}
 		} catch (\Throwable $e) {