-
-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(jsonschema): make JSON-LD specific properties required #6366
feat(jsonschema): make JSON-LD specific properties required #6366
Conversation
ff80dcf
to
eecfe29
Compare
e45097b
to
3535a47
Compare
Thank you for your review. I've accepted your suggestion and fix the code. However, I noticed something and am starting to question this PR fix. I would like to hear your opinion. Originally, the readOnly property should be automatically removed from the request body schema.Swagger's OpenAPI Guide (based on OAS 3.0) states:
And, OAS 3.0 states:
Thus, at least in OAS 3.0, the readOnly property should be automatically removed from the request body schema. In fact, if you look closely at the output of the Swagger UI (the version we're using in API Platform 3.3), you'll see that the readOnly property has been removed from the request body schema. However, in OAS 3.1, there is no such description regarding the readOnly property. In OAS 3.0, the This seems like a problem with OAS itself, but I couldn't find any issues or pull requests regarding this in the OAS repository. If you believe that the readOnly property should be automatically removed from the request body schema even in OAS 3.1, all that API Platform's SchemaFactory should do is simply make the JSON-LD specific property required. Because those properties are readOnly originally. About the implementation of major code generatorsWhen automatically generating TypeScript code from the sample resources in this PR using OpenAPI Generator (typescript-axios), OpenAPI Generator (typescript-fetch), and OpenAPI TypeScript, the following code was generated. OpenAPI Generator (typescript-axios)The readOnly properties are included in the request body schema. /**
*
* @export
* @interface TodoJsonld
*/
export interface TodoJsonld {
/**
*
* @type {TodoJsonldContext}
* @memberof TodoJsonld
*/
'@context'?: TodoJsonldContext;
/**
*
* @type {string}
* @memberof TodoJsonld
*/
'@id'?: string;
/**
*
* @type {string}
* @memberof TodoJsonld
*/
'@type'?: string;
/**
*
* @type {number}
* @memberof TodoJsonld
*/
'id'?: number;
/**
*
* @type {string}
* @memberof TodoJsonld
*/
'title': string;
/**
*
* @type {string}
* @memberof TodoJsonld
*/
'description'?: string | null;
/**
*
* @type {boolean}
* @memberof TodoJsonld
*/
'done': boolean;
}
export const TodoApiAxiosParamCreator = function (configuration?: Configuration) {
return {
// ...
/**
* Creates a Todo resource.
* @summary Creates a Todo resource.
* @param {TodoJsonld} todoJsonld The new Todo resource
* @param {*} [options] Override http request option.
* @throws {RequiredError}
*/
apiTodosPost: async (todoJsonld: TodoJsonld, options: RawAxiosRequestConfig = {}): Promise<RequestArgs> => {
// ...
},
}
} OpenAPI Generator (typescript-fetch)The readOnly properties are removed from the request body schema. (However, for JSON-LD specific properties it doesn't work fully due to this issue for now.) export interface ApiTodosGetCollectionRequest {
page?: number;
}
export interface ApiTodosIdDeleteRequest {
id: string;
}
export interface ApiTodosIdGetRequest {
id: string;
}
export interface ApiTodosIdPatchRequest {
id: string;
todo: Omit<Todo, 'id'>;
}
export interface ApiTodosIdPutRequest {
id: string;
todoJsonld: Omit<TodoJsonld, '@id'|'@type'|'id'>;
}
export interface ApiTodosPostRequest {
todoJsonld: Omit<TodoJsonld, '@id'|'@type'|'id'>;
} OpenAPI TypeScriptAs mentioned in this PR description, the readOnly property is included in the request body schema. The readOnly properties are included in the request body schema. What should API Platform do?Although the issue of unclear handling of the readOnly property in OAS 3.1 remains, the basic idea is that the readOnly property should be removed from the request body schema. If so, all API Platform's SchemaFactory needs to do is simply make the JSON-LD specific properties required. However, doing this would create backwards compatibility issues for API Platform users currently using code generators such as OpenAPI Generator (typescript-axios) and OpenAPI TypeScript. This is because API clients generated by these "misimplemented" code generators will cause type errors if they do not include JSON-LD specific properties in the request body schema. If we're going to strictly follow OAS, we should just make the JSON-LD specific properties required, but if we're concerned about practical backward compatibility issues, it may be safer to explicitly separate the schema between input and output, as this PR currently proposes. What do you think about this issue? Regards. |
$definitions = $resultSchema->getDefinitions(); | ||
$itemsDefinitionKey = array_key_first($definitions->getArrayCopy()); | ||
|
||
// @noRector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this noRector attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that If |
As mentioned I see that it's impossible to manage existing subobjects like in this example PHPUnit test (commit) ( So if you want to make them mandatory it is necessary to separate at least the |
…th different keys
Co-authored-by: Antoine Bluchet <soyuka@users.noreply.github.com>
9cc9c63
to
d9917ed
Compare
d9917ed
to
016ccac
Compare
…ired only in output schema
4a2933f
to
7a32d6a
Compare
@soyuka With the merge of #6451 into 3.3, it is now the correct specification that the request body includes To organize the current situation, there are two points that we believe are correct:
Therefore, this PR modifies SchemaFactory as follows:
This change should not cause any BC Breaks for users. Regards. |
Thanks @ttskch ! |
This PR modifies SchemaFactory for JSON-LD to output JSON-LD specific properties such as
@context
@id
@type
as REQUIRED.Below is a simple example.
Currently, the following schema is generated for the above API resource.
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.In the first place, even though
ApiPlatform\Hydra\JsonSchema\SchemaFactory
does not add JSON-LD specific properties in theinput
context, the schemas for input and output are generated with the same key, so in the OpenAPI document, properties such as@context
@id
@type
are output even in POST/PUT/PATCH operations. This is the fundamental problem.Therefore, in this PR, I first modified the schemas for input and output to be generated with different keys, so that JSON-LD specific properties would not be output during POST/PUT/PATCH operations.
And I made sure to always output JSON-LD specific properties such as
@context
@id
@type
as required (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.