-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Add callback model #861
Add callback model #861
Conversation
|
||
if (operation == null) | ||
throw new RuntimeException("operation cannot be null in fromOperation"); | ||
Object isCallbackRequest = op.vendorExtensions.remove("x-callback-request"); |
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.
@devplayer0 Curious, why remove the vendor extension "x-callback-request"?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
} | ||
operationId = removeNonNameElementToCamelCase(operationId); | ||
op.operationId = op.isCallbackRequest ? null : toOperationId(operationId); |
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.
@devplayer0 may I know why the operationId
is not set if callback is defined in the spec?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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`
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if 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
CodegenCallback
class, a list of which is now present inCodegenOperation
.CodegenOperation
now also includes aisCallbackRequest
boolean sincefromCallback()
(the method added toDefaultCodegen
to process callbacks in OpenAPI operations) uses CodegenOperation as the model for a callback request.A
CodegenOperation
which represents a callback request will have anull
operation id.A test is included for this new model.
(See #372)