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

Add pydantic request models and documentation to API #102

Merged
merged 49 commits into from
Aug 30, 2021

Conversation

matthiasschaub
Copy link
Collaborator

@matthiasschaub matthiasschaub commented Jul 21, 2021

Description

Add pydantic request models and documentation to API.

Now through swagger it is possible to see which values are allowed for following parameters: dataset, fidField, indicator_name, report_name.
Also some documentation is added, which will be available through the swagger interface.

Corresponding issue

Adresses in part #2 and #120

Checklist

  • I have updated my branch to main (e.g. through git rebase main)
  • My code follows the style guide and was checked with pre-commit before committing
  • I have commented my code
  • I have added sufficient unit and integration tests
    • Extend tests on definitions.py
  • I have updated the CHANGELOG.md

@matthiasschaub matthiasschaub force-pushed the api-request-example branch 3 times, most recently from feff523 to 41305db Compare July 27, 2021 16:13
@matthiasschaub matthiasschaub marked this pull request as ready for review July 27, 2021 16:14
@matthiasschaub matthiasschaub changed the title Add request example data to FastApi Add pydatic request models and metadata to API Jul 27, 2021
@matthiasschaub matthiasschaub changed the title Add pydatic request models and metadata to API Add pydatic request models and parameter documentation to API Jul 28, 2021
@matthiasschaub matthiasschaub changed the title Add pydatic request models and parameter documentation to API Add pydatic request models and documentation to API Jul 28, 2021
@matthiasschaub matthiasschaub added the waiting for review This pull request urgently needs a code review label Jul 28, 2021
@matthiasschaub matthiasschaub removed the waiting for review This pull request urgently needs a code review label Jul 28, 2021
Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only had a brief look over your changes right now. At first glance it looks like a good change. The only general question that comes up is why they are called request models and not response models 🤔

CHANGELOG.md Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/api.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/api.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/utils/models.py Outdated Show resolved Hide resolved
@matthiasschaub matthiasschaub marked this pull request as draft August 3, 2021 12:15
CHANGELOG.md Outdated Show resolved Hide resolved
@joker234 joker234 self-requested a review August 9, 2021 08:56
@joker234 joker234 changed the title Add pydatic request models and documentation to API Add pydantic request models and documentation to API Aug 9, 2021
Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that was a lot of work. Thank you!

Some questions:

  • If I see this correctly, the parameters and field names in GET and POST requests will be different? Why do we have to stick with the camelCase parameter names in GET requests?
  • Where are the classmethods from request_models.py used?

workers/ohsome_quality_analyst/api/api.py Show resolved Hide resolved
workers/ohsome_quality_analyst/api/api.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/api/api.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/api/api.py Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/utils/definitions.py Outdated Show resolved Hide resolved
workers/tests/unittests/test_definitions.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/api/request_models.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/api/request_models.py Outdated Show resolved Hide resolved
@matthiasschaub
Copy link
Collaborator Author

matthiasschaub commented Aug 26, 2021

Wow, that was a lot of work. Thank you!

Some questions:

* If I see this correctly, the parameters and field names in GET and POST requests will be different? Why do we have to stick with the camelCase parameter names in GET requests?

* Where are the classmethods from `request_models.py` used?

I am glad to hear to work is appreciated. Thanks for your throughout review of this big PR. I will give my best to keep PRs small in the future.

To speak to your first question: The parameter and field names will stay the same as it was before (regardless of the request type POST or GET). The trick here is the alias_generator attribute of the Config class inside the BaseModel, which point to a funtion to translate variable names in snake case to lower camel case (snake_to_lower_camel).

Second question: The class methods will be called on each initialization of the class. pydantic magic will take care of that.

CHANGELOG.md Show resolved Hide resolved
workers/ohsome_quality_analyst/api/api.py Outdated Show resolved Hide resolved
Use it in definitions.py to get dataset values for dict.
... "if they don't collide with the properties set by OQT"
@joker234 joker234 added the enhancement New feature or request label Aug 30, 2021
@joker234 joker234 added this to the Release 0.6.0 milestone Aug 30, 2021
Add flake ignore comments for Fast-API parameters which are definied as mixedCase.
Run Black.
... "if they do not collide with the properties set by OQT."
joker234 and others added 5 commits August 30, 2021 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants