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

♻️ Moves webserver's projects api models to models_library #4425

Merged
merged 8 commits into from
Jun 27, 2023

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 27, 2023

What do these changes do?

OAS include schemas for the request body, response payload, path & query parameters. These schemas are represented in the code by pydantic models. Moving these models to models_library has the following benefits:

  • clients can use models (mypy type checks, early runtime validation)
  • separation between api schemas and domain (i.e. internal) data models
  • better control of schemas to guarantee API backwards compatibility (e.g. in api-server)

The schemas in models_library only correspond to those used by the backend services, i.e. all the APIs except for that of the api-server service.

Highlights

This PR includes:

  • models for projects resource in the webserver's API
    • tests against all captures of the webserver
  • models for long_running_tasks resource in servicelib
  • installs models_library into servicelib

Code Layout

I propose to define modules named like models_library.api_schemas_$(service_or_module_name) with

  • base:
    • package for common types/models to api schemas and domain models (in the corresponding services)
    • common functionality should be as minimal as possible since it is exposed in the API
  • $(resource-name) module for the resource schemas

Dependencies

A proposal on how to extend modules intra-dependencies including

graph LR
subgraph packages
    pg_models["postgres_database"]
    data_models["models_library"]
    settings["settings_library"]
    servicelib_fastapi["servicelib[fastapi]"]
    servicelib_aiohttp["servicelib[aiohttp]"]
end

subgraph main requirements
    sqlalchemy
    aiopg
    pydantic
    fastapi
    aiohttp
end

subgraph intra-package dependencies
    pg_models --> sqlalchemy
    pg_models --> data_models
    pg_models --> aiopg
    data_models --> pydantic
    settings --> pydantic
    servicelib_fastapi --> fastapi
    servicelib_fastapi --> data_models
    servicelib_fastapi --> settings
    servicelib_aiohttp --> aiohttp
    servicelib_aiohttp --> data_models
    servicelib_aiohttp --> settings
end
Loading

Related issue/s

How to test

DevOps

None

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #4425 (a99bd30) into master (9dc5a02) will increase coverage by 5.9%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4425     +/-   ##
========================================
+ Coverage    78.5%   84.5%   +5.9%     
========================================
  Files         820     722     -98     
  Lines       34967   34460    -507     
  Branches      627     664     +37     
========================================
+ Hits        27478   29129   +1651     
+ Misses       7340    5177   -2163     
- Partials      149     154      +5     
Flag Coverage Δ
integrationtests 61.0% <100.0%> (-6.9%) ⬇️
unittests 83.2% <100.0%> (+7.4%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...brary/src/servicelib/long_running_tasks/_models.py 100.0% <100.0%> (ø)
...e_service_webserver/projects/_crud_create_utils.py 97.2% <100.0%> (+38.1%) ⬆️
...ore_service_webserver/projects/_crud_read_utils.py 97.5% <100.0%> (ø)
...mcore_service_webserver/projects/_handlers_crud.py 82.0% <100.0%> (+11.1%) ⬆️
...mcore_service_webserver/projects/_permalink_api.py 97.2% <100.0%> (+24.7%) ⬆️

... and 608 files with indirect coverage changes

@pcrespov pcrespov self-assigned this Jun 27, 2023
@pcrespov pcrespov added this to the Watermelon milestone Jun 27, 2023
@pcrespov pcrespov added a:webserver issue related to the webserver service a:models-library labels Jun 27, 2023
@pcrespov pcrespov changed the title WIP: ♻️ Is4110/projects api models ♻️ Moves webserver's projects api models to models_library Jun 27, 2023
@pcrespov pcrespov marked this pull request as ready for review June 27, 2023 13:12
@pcrespov pcrespov force-pushed the is4110/projects-api-models branch from afcb0f7 to 12558f6 Compare June 27, 2023 13:12
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

as discussed, since we have pydantic through the basic_models in the postgres-database package I am not sure it makes sense to separate it from the postgres-repositories one.

@pcrespov
Copy link
Member Author

pcrespov commented Jun 27, 2023

as discussed, since we have pydantic through the basic_models in the postgres-database package I am not sure it makes sense to separate it from the postgres-repositories one.

@sanderegg check the diagram again. YOu mean this?

I personally like to separate models from "functions": servicelib would be the "utils" of models_lib and i would have another for postgres_lib

@pcrespov pcrespov force-pushed the is4110/projects-api-models branch from 12558f6 to a99bd30 Compare June 27, 2023 14:06
@codeclimate
Copy link

codeclimate bot commented Jun 27, 2023

Code Climate has analyzed commit a99bd30 and detected 0 issues on this pull request.

View more on Code Climate.

@pcrespov pcrespov enabled auto-merge (squash) June 27, 2023 14:10
@sonarqubecloud
Copy link

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

@pcrespov pcrespov merged commit fd95cd3 into ITISFoundation:master Jun 27, 2023
@pcrespov pcrespov deleted the is4110/projects-api-models branch June 27, 2023 17:21
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jul 12, 2023
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:models-library a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants