Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/JsonApi/JsonSchema/SchemaFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@ public function buildSchema(string $className, string $format = 'jsonapi', strin
unset($schema['type']);

$properties = $this->buildDefinitionPropertiesSchema($key, $className, $format, $type, $operation, $schema, []);
$properties['data']['properties']['attributes']['$ref'] = $prefix.$key;

$properties['data'] = [
'type' => 'array',
Expand Down Expand Up @@ -347,9 +346,8 @@ private function buildDefinitionPropertiesSchema(string $key, string $className,
$attributes[$propertyName] = $property;
}

$currentRef = $this->getSchemaUriPrefix($schema->getVersion()).$schema->getRootDefinitionKey();
$replacement = self::PROPERTY_PROPS;
$replacement['attributes'] = ['$ref' => $currentRef];
$replacement['attributes']['properties'] = $attributes;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reserved-name handling is incomplete. The attribute loop above (lines 341-346) only renames id_id, but the runtime ReservedAttributeNameConverter::JSON_API_RESERVED_ATTRIBUTES renames five names:

'id' => '_id', 'type' => '_type', 'links' => '_links',
'relationships' => '_relationships', 'included' => '_included',

The base JSON schema is built with the generic api_platform.name_converter (json_schema.php:31), not the reserved converter (which is wired only into the JSON:API normalizers, jsonapi.php:69), so a resource property literally named type (a common field name) arrives here as type and is emitted in the schema as attributes.properties.type. At runtime ItemNormalizer renames it to _type, so the documented key never matches the response — the same doc/response mismatch this PR is fixing for relations. Since the schema now enumerates attribute keys, this is observable.

Suggest sourcing the rename from the converter's map instead of hard-coding the id case, e.g.:

$name = ReservedAttributeNameConverter::JSON_API_RESERVED_ATTRIBUTES[$propertyName] ?? $propertyName;
$attributes[$name] = $property;

(and dropping the if ('id' === $propertyName) special case). A fixture with a property named type would also lock this down — the new test only exercises id.


$included = [];
if (\count($relationships) > 0) {
Expand Down
95 changes: 93 additions & 2 deletions src/JsonApi/Tests/JsonSchema/SchemaFactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@

use ApiPlatform\JsonApi\JsonSchema\SchemaFactory;
use ApiPlatform\JsonApi\Tests\Fixtures\Dummy;
use ApiPlatform\JsonApi\Tests\Fixtures\RelatedDummy;
use ApiPlatform\JsonSchema\DefinitionNameFactory;
use ApiPlatform\JsonSchema\Schema;
use ApiPlatform\JsonSchema\SchemaFactory as BaseSchemaFactory;
use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
Expand All @@ -31,7 +33,9 @@
use ApiPlatform\Metadata\Resource\ResourceMetadataCollection;
use ApiPlatform\Metadata\ResourceClassResolverInterface;
use PHPUnit\Framework\TestCase;
use Prophecy\Argument;
use Prophecy\PhpUnit\ProphecyTrait;
use Symfony\Component\TypeInfo\Type;

class SchemaFactoryTest extends TestCase
{
Expand Down Expand Up @@ -112,7 +116,8 @@ public function testHasRootDefinitionKeyBuildSchema(): void
'type' => 'string',
],
'attributes' => [
'$ref' => '#/definitions/Dummy',
'type' => 'object',
'properties' => [],
],
],
'required' => [
Expand Down Expand Up @@ -155,7 +160,8 @@ public function testSchemaTypeBuildSchema(): void
$this->assertArrayHasKey('data', $objectSchema['properties']);

$this->assertArrayHasKey('items', $objectSchema['properties']['data']);
$this->assertArrayHasKey('$ref', $objectSchema['properties']['data']['items']['properties']['attributes']);
$this->assertArrayNotHasKey('$ref', $objectSchema['properties']['data']['items']['properties']['attributes']);
$this->assertSame('object', $objectSchema['properties']['data']['items']['properties']['attributes']['type']);

$properties = $objectSchema['properties'];
$this->assertArrayHasKey('data', $properties);
Expand Down Expand Up @@ -199,4 +205,89 @@ public function testPostOutputSchemaRequiresId(): void
$data = $definitions[$rootDefinitionKey]['properties']['data'];
$this->assertSame(['type', 'id'], $data['required']);
}

public function testRelationIsExcludedFromAttributes(): void
{
$schemaFactory = $this->buildSchemaFactoryWithRelation();
$resultSchema = $schemaFactory->buildSchema(Dummy::class, 'jsonapi');

$definitions = $resultSchema->getDefinitions();
$rootDefinitionKey = $resultSchema->getRootDefinitionKey();
$dataProperties = $definitions[$rootDefinitionKey]['properties']['data']['properties'];

$this->assertArrayHasKey('attributes', $dataProperties);
$this->assertArrayNotHasKey('$ref', $dataProperties['attributes']);
$this->assertSame('object', $dataProperties['attributes']['type']);

$attributes = $dataProperties['attributes']['properties'];
$this->assertArrayHasKey('name', $attributes);
$this->assertArrayNotHasKey('relatedDummy', $attributes, 'relations must not be documented as attributes');

$this->assertArrayHasKey('_id', $attributes, 'id is exposed as _id in the JSON:API attributes');
$this->assertArrayNotHasKey('id', $attributes);

$this->assertArrayHasKey('relationships', $dataProperties);
$this->assertArrayHasKey('relatedDummy', $dataProperties['relationships']['properties']);
}

private function buildSchemaFactoryWithRelation(): SchemaFactory

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildSchemaFactoryWithRelation() re-declares ~50 lines of mock and factory wiring that setUp() already builds (the BaseSchemaFactory + SchemaFactory construction tail here is near-identical to setUp() lines 63-79, differing only in the extra resourceClassResolver arg and the multi-class mocks). A constructor-signature change to either factory (e.g. the recent definitionNameFactory addition) now has to be edited in two places, and the two stubs of propertyNameCollectionFactory->create(Dummy::class, ...) use different matchers (exact array in setUp, Argument::type('array') here). Extracting a shared builder that both setUp() and this test call with their data would remove the drift risk.

Note: setUp()'s BaseSchemaFactory omits resourceClassResolver while this one passes it — worth reconciling while consolidating.

{
$dummyOperation = (new Get())->withName('get')->withShortName('Dummy');
$relatedOperation = (new Get())->withName('get')->withShortName('RelatedDummy');

$resourceMetadataFactory = $this->prophesize(ResourceMetadataCollectionFactoryInterface::class);
$resourceMetadataFactory->create(Dummy::class)->willReturn(
new ResourceMetadataCollection(Dummy::class, [
(new ApiResource())->withOperations(new Operations(['get' => $dummyOperation])),
])
);
$resourceMetadataFactory->create(RelatedDummy::class)->willReturn(
new ResourceMetadataCollection(RelatedDummy::class, [
(new ApiResource())->withOperations(new Operations(['get' => $relatedOperation])),
])
);

$propertyNameCollectionFactory = $this->prophesize(PropertyNameCollectionFactoryInterface::class);
$propertyNameCollectionFactory->create(Dummy::class, Argument::type('array'))->willReturn(new PropertyNameCollection(['id', 'name', 'relatedDummy']));
$propertyNameCollectionFactory->create(RelatedDummy::class, Argument::type('array'))->willReturn(new PropertyNameCollection(['id', 'name']));

$propertyMetadataFactory = $this->prophesize(PropertyMetadataFactoryInterface::class);
$propertyMetadataFactory->create(Dummy::class, 'id', Argument::type('array'))->willReturn(
(new ApiProperty())->withNativeType(Type::int())->withReadable(true)->withSchema(['type' => 'integer'])
);
$propertyMetadataFactory->create(Dummy::class, 'name', Argument::type('array'))->willReturn(
(new ApiProperty())->withNativeType(Type::string())->withReadable(true)->withSchema(['type' => 'string'])
);
$propertyMetadataFactory->create(Dummy::class, 'relatedDummy', Argument::type('array'))->willReturn(
(new ApiProperty())->withNativeType(Type::object(RelatedDummy::class))->withReadable(true)->withSchema(['type' => Schema::UNKNOWN_TYPE])
);
$propertyMetadataFactory->create(RelatedDummy::class, 'id', Argument::type('array'))->willReturn(
(new ApiProperty())->withNativeType(Type::int())->withReadable(true)->withSchema(['type' => 'integer'])
);
$propertyMetadataFactory->create(RelatedDummy::class, 'name', Argument::type('array'))->willReturn(
(new ApiProperty())->withNativeType(Type::string())->withReadable(true)->withSchema(['type' => 'string'])
);

$resourceClassResolver = $this->prophesize(ResourceClassResolverInterface::class);
$resourceClassResolver->isResourceClass(Dummy::class)->willReturn(true);
$resourceClassResolver->isResourceClass(RelatedDummy::class)->willReturn(true);

$definitionNameFactory = new DefinitionNameFactory(null);

$baseSchemaFactory = new BaseSchemaFactory(
resourceMetadataFactory: $resourceMetadataFactory->reveal(),
propertyNameCollectionFactory: $propertyNameCollectionFactory->reveal(),
propertyMetadataFactory: $propertyMetadataFactory->reveal(),
resourceClassResolver: $resourceClassResolver->reveal(),
definitionNameFactory: $definitionNameFactory,
);

return new SchemaFactory(
schemaFactory: $baseSchemaFactory,
propertyMetadataFactory: $propertyMetadataFactory->reveal(),
resourceClassResolver: $resourceClassResolver->reveal(),
resourceMetadataFactory: $resourceMetadataFactory->reveal(),
definitionNameFactory: $definitionNameFactory,
);
}
}
37 changes: 14 additions & 23 deletions tests/Functional/JsonSchema/JsonApiJsonSchemaTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,28 +59,19 @@ public function testJsonApi(): void
{
$speciesSchema = $this->schemaFactory->buildSchema(Issue6317::class, 'jsonapi', Schema::TYPE_OUTPUT);
$this->assertEquals('#/definitions/Issue6317.jsonapi', $speciesSchema['$ref']);
$this->assertEquals([
'properties' => [
'data' => [
'type' => 'object',
'properties' => [
'id' => [
'type' => 'string',
],
'type' => [
'type' => 'string',
],
'attributes' => [
'$ref' => '#/definitions/Issue6317',
],
],
'required' => [
'type',
'id',
],
],
],
], $speciesSchema['definitions']['Issue6317.jsonapi']);
$data = $speciesSchema['definitions']['Issue6317.jsonapi']['properties']['data'];
$this->assertSame('object', $data['type']);
$this->assertSame(['type' => 'string'], $data['properties']['id']);
$this->assertSame(['type' => 'string'], $data['properties']['type']);
$this->assertSame(['type', 'id'], $data['required']);

$attributes = $data['properties']['attributes'];
$this->assertArrayNotHasKey('$ref', $attributes);
$this->assertSame('object', $attributes['type']);
$this->assertEqualsCanonicalizing(
['name', 'value', '_id', 'ordinal', 'cardinal'],
array_keys($attributes['properties'])
);
}

public function testJsonApiIncludesSchemaForQuestion(): void
Expand Down Expand Up @@ -150,7 +141,7 @@ public function testJsonApiIncludesSchemaForSpecies(): void
'type' => 'string',
],
'attributes' => [
'$ref' => '#/definitions/Species',
'type' => 'object',

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only assertion on the collection path's attributes (the path the production change at the top of buildSchema touches), and assertArraySubset only checks that the listed keys are present — it never verifies attributes.properties is populated and relation-excluded. A regression that produced empty or relation-containing attributes.properties on the collection path would still pass here.

Consider asserting the collection item's attributes.properties keys the way testJsonApi (item path) now does with assertEqualsCanonicalizing(...), so both paths have a positive content guard.

],
],
'required' => [
Expand Down
Loading