PATCH schema lost all required properties

[thomas-hiron]

API Platform version(s) affected: 4.2.2

Description
Since 4.2.2, my openapi specs changed due to this fix: #6394
There a two issues:

• the definition name is now postfixed by .jsonMergePath and OpenApi Generator generates new object names. I have to refactor many objects.
• all my PATCH input definition lost the required properties.

About the required properties, I have PATCH operations on an entity with a different input that has required fields.
Due to this new condition, I understand I won't be able to have a required field in my schema anymore?
How can I get them back?
Should this be a PUT? I'm not sure which is the best here.

How to reproduce

#[ApiResource(
    operations: [
        new Patch(
            uriTemplate: '/my-entities/{uuid}/custom-action',
            input: CustomInput::class,
            processor: CustomInputProcessor::class,
        ),
    ]
)]
#[ORM\Entity]
class MyEntity
{}

class CustomInput
{
    #[Assert\NotBlank]
    public ?string $myProperty = null;
}

Before:

{
    "CustomInput": {
        "type": "object",
        "required": [
            "my_property",
        ],
        "properties": {
            "my_propery": {
                "type": [
                    "string",
                    "null"
                ]
            },
        }
    }
}

After:

{
    "CustomInput.jsonMergePath": {
        "type": "object",
        "properties": {
            "my_propery": {
                "type": [
                    "string",
                    "null"
                ]
            },
        }
    }
}

Possible Solution

Additional Context

[ttskch]

@thomas-hiron The request body format for the PATCH operation is JSON Merge Patch (application/merge-patch+json), and according to the specifications, required fields are not allowed in the request body for JSON Merge Patch. In your case, I think using PUT would be a more appropriate approach.

[thomas-hiron]

Thanks, that's what we did indeed!

About the new postfix .jsonMergePath, that's still quite bothering. We have a custom DefinitionNameFactory, so we can have simplier names than original DefinitionNameFactory (without prefix and groups), but the postfix is added in SchemaFactory.
Shouldn't the postfix be added in the DefinitionNameFactory so people can benefit from it, or not?

With OpenApi Generator, many models have been renamed due to this postfix, and we had to create a hacky decorator to remove the postfix from definitions and references.

    private function updateJsonMergePatch(mixed &$docs): void
    {
        if (!is_array($docs)) {
            return;
        }

        foreach ($docs as $key => &$doc) {
            /* Key contains .jsonMergePatch, remove it to keep old name */
            if (str_ends_with($key, '.jsonMergePatch')) {
                $keyWithoutPostfix = str_replace('.jsonMergePatch', '', $key);

                /* Copy definition without postfix */
                $docs[$keyWithoutPostfix] = $docs[$key];

                /* Then remove old definition */
                unset($docs[$key]);

                continue;
            }

            /* Recursive handling to find references */
            if (is_array($doc)) {
                $this->updateJsonMergePatch($doc);

                continue;
            }

            /* Found ref, rename it */
            if (is_string($doc) && str_ends_with($doc, '.jsonMergePatch')) {
                $doc = str_replace('.jsonMergePatch', '', $doc);
            }
        }
    }

Or maybe a configuration on the operation addJsonMergePatchPostfix defaulting to true?

[ttskch]

@thomas-hiron

I think it's a good idea to add the suffix using DefinitionNameFactory! Would it be possible for you to create a pull request?

With OpenApi Generator, many models have been renamed due to this postfix, and we had to create a hacky decorator to remove the postfix from definitions and references.

Simply replacing ".jsonMergePatch" with "" would overwrite the schema for JSON format (including required fields) with the shema for JSON Merge Patch format (which does not include required fields), resulting in an incorrect OpenAPI document, wouldn't it?

[thomas-hiron]

Yes, I guess it would.
To address that issue, we have another hack, that will be removed when migrating to DTO resources!

About the PR, why not, but I'm not sure about the last condition:

Schema::TYPE_INPUT === $type

The DefinitionNameFactory hasn't access to this info, and I don't know how to safely check if the name is being generated for the input definition.
Maybe $inputOrOutputClass being different from $operation->getOutput()? WDYT?

[ttskch]

@thomas-hiron Instead of modifying DefinitionNameFactory itself, it seems the correct approach is to pass jsonMergePatch as the $format argument to DefinitionNameFactory::create(). (The .jsonld schema is generated in this way.)

[thomas-hiron]

That would be a BC break for those with a custom DefinitionNameFactory who rely on the json format, right?
Other than that, this should be an easy update 👌

[ttskch]

No BC break if the custom DefinitionNameFactory correctly determines whether the value of the $format argument is 'json', I think.

[thomas-hiron]

If someone's custom DefinitionNameFactory relied on $format === 'json', now they might receive merge-patch+json or something instead, and their schema definition would change!
But isn't it weird to update the format on the fly to merge-patch+json?

Maybe this should be defined by a new $serializerContext key instead? No BC, and we can safely move the postfix to the DefinitionNameFactory

[ttskch]

Hmm, the DefinitionNameFactory is a module that is expected to receive an arbitrary format from the outside via the $format argument. Therefore, creating a custom DefinitionNameFactory that assumes $format === 'json' is completely wrong, and I believe there is no need to protect against this incorrect custom DefinitionNameFactory ceasing to function as a breaking change.

[tecbird]

We ran also in this problem and we now fix it via decorating the "DefinitionNameFactory".
Thanks for this PR!

We currently allow both patch formats "json" and "merge-patch+json" for our endpoints.
Sadly the SchemaFactory always change the format from "json" to "merge-patch+json" so we can not differ the condition on exactly the matching format.

Code: https://github.com/api-platform/core/pull/7510/files#diff-b97f49d72691e032d88b06dda3c9680f93cb90327b80f511560241f048dc6e27R94

Somebody any ideas to fix it only if patch format is set to "application/json" and not for "application/merge-patch+json"

[ttskch]

@tecbird Is it possible to specify "json" as patch_format?
https://api-platform.com/docs/core/content-negotiation/#configuring-patch-formats

[soyuka]

Can you open a new issue? maybe that we need to add something to the context inside the definitionNameFactory?