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.

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

fix swagger page validation errors #10101

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Conversation

jfkonecn
Copy link
Contributor

Fix #9414

Testing Questions

Do you want me to add a python script to check for regression errors?

Maybe something similar to the code in the issue?

#! /bin/python3
from bravado.client import SwaggerClient
# fix exception is thrown then fail the test
SwaggerClient.from_url('http://localhost:8080/api/v2/api-docs?group=default')

If so, where should I add it?

PR changes

Describe changes proposed in this pull request:

  • Hid the Authentication object in the swagger pages in the StudyController
    • It seems like something that should be passed in the body of a POST request. Swagger was confused by it and so I removed it. For context I was getting this error below when running my test script.
jsonschema.exceptions.ValidationError: {'name': 'credentials', 'in': 'query', 'required': False, 'type': 'object'} is not valid under any of the given schemas

Failed validating 'oneOf' in schema['properties']['paths']['patternProperties']['^/']['properties']['get']['properties']['parameters']['items']:
    {'oneOf': [{'$ref': '#/definitions/parameter'},
               {'$ref': '#/definitions/jsonReference'}]}

On instance['paths']['/studies']['get']['parameters'][2]:
    {'in': 'query',
     'name': 'credentials',
     'required': False,
     'type': 'object'}
  • Changed default value values for tier parameters for the TreatmentController from Treatment to Agent since Treatment is not a valid value for ClinicalEventKeyCode

Checks

  • Runs on heroku
  • Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
  • The commit log is comprehensible. It follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
  • Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!

@ersinciftci @inodb

@inodb inodb added the gsoc label Apr 4, 2023
@inodb inodb requested a review from haynescd April 5, 2023 14:42
@sonarcloud
Copy link

sonarcloud bot commented Apr 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@haynescd
Copy link
Collaborator

haynescd commented Apr 7, 2023

Yea, good idea to add testing for our swagger api.

For the place to add it I would suggest creating a new github workflow (ex: sanity-check.yml)

Here is a potential solution for validating Swagger API if you didn't
https://swagger.io/blog/api-design/validate-openapi-definitions-swagger-editor/

@inodb
Copy link
Member

inodb commented Apr 7, 2023

@jfkonecn filed a separate ticket to add testing: cBioPortal/icebox#513. We don't really have a recommended place for adding these types of tests yet and there's a lot of ways one might do this, so prolly warrants further discussion

@inodb inodb added the api label Apr 7, 2023
@inodb inodb merged commit 1898d06 into cBioPortal:master Apr 7, 2023
@inodb inodb changed the title fixed swagger page validation errors fix swagger page validation errors Apr 7, 2023
@jfkonecn
Copy link
Contributor Author

jfkonecn commented Apr 7, 2023

@jfkonecn filed a separate ticket to add testing: cBioPortal/icebox#513. We don't really have a recommended place for adding these types of tests yet and there's a lot of ways one might do this, so prolly warrants further discussion

Sounds good. If you want to include me in the conversation feel free to add me to a meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swagger Validation Issues
3 participants