diff --git a/lib/Service/Configuration/ImportHandler.php b/lib/Service/Configuration/ImportHandler.php index fa3eab8590..fb7b934e7c 100644 --- a/lib/Service/Configuration/ImportHandler.php +++ b/lib/Service/Configuration/ImportHandler.php @@ -1490,11 +1490,34 @@ public function importSchema( } } + // 🔴 Both branches write to `items.$ref`. The second one used to + // write to `$property['$ref']` — the ARRAY's own ref — which did + // two wrong things at once: it left `items.$ref` as an unmapped + // slug, and it grafted a schema ID (an INT) onto the array + // property as a top-level `$ref`. + // + // That second effect is not cosmetic. `ValidateObject` strips a + // top-level `$ref` from string-typed properties and from + // `items`, but never from an array-typed property, so the int + // survived into the schema handed to Opis. Opis parses a + // property's subschema lazily — only when that property is + // PRESENT in the written data — and then throws + // `$ref must be a non-empty string` because an int is not a + // string. The message names neither the property nor the + // schema, so it reads like a broken register. + // + // Only reachable on the `schemasMap` fallback, i.e. when the + // referenced slug was not part of this import's own + // slug->id map. That is the FIRST install of a configuration + // whose cross-references resolve against already-present + // schemas — which is why it never showed on a long-lived + // instance and surfaced on every clean one (hermiq's `agent` + // schema: skillInstalls / contextRefs / delegationAllowlist). if (($property['items']['$ref'] ?? null) !== null) { if (($slugsAndIdsMap[$property['items']['$ref']] ?? null) !== null) { $property['items']['$ref'] = $slugsAndIdsMap[$property['items']['$ref']]; } else if (($this->schemasMap[$property['items']['$ref']] ?? null) !== null) { - $property['$ref'] = $this->schemasMap[$property['items']['$ref']]->getId(); + $property['items']['$ref'] = $this->schemasMap[$property['items']['$ref']]->getId(); } } diff --git a/lib/Service/Object/ValidateObject.php b/lib/Service/Object/ValidateObject.php index f174400016..2fa4758594 100644 --- a/lib/Service/Object/ValidateObject.php +++ b/lib/Service/Object/ValidateObject.php @@ -468,6 +468,27 @@ private function transformOpenRegisterObjectConfigurations(object $schemaObject) */ private function transformPropertyForOpenRegister(object $propertySchema): void { + // 🔴 First, drop any `$ref` that CANNOT be a JSON Schema reference. + // + // OpenRegister stores a `$ref` as a relation marker, and every branch + // below already removes it before the schema reaches Opis — but only for + // the shapes those branches recognise. A `$ref` that is an int, a null, + // an array or an empty string is not one of them, survives, and makes + // Opis throw `$ref must be a non-empty string` from + // RefKeywordParser::parse(). + // + // That exception fires LAZILY, when the offending property is present in + // the written data, so it names neither the property nor the schema and + // reads like a broken register rather than a stored schema defect. It is + // also unrecoverable for the caller: no payload shape fixes a schema. + // + // Registers imported before openregister#2321 already carry exactly this + // (an int `$ref` grafted onto an array property by ImportHandler), so + // this normalisation is what heals them without a migration. A VALID + // reference — a non-empty string — is untouched here and handled by the + // branches below exactly as before. + $this->dropUnusableRef(schema: $propertySchema); + // UUID pattern for related object references. $uuidPat = '^([a-z]+-)?([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9a-f]{32}|[0-9]+)$'; @@ -539,6 +560,9 @@ private function transformPropertyForOpenRegister(object $propertySchema): void if ($isArrayType === true && $hasItems === true && is_object($propertySchema->items) === true) { $itemsSchema = $propertySchema->items; + // Same normalisation as the property itself — see dropUnusableRef(). + $this->dropUnusableRef(schema: $itemsSchema); + // Handle inversedBy relationships for array items. // TODO: Move writeBack, removeAfterWriteBack, and inversedBy from items to config. if (($itemsSchema->inversedBy ?? null) !== null) { @@ -598,6 +622,37 @@ private function transformPropertyForOpenRegister(object $propertySchema): void } }//end transformPropertyForOpenRegister() + /** + * Remove a `$ref` that cannot be a JSON Schema reference. + * + * A JSON Schema `$ref` MUST be a non-empty string; Opis enforces that in + * `RefKeywordParser::parse()` and throws `$ref must be a non-empty string` + * for anything else. OpenRegister only ever uses `$ref` as a relation + * marker, never for validation-time resolution, so removing an unusable one + * loses nothing and turns an opaque 500 back into a normal validation pass. + * + * Valid refs (non-empty strings) are deliberately left alone — the + * surrounding transform branches own those. + * + * @param object $schema The property or items schema to normalise in place. + * + * @return void + */ + private function dropUnusableRef(object $schema): void + { + if (property_exists($schema, '$ref') === false) { + return; + } + + $ref = $schema->{'$ref'}; + if (is_string($ref) === true && $ref !== '') { + return; + } + + unset($schema->{'$ref'}); + + }//end dropUnusableRef() + /** * Transforms object properties based on OpenRegister object configuration. * diff --git a/tests/Unit/Service/Configuration/ImportHandlerCoverageTest.php b/tests/Unit/Service/Configuration/ImportHandlerCoverageTest.php index a514bf6056..12e9eb92f7 100644 --- a/tests/Unit/Service/Configuration/ImportHandlerCoverageTest.php +++ b/tests/Unit/Service/Configuration/ImportHandlerCoverageTest.php @@ -367,9 +367,22 @@ public function testGetDuplicateRegisterInfoShowsUnknownWhenNoCreatedDate(): voi // ========================================================================= /** - * importSchema resolves items.$ref from schemasMap. + * importSchema resolves items.$ref from schemasMap — INTO `items`, and + * without inventing a top-level `$ref` on the array property. + * + * 🔴 This used to assert only `instanceof Schema`, which is true whatever + * the branch writes, so it exercised the defect and reported green. The + * branch wrote the resolved id to `$property['$ref']` instead of + * `$property['items']['$ref']`, which left the items ref an unmapped slug + * AND grafted an INT `$ref` onto an array property. Opis rejects a + * non-string `$ref` with `$ref must be a non-empty string` the moment such + * a property appears in a written object — the 500 hermiq's clean-install + * e2e hit on `PUT /api/agents/{id}/tool-grants`. + * + * The assertions are on the payload handed to the mapper, because that is + * what gets persisted. */ - public function testImportSchemaResolvesItemsRefFromSchemasMap(): void + public function testImportSchemaResolvesItemsRefIntoItemsAndNotOntoTheProperty(): void { $refSchema = $this->makeSchema(42, 'related-schema'); $this->setProperty($this->handler, 'schemasMap', ['related-schema' => $refSchema]); @@ -388,19 +401,40 @@ public function testImportSchemaResolvesItemsRefFromSchemasMap(): void ]; $createdSchema = $this->makeSchema(100, 'items-ref-schema'); + $persisted = null; $this->schemaMapper->method('find') ->willThrowException(new \OCP\AppFramework\Db\DoesNotExistException('')); $this->schemaMapper->method('createFromArray') - ->willReturn($createdSchema); + ->willReturnCallback( + function (array $payload) use (&$persisted, $createdSchema): Schema { + $persisted = $payload; + return $createdSchema; + } + ); $this->schemaMapper->method('update') ->willReturnArgument(0); $result = $this->handler->importSchema($data, [], null, null, '1.0.0'); $this->assertInstanceOf(Schema::class, $result); + $this->assertIsArray($persisted, 'the schema payload must reach the mapper'); + + $property = $persisted['properties']['items_prop']; + + $this->assertSame( + 42, + $property['items']['$ref'], + 'the items $ref must be resolved to the referenced schema id' + ); + $this->assertArrayNotHasKey( + '$ref', + $property, + 'an ARRAY property must never gain a top-level $ref from its items — ' + .'a non-string $ref makes Opis throw "$ref must be a non-empty string"' + ); - }//end testImportSchemaResolvesItemsRefFromSchemasMap() + }//end testImportSchemaResolvesItemsRefIntoItemsAndNotOntoTheProperty() // ========================================================================= diff --git a/tests/Unit/Service/Object/ValidateObjectUnusableRefTest.php b/tests/Unit/Service/Object/ValidateObjectUnusableRefTest.php new file mode 100644 index 0000000000..3ac1acab14 --- /dev/null +++ b/tests/Unit/Service/Object/ValidateObjectUnusableRefTest.php @@ -0,0 +1,218 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + */ + +namespace OCA\OpenRegister\Tests\Unit\Service\Object; + +use OCA\OpenRegister\Db\MagicMapper; +use OCA\OpenRegister\Db\Schema; +use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Service\Object\ValidateObject; +use OCP\IAppConfig; +use OCP\IURLGenerator; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * Locks the behaviour of an unusable `$ref` on a stored schema. + */ +class ValidateObjectUnusableRefTest extends TestCase +{ + + /** + * The subject under test. + * + * @var ValidateObject + */ + private ValidateObject $handler; + + + /** + * Build a ValidateObject with mocked collaborators. + * + * @return void + */ + protected function setUp(): void + { + parent::setUp(); + + $urlGenerator = $this->createMock(IURLGenerator::class); + $urlGenerator->method('getBaseUrl')->willReturn('http://localhost:8080'); + + $this->handler = new ValidateObject( + $this->createMock(IAppConfig::class), + $this->createMock(MagicMapper::class), + $this->createMock(SchemaMapper::class), + $urlGenerator, + $this->createMock(LoggerInterface::class) + ); + + }//end setUp() + + + /** + * A Schema entity with the given slug. + * + * @param string $slug Schema slug. + * + * @return Schema + */ + private function schema(string $slug='agent'): Schema + { + $schema = new Schema(); + $schema->setSlug($slug); + $schema->setTitle('Agent'); + return $schema; + + }//end schema() + + + /** + * A schema object with one array property carrying the given top-level + * `$ref` — the exact shape a pre-fix import left behind. + * + * @param mixed $ref The stored `$ref` value. + * + * @return object + */ + private function schemaWithPropertyRef(mixed $ref): object + { + return json_decode( + json_encode( + [ + 'type' => 'object', + 'properties' => [ + 'delegationAllowlist' => [ + 'type' => 'array', + '$ref' => $ref, + 'items' => [ + 'type' => 'string', + 'format' => 'uuid', + ], + ], + ], + ] + ) + ); + + }//end schemaWithPropertyRef() + + + /** + * An INT `$ref` — the ImportHandler shape — validates instead of throwing. + * + * The property must be PRESENT in the object: Opis parses a property's + * subschema lazily, which is why this endpoint looked healthy for months + * on instances whose optional relation arrays were never written. + * + * @return void + */ + public function testIntegerRefOnAnArrayPropertyDoesNotThrow(): void + { + $object = ['delegationAllowlist' => ['550e8400-e29b-41d4-a716-446655440000']]; + + $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef(4365)); + + $this->assertTrue( + $result->isValid(), + 'an int $ref must be dropped, not handed to Opis as a JSON Schema reference' + ); + + }//end testIntegerRefOnAnArrayPropertyDoesNotThrow() + + + /** + * An EMPTY-STRING `$ref` is equally unusable and equally dropped. + * + * @return void + */ + public function testEmptyStringRefDoesNotThrow(): void + { + $object = ['delegationAllowlist' => ['11111111-1111-1111-1111-111111111111']]; + + $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef('')); + + $this->assertTrue($result->isValid(), 'an empty-string $ref must be dropped'); + + }//end testEmptyStringRefDoesNotThrow() + + + /** + * A NULL `$ref` is dropped too — and a null property value, which survives + * OpenRegister's own empty-value filter, is what actually reaches Opis. + * + * @return void + */ + public function testNullRefWithANullValueDoesNotThrow(): void + { + $object = ['delegationAllowlist' => null]; + + $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef(null)); + + $this->assertTrue($result->isValid(), 'a null $ref must be dropped'); + + }//end testNullRefWithANullValueDoesNotThrow() + + + /** + * 🔑 NEGATIVE CONTROL. Dropping the unusable `$ref` must not switch + * validation off for the property — a value of the wrong TYPE still fails. + * + * Without this, all three tests above would pass just as well if the fix + * had removed the property from validation altogether. + * + * @return void + */ + public function testDroppingTheRefStillLeavesTheTypeEnforced(): void + { + $object = ['delegationAllowlist' => 'not an array at all']; + + $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef(4365)); + + $this->assertFalse( + $result->isValid(), + 'a string where the schema declares an array must still fail validation' + ); + + }//end testDroppingTheRefStillLeavesTheTypeEnforced() + + +}//end class