feat(jsonschema): make JSON-LD specific properties required only in the schema for output

[ttskch]

Q	A
Branch?	3.4
Tickets	#6484
License	MIT
Doc PR	N/A

In JSON Schema for JSON-LD, following things are the truth:

• JSON-LD specific properties such as @context @id @type can be included in the request body. (See fix(jsonld): allow @id, @context and @type on denormalization 2 #6451)
• JSON-LD specific properties such as @context @id @type always have a value in the response body. (In other words, they are required.)

Therefore, this PR will make the following fixes:

• Make JSON-LD specific properties such as @context @id @type non-readOnly.
• Separate the JSON-LD schema for input and output.
• Only in the schema for output, make JSON-LD specific properties such as @context @id @type required.

Below is a simple example.

#[ORM\Entity(repositoryClass: TodoRepository::class)]
#[ApiResource]
class Todo
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column]
    private ?int $id = null;

    #[ORM\Column(length: 255)]
    #[Assert\NotBlank]
    #[Assert\Length(max: 255)]
    private ?string $title = null;

    #[ORM\Column(type: Types::TEXT, nullable: true)]
    private ?string $description = null;

    #[ORM\Column]
    #[Assert\NotNull]
    private ?bool $done = null;

Currently, the following schema is generated for the above API resource.

This schema is clearly incorrect because:

• JSON-LD specific properties such as @context @id @type can be included in the request body. (See fix(jsonld): allow @id, @context and @type on denormalization 2 #6451) So the must not be readOnly.
• JSON-LD specific properties such as @context @id @type always have a value in the response body. (In other words, they are required in the output context.)

JSON-LD specific properties such as @context @id @type should be output as required in the OpenAPI document. However, since the above schema is shared by both the request body and response body, if we set these properties to required, they will also be required in the request body.

The fundamental problem is that the above schema is shared by both the request body and response body. In the first place, in the current implementation, even though ApiPlatform\Hydra\JsonSchema\SchemaFactory does not add JSON-LD specific properties in the input context, the schemas for input and output are generated with the same key, so the same schema is shared between the input and output contexts. This is clearly a bug.

See:

• core/src/OpenApi/Factory/OpenApiFactory.php

  Lines 265 to 267 in 953ec19

  	$operationOutputSchema = $this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_OUTPUT, $operation, $schema, null, $forceSchemaCollection);
  	$operationOutputSchemas[$operationFormat] = $operationOutputSchema;
  	$this->appendSchemaDefinitions($schemas, $operationOutputSchema->getDefinitions());

• core/src/OpenApi/Factory/OpenApiFactory.php

  Lines 415 to 417 in 953ec19

  	$operationInputSchema = $this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_INPUT, $operation, $schema, null, $forceSchemaCollection);
  	$operationInputSchemas[$operationFormat] = $operationInputSchema;
  	$this->appendSchemaDefinitions($schemas, $operationInputSchema->getDefinitions());

Therefore, in this PR, I first modified the schemas for input and output to be generated with different keys.

And I also made JSON-LD specific properties such as @context @id @type non-readOnly (because they can be included in the request body) and made those properties required only in the schema for output (if those properties exist).

This ensures that JSON-LD specific properties are not required in the request body, and those properties are marked as required in the response body.

This has the advantage that the type information of API clients automatically generated by OpenAPI TypeScript etc. will be more precise.

  export interface operations {
      api_todos_post: {
          requestBody: {
-             "application/ld+json": components["schemas"]["Todo.jsonld"];
+             "application/ld+json": components["schemas"]["Todo.jsonld.input"];
          };
          responses: {
              201: {
                  content: {
-                     "application/ld+json": compoennts["schemas"]["Todo.jsonld"];
+                     "application/ld+json": compoennts["schemas"]["Todo.jsonld.output"];
                  };
              };
          };
      };
  };
  
  export interface components {
      schemas: {
+         "Todo.jsonld.input": {
+             "@context"?: string | {
+                 "@vocab": string;
+                 /** @enum {string} */
+                 hydra: "http://www.w3.org/ns/hydra/core#";
+                 [key: string]: unknown;
+             };
+             "@id"?: string;
+             "@type"?: string;
+             readonly id?: number;
+             title: string;
+             description?: string | null;
+             done: boolean;
+         };
-         "Todo.jsonld": {
+         "Todo.jsonld.output": {
-             readonly "@context"?: string | {
+             "@context": string | {
                  "@vocab": string;
                  /** @enum {string} */
                  hydra: "http://www.w3.org/ns/hydra/core#";
                  [key: string]: unknown;
              };
-             readonly "@id"?: string;
+             "@id": string;
-             readonly "@type"?: string;
+             "@type": string;
              readonly id?: number;
              title: string;
              description?: string | null;
              done: boolean;
          };
      };
  };

[ttskch]

@soyuka /cc @paullallier We still need to discuss the following points:

• Which of @context, @id and @type are required and which are not required in the output schema
• About compatibility with #[ApiProperty(genId: false)] (See 3.3.10 [BC break?]: Entities with embedded non-resource subentities fail schema checks after #6366 #6483)

[paullallier]

Thanks @ttskch.

I'm not going to claim to know much about the strict requirements of the OpenApi spec and I'm primarily interested in having a functional API for my project. I suspect the previous iteration of the fix actually didn't break my API itself, just the assertMatchesResourceItemJsonSchema assertion - though I'd want that still to work since it's important to me that my CI tests pass (and I don't really want to remove the check entirely).

That's not to say we shouldn't bother trying to be consistent with the OpenApi spec.

I think you're likely to be correct that, generally, for an entity with an embedded array of subresources (say a "Course" entity was an array of "Student" entities enrolled on that course) - the subresources should have an @id, because you can get the details of a single student. Probably should have an @context too, though I don't know whether it MUST.

My use case is different from this though. For example, I have an "Invoice" entity with an array of "InvoiceLine" entities embedded into it. In that case, it's not desirable to me that someone should be able to GET one of the InvoiceLine entities in isolation from the Invoice and I haven't made an endpoint to get them. I'm using genId: false to suppress generation of a skolem IRI. I haven't looked at the history, but I suspect that's probably why that genId parameter exists. In this case, the subresource isn't really a resource, even though - internally - it's an entity object.

A side note, I did also get some failures when checking the schema on errors (which might have been a mistake on my part anyway - but it was passing before). We should potentially check that the problem objects are correctly including @context if that's really needed.

[ttskch]

Thank you for the detailed explanation @paullallier. /cc @soyuka

I've created a simple reproduction environment for this issue below.

https://github.com/ttskch/api-platform-core-6485

• There are two entities of Todo "resource" entity and Subtask "non-resource" entity.
• Todo has a collection of Subtask

In this situation, I created a simple test using assertMatchesResourceItemJsonSchema() method.

With a42ca67, this test fails as you say.

But after applying the fix in 23af961, this test passes.

From my understanding, in the schema for sub-resources, @context is always absent, and @id is removed when genId: false is set, so only @type is required for sub-resources.

In SchemaFactory::buildSchema(), whether the current schema is a sub-schema can be determined by whether the 4th argument is null, so I think this is a reasonable fix.

What do you guys think? Thanks.

[soyuka]

Thanks for your work @ttskch! I was on holiday and I'll take time to see how everything fits here, I'll come back to that asap.

[soyuka]

So, the main issue here is that we multiply the JSON Schemas for a given resource which is really heavy work for no much gain. I don't have much time (as 3.4/4.0 release is planned for September I have a lot of work right now), but we need to read the JSON schema specification (https://json-schema.org/draft/2020-12/json-schema-core) as I'm sure that there's a way to reference the JSON-LD specific fields. If there's none, I think that it's acceptable to have optional properties for special fields (@id, @type, @context etc.) eventhough they're required on output. WDYT @vincentchalamon ?

[soyuka]

We need to use $id to have base schemas that each resource schema would extend: https://json-schema.org/draft/2020-12/json-schema-core#id-keyword. This is not so easy to implement but I'll willing to give a shot (after the API Platform con is done ^^).

[soyuka]

should be fixed in 4.2