Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add callback model #861

Merged
merged 3 commits into from
Aug 28, 2018
Merged

Add callback model #861

merged 3 commits into from
Aug 28, 2018

Conversation

devplayer0
Copy link
Contributor

@devplayer0 devplayer0 commented Aug 21, 2018 •
edited
Loading

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./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\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

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 callbacks in OpenAPI operations) 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.

(See #372)

@devplayer0 devplayer0 mentioned this pull request Aug 21, 2018
Open
wing328
wing328 reviewed Aug 22, 2018
View reviewed changes
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

if (operation == null)
throw new RuntimeException("operation cannot be null in fromOperation");
Object isCallbackRequest = op.vendorExtensions.remove("x-callback-request");
Copy link
Member

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"?

Copy link
Contributor Author

@devplayer0 devplayer0 Aug 22, 2018 •
edited
Loading

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.

Copy link
Member

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)

wing328
wing328 reviewed Aug 22, 2018
View reviewed changes
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java Outdated
}
operationId = removeNonNameElementToCamelCase(operationId);
op.operationId = op.isCallbackRequest ? null : toOperationId(operationId);
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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

@wing328
Copy link
Member

wing328 commented Aug 22, 2018

@devplayer0 thanks for the PR. I've left some comments. Please review when you've time.

@devplayer0
Add callback model (OpenAPITools#372)
3701e8d 
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
Generate callback request operationId
7b6bdfc
@devplayer0
Copy link
Contributor Author

@wing328 I have updated the PR to generate an operationId for callback requests and added that note.

@wing328
Copy link
Member

wing328 commented Aug 23, 2018

@devplayer0 👍 can you add the following license header to the new file(s) introduced in this PR?

/*
 * Copyright 2018 OpenAPI-Generator Contributors (https://openapi-generator.tech)
 * Copyright 2018 SmartBear Software
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

@devplayer0
Copy link
Contributor Author

@wing328 Done.

@wing328 wing328 added this to the 3.2.3 milestone Aug 23, 2018
@wing328
Copy link
Member

wing328 commented Aug 23, 2018

@devplayer0 I'll run some tests with this PR tomorrow and let you know if I've further feedback.

@wing328
Copy link
Member

wing328 commented Aug 26, 2018

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.

@wing328 wing328 merged commit 5926ee5 into OpenAPITools:master Aug 28, 2018
@devplayer0 devplayer0 deleted the add-callbacks branch August 28, 2018 13:10
@wing328
Copy link
Member

wing328 commented Aug 30, 2018

@devplayer0 thanks for the PR, which has been included in the v3.2.3 release: https://twitter.com/oas_generator/status/1035200785066254336

@devplayer0
Copy link
Contributor Author

@wing328 Great!

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@devplayer0 @wing328
Add callback model (OpenAPITools#861)
7758e75 
* 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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wing328 wing328 wing328 left review comments

Assignees
No one assigned
Labels
Enhancement: Feature Feature: OAS 3.0 spec support
Projects
None yet
Milestone
3.2.3
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@devplayer0 @wing328