Add callback model#861
Conversation
There was a problem hiding this comment.
@devplayer0 Curious, why remove the vendor extension "x-callback-request"?
There was a problem hiding this comment.
@wing328 It's removed since isCallbackRequest is set on the operation just below this line - I was just using the vendor extension as a way of distinguishing between a normal operation and one that represents a callback request in fromOperation() (this extension is set to true in fromCallback() without changing the existing method signature.
There was a problem hiding this comment.
Please leave a coment in https://github.com/OpenAPITools/openapi-generator/blob/6544528df7238caeb559c57b0e5dcb3614063940/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L2671 about x-callback-request being removed later in the process.
I'll see I've a better to do it without using vendor extensions (to make it consistent with the rest of the program)
There was a problem hiding this comment.
@devplayer0 may I know why the operationId is not set if callback is defined in the spec?
There was a problem hiding this comment.
@wing328 My initial thought was that since it's not a server side method, callbacks aren't really "operations" on an API per se. In the OpenAPI example for callbacks operation IDs are not used when defining the callback request.
If I were to not set the operation ID to null, I feel like it would be important to change the behaviour of generating a default operation ID if the user does not provide one - the runtime expression parts should probably be removed and the callback identifier (onData in the OpenAPI example) should probably be used as a prefix. What do you think?
There was a problem hiding this comment.
In the OpenAPI example for callbacks operation IDs are not used when defining the callback request.
operationId (optional) is for uniquely identifying the operation:
Unique string used to identify the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is RECOMMENDED to follow common programming naming conventions.
In that example, there's only one operation so it's ok not to specify the operationId.
the runtime expression parts should probably be removed and the callback identifier (onData in the OpenAPI example) should probably be used as a prefix.
Perfectly fine with me that you want to change the default naming when the callback is specified. My experience told me that the default will not meet everyone's requirement so the users need to specify the "operationId" if they want a better operationId
|
@devplayer0 thanks for the PR. I've left some comments. Please review when you've time. |
This adds a new `CodegenCallback` class, a list of which is now present in `CodegenOperation`. `CodegenOperation` now also includes a `isCallbackRequest` boolean since `fromCallback()` (the method added to `DefaultCodegen` to process operations which contain OpenAPI callbacks) uses CodegenOperation as the model for a callback request. A `CodegenOperation` which represents a callback request will have a `null` operation id. A test is included for this new model.
devplayer0
force-pushed
the
add-callbacks
branch
from
6544528 to
7b6bdfc
Compare
|
@wing328 I have updated the PR to generate an |
|
@devplayer0 👍 can you add the following license header to the new file(s) introduced in this PR? |
|
@wing328 Done. |
|
@devplayer0 I'll run some tests with this PR tomorrow and let you know if I've further feedback. |
|
UPDATE: I ran some tests and the result looks good. I'll merge it tomorrow if no one has further feedback/question on this PR. |
|
@devplayer0 thanks for the PR, which has been included in the v3.2.3 release: https://twitter.com/oas_generator/status/1035200785066254336 |
|
@wing328 Great! |
* Add callback model (OpenAPITools#372) This adds a new `CodegenCallback` class, a list of which is now present in `CodegenOperation`. `CodegenOperation` now also includes a `isCallbackRequest` boolean since `fromCallback()` (the method added to `DefaultCodegen` to process operations which contain OpenAPI callbacks) uses CodegenOperation as the model for a callback request. A `CodegenOperation` which represents a callback request will have a `null` operation id. A test is included for this new model. * Generate callback request `operationId` * Add license to `CodegenCallback`
2 participants
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\.master,3.3.x,4.0.x. Default:master.Description of the PR
This adds a new
CodegenCallbackclass, a list of which is now present inCodegenOperation.CodegenOperationnow also includes aisCallbackRequestboolean sincefromCallback()(the method added toDefaultCodegento process callbacks in OpenAPI operations) uses CodegenOperation as the model for a callback request.A
CodegenOperationwhich represents a callback request will have anulloperation id.A test is included for this new model.
(See #372)