-
Couldn't load subscription status.
- Fork 219
Openapi validator #6733
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
base: master
Are you sure you want to change the base?
Openapi validator #6733
Conversation
Signed-off-by: Ioannis Bonatakis <[email protected]>
define a spec file for openapi and configure the plugin in the server. it requires . The commit shows a manual validation of the inputs. Signed-off-by: Ioannis Bonatakis <[email protected]>
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.
Ok, good :)
Now please put yourself in the shoes of the person picking up the followup ticket and looking at all the commented out stuff and also the not commented out stuff that is not needed.
A proof of concept doesn't need to pass all tests, and it can also have commented out parts, but please clean up the not needed parts that don't serve any purpose for what we want. I think it will look much cleaner then, and then we can concentrate on the things that are left.
See individual comments.
| #$self->openapi->valid_input or return; | ||
| #my $validation = $self->openapi->validator; |
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 remove
| =cut | ||
|
|
||
| sub show ($self) { | ||
| # manually triggers validation - unclear if this is how it should be |
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.
You can remove the comment. That's how it should work.
| $job = $job->to_hash(assets => 1, check_assets => $check_assets, deps => 1, details => $details, parent_group => 1); | ||
| $job->{followed_id} = $job_id if ($job_id != $job->{id}); | ||
| $self->render(json => {job => $job}); | ||
| $self->render(openapi => {job => $job}); |
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.
With this you are telling the plugin to validate the response.
We said that we don't want this for now (we can always add it later).
If you remove this change, you can also remove the responses parts from the yaml file.
|
|
||
| sub prio ($self) { | ||
|
|
||
| # seems this is the default behavior |
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.
?
| #my $v = $self->validation; | ||
| #my $prio = $v->required('prio')->num->param; |
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 remove
| operationId: apiv1_job | ||
| x-mojo-name: apiv1_job |
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 think we need only one of those two
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.
As far as I can tell, operationId is not required. is not one or the other. operationId can be used for some more advanced stuff (like links), but it has to be unique. here I use the same as x-mojo-name but I think it is not unique in WebAPI.pm
| responses: | ||
| '200': | ||
| description: A successful response. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| jobs: | ||
| type: array | ||
| items: {} | ||
| '404': | ||
| description: 'Not Found.' | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| example: 'Scheduled product to clone settings from not found.' | ||
| properties: | ||
| error: | ||
| type: string | ||
| # '400': | ||
| # description: Erroneous parameters (key invalid, list_value invalid) | ||
| # content: | ||
| # application/json: | ||
| # schema: | ||
| # type: object | ||
| # properties: | ||
| # error_status: | ||
| # type: integer | ||
| # error: | ||
| # type: string | ||
| # example: Erroneous parameters (key invalid, list_value invalid) | ||
| # text/plain: | ||
| # schema: | ||
| # type: string | ||
| # example: Erroneous parameters (key invalid, list_value invalid) |
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 remove
| # - $ref: '#/components/parameters/CSRFHeader' | ||
| # - in: cookie | ||
| # name: csrftoken | ||
| # schema: | ||
| # type: string |
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 remove
| responses: | ||
| '200': | ||
| description: 'OK. The jobs were scheduled synchronously.' | ||
| content: | ||
| application/json: | ||
| schema: | ||
| #$ref: '#/components/schemas/IsoSyncResponse' | ||
| type: object | ||
| properties: | ||
| result: | ||
| #type: {} #??????? | ||
| description: 'True if the update was successful' | ||
| # '202': | ||
| # description: 'Accepted. The job scheduling was enqueued to run asynchronously.' | ||
| # content: | ||
| # application/json: | ||
| # schema: | ||
| # $ref: '#/components/schemas/IsoAsyncResponse' | ||
| # '400': | ||
| # description: 'Bad Request with something is missing or invalid.' | ||
| # content: | ||
| # application/json: | ||
| # schema: | ||
| # type: string | ||
| # example: 'Specified scheduled_product_id is invalid.' | ||
| # '404': | ||
| # description: 'Not Found.' | ||
| # content: | ||
| # text/plain: | ||
| # schema: | ||
| # type: string | ||
| # example: 'Scheduled product to clone settings from not found.' |
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 remove
| components: | ||
| parameters: | ||
| CSRFHeader: | ||
| name: X-Requested-With | ||
| in: header | ||
| description: 'Required header to bypass CSRF check for API clients.' | ||
| required: true | ||
| schema: | ||
| type: string | ||
| default: XMLHttpRequest | ||
| securitySchemes: | ||
| basicAuth: | ||
| type: http | ||
| scheme: basic | ||
| description: "HTTP Basic Authentication using your username and a composite password of 'API_KEY:API_SECRET'." | ||
| oauth2Auth: | ||
| type: oauth2 | ||
| flows: | ||
| implicit: | ||
| authorizationUrl: https://id.opensuse.org/login/ldap | ||
| scopes: | ||
| admin: admin | ||
| operator: operator | ||
| bearerAuth: | ||
| type: http | ||
| scheme: bearer | ||
| bearerFormat: JWT # optional, arbitrary value for documentation purposes | ||
| schemas: | ||
| isosSync: | ||
| type: object | ||
| required: | ||
| - DISTRI | ||
| - VERSION | ||
| - FLAVOR | ||
| - ARCH | ||
| properties: | ||
| DISTRI: | ||
| type: string | ||
| description: 'The Linux distribution.' | ||
| example: 'opensuse' | ||
| VERSION: | ||
| type: string | ||
| description: 'The distribution version.' | ||
| example: '15.4' | ||
| FLAVOR: | ||
| type: string | ||
| description: 'The build flavor, e.g., DVD, Live, etc.' | ||
| example: 'DVD' | ||
| ARCH: | ||
| type: string | ||
| description: 'The processor architecture.' | ||
| example: 'x86_64' | ||
| async: | ||
| type: boolean | ||
| description: 'If true, the operation runs as a background job.' | ||
|
|
||
| isosAsync: | ||
| type: object | ||
| required: | ||
| - scheduled_product_clone_id | ||
| properties: | ||
| scheduled_product_clone_id: | ||
| type: integer | ||
| description: 'ID of a previous product to clone settings from.' | ||
| example: 12345 | ||
| async: | ||
| type: boolean | ||
| description: 'If true, the operation runs as a background job.' | ||
| additionalProperties: {} | ||
|
|
||
| IsoSyncResponse: | ||
| type: object | ||
| properties: | ||
| scheduled_product_id: | ||
| type: integer | ||
| count: | ||
| type: integer | ||
| ids: | ||
| type: array | ||
| items: | ||
| type: integer | ||
| failed: | ||
| type: object | ||
| additionalProperties: true | ||
|
|
||
| IsoAsyncResponse: | ||
| type: object | ||
| properties: | ||
| job_id: | ||
| type: integer | ||
| scheduled_product_id: | ||
| type: integer |
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 remove
| my $r = $self->routes->namespaces( | ||
| [ | ||
| 'OpenQA::Shared::Controller', 'OpenQA::WebAPI::Controller', | ||
| 'OpenQA::WebAPI', 'OpenQA::WebAPI::Controller::API::V1' |
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'm wondering about this one. Cc @kraih @Martchus
We need this so that the OpenAPI plugin can find the controllers.
But could this have any other implications to add this as a "global" namespace?
Currently we explicitly route to that namespace in WebAPI.pm for the api routes.
I couldn't find any configuration option in the OpenAPI plugin to configure a namespace.
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.
If we cannot specify the namespace (and just specify the controller/method via e.g. x-mojo-to: 'job#show') then we have to make sure that the names of the controller packages are unique. Probably not a big deal. Probably it wouldn't be hard to extend the plugin to allow specifying e.g. x-mojo-ns: 'OpenQA::WebAPI::Controller::API::V1' to avoid this.
|
I am not sure why this gets reviews. I will close it but I will try to answer general question wherever I can. |
|
Please leave it open for now, so it is visible for people, and please address the requested changes |
I wish to leave it as is. I do not expect this to ever merged and I wouldnt like to spend more time. |
There are two commits with the very basic changes.
which provides:
Two packages are required
cpanm Mojolicious::Plugin::OpenAPIandcpanm Mojolicious::Plugin::SwaggerUI. Remove the lines in the Setup for the second if you do not need it for now.If not, when you start the server you should see the API documentation with the Swagger UI (go to http://127.0.0.1/api/docs)
My understanding is that there are two ways to perform the validation. this is the not-automatic way.
You can ignore the components section as well. it is not used.