Skip to content

♻️ webserver: Refactor projects Domain to Align with Standardized Module Structure #7409

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

Merged
merged 39 commits into from
Mar 24, 2025

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Mar 21, 2025

What do these changes do?

This is the first in a series of (very noisy) PRs aimed at refactoring the projects domain to align interfaces with the standardized module structure introduced in #7294. The goal is to bring consistency and improve maintainability by following a well-defined convention which follows this pattern:

- projects
   - _controllers
       - _rest_schemas.py
       - _rest_exceptions.py 
       - $(component)_rest.py
       - $(component)_rpc.py
       - $(component)_slot.py
       - ...
   - _$(component)_repository.py
   - _$(component)_service.py
   - ...
   - projects_service.py
   - projects_$(component)_service.py
   - exceptions.py
   - models.py
   - settings.py
   - plugin.py

In this structure, $(component) refers to a logical section of the domain. Components can be internal (e.g. comments) or integration stubs to other domains (e.g. trash which has its own domain outside).

One of the advantages of this layout is that it cleanly reveals the coupling between modules. After this PR, "bad couplings" already are surfarcing. Nonetheless, those will be resolve in coming PRs. Moreover several components will be further refactored in coming PRs for the following reasons:

  • High coupling, often leading to circular dependencies
    → Solution: introduce abstract interfaces and reduce cross-imports
  • Overgrown modules that need to be split for clarity and separation of concerns

The following modules are especially high in coupling and will be refactored in later PRs:

  1. _crud_api_*.py → to be integrated into _projects_*_service.py
  2. _nodes_utils.py
  3. _projects_service*.py, api.py → to be merged into _projects_*_service.py
  4. nodes_utils.py
  5. utils.py

This PR establishes the initial structural foundation and introduces a consistent module layout.

This is the first several PRs that refactors the projects domain. This step aligns with the standardized module structure introduced in #7294, which follows the following convention:

- projects
   - _controllers
       - _rest_schemas.py
       - _rest_exceptions.py 
       - $(component)_rest.py
       - $(component)_rpc.py
       - $(component)_slot.py
       - ...
   - _$(component)_repository.py
   - _$(component)_service.py
   - ...
   - projects_service.py
   - projects_$(component)_service.py
   - exceptions.py
   - models.py
   - settings.py
   - plugin.py

A $(component) refer to a section of the domain's design. It can be internal (e.g. project's comments) or a stub used to interact with another domain (e.g. project's trash).
Some of these components will be further refactoring. These are the main reasons:

  • they are highly coupled, introducing easily circular dependencies
    • solution: using an abstract interface, they can be
  • they are too large and it is better to split

To keep changes minimal and reduce risk, the following high-coupling modules will be addressed in subsequent PRs:

  1. _crud_api_*.py → to be merged into _projects_*_service.py
  2. _nodes_utils.py
  3. _projects_service*.py, api.py → to be merged into _projects_*_service.py
  4. nodes_utils.py
  5. utils.py

This is the current view of the skeleton
image

Related issue/s

How to test

cd services/web/server
make install-dev
pytest -vv tests/unit/**/test_*project*.py

Dev-ops

None

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 92.92237% with 31 lines in your changes missing coverage. Please review.

Project coverage is 88.03%. Comparing base (abe7652) to head (4e4e780).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7409      +/-   ##
==========================================
+ Coverage   87.31%   88.03%   +0.71%     
==========================================
  Files        1713     1697      -16     
  Lines       66601    65572    -1029     
  Branches     1131     1131              
==========================================
- Hits        58152    57724     -428     
+ Misses       8127     7526     -601     
  Partials      322      322              
Flag Coverage Δ
integrationtests 65.26% <63.01%> (+0.09%) ⬆️
unittests 86.50% <90.86%> (+0.01%) ⬆️
Components Coverage Δ
api 76.84% <ø> (ø)
pkg_aws_library 94.24% <ø> (ø)
pkg_dask_task_models_library 97.09% <ø> (ø)
pkg_models_library 92.05% <ø> (ø)
pkg_notifications_library 84.57% <ø> (ø)
pkg_postgres_database 88.11% <ø> (ø)
pkg_service_integration 70.03% <ø> (ø)
pkg_service_library 73.01% <ø> (ø)
pkg_settings_library 90.78% <ø> (ø)
pkg_simcore_sdk 85.46% <ø> (ø)
agent 96.46% <ø> (ø)
api_server 90.68% <ø> (ø)
autoscaling 96.08% <ø> (ø)
catalog 92.14% <ø> (ø)
clusters_keeper 99.24% <ø> (ø)
dask_sidecar 91.25% <ø> (ø)
datcore_adapter 98.11% <ø> (ø)
director 76.61% <ø> (ø)
director_v2 91.37% <ø> (+0.07%) ⬆️
dynamic_scheduler 97.33% <ø> (ø)
dynamic_sidecar 90.16% <ø> (+0.04%) ⬆️
efs_guardian 89.79% <ø> (ø)
invitations 93.28% <ø> (ø)
payments 92.66% <ø> (ø)
resource_usage_tracker 89.02% <ø> (-0.11%) ⬇️
storage 84.28% <ø> (-0.20%) ⬇️
webclient ∅ <ø> (∅)
webserver 88.06% <92.92%> (+2.40%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abe7652...4e4e780. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov self-assigned this Mar 21, 2025
@pcrespov pcrespov added this to the The Awakening milestone Mar 21, 2025
@pcrespov pcrespov added the a:webserver issue related to the webserver service label Mar 21, 2025
@pcrespov pcrespov changed the title ♻️ Is7393/projects refactor ♻️ webserver: Refactor projects Domain to Align with Standardized Module Structure Mar 21, 2025
@pcrespov pcrespov marked this pull request as ready for review March 21, 2025 15:13
@pcrespov pcrespov force-pushed the is7393/projects-refactor branch from 027a517 to ee96fc3 Compare March 21, 2025 16:37
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.

Thanks for making order in the webserver!

so if I get that right:

  • handlers --> controllers
  • _api --> service
  • _db --> repository

I am personally not convinced at all by "controller" part. but well that is just a name. Also CRUD does not really contain special calls. Also I still find not good that the domain trash spills into the projects one. For me that is an additional function and projects should not be polluted by it.

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the heroic effort!
This is really great! As I mention below I think it is unfortunate that we have different conventions about the prefix/suffix depending on which layer in our architecture code is. But ok, that's a detail.

@pcrespov pcrespov force-pushed the is7393/projects-refactor branch from 2550848 to 8fecbb1 Compare March 24, 2025 10:01
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

all good, please check my one comment about the remove lined of code

@pcrespov
Copy link
Member Author

Based on the discussion with @GitHK , here some idea on how to start a new domain that is then transformed into a separate uservice

image

@pcrespov
Copy link
Member Author

pcrespov commented Mar 24, 2025

And here a layout of the pattern we are using. It is work in progress and included in docs/controller-service-repository.drawio.svg
controller-service-repository

@pcrespov pcrespov force-pushed the is7393/projects-refactor branch from 8fecbb1 to fa0002e Compare March 24, 2025 15:04
@pcrespov pcrespov enabled auto-merge (squash) March 24, 2025 15:19
@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Mar 24, 2025
@pcrespov
Copy link
Member Author

@Mergifyio queue

Copy link
Contributor

mergify bot commented Mar 24, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = [build] docker images (excluding frontend) (3.11, ubuntu-24.04)
        • check-neutral = [build] docker images (excluding frontend) (3.11, ubuntu-24.04)
        • check-skipped = [build] docker images (excluding frontend) (3.11, ubuntu-24.04)

@pcrespov pcrespov force-pushed the is7393/projects-refactor branch from e9fb914 to 4e4e780 Compare March 24, 2025 18:10
Copy link

@pcrespov pcrespov disabled auto-merge March 24, 2025 18:41
@pcrespov pcrespov merged commit 38ae3c0 into ITISFoundation:master Mar 24, 2025
91 of 92 checks passed
@pcrespov pcrespov deleted the is7393/projects-refactor branch March 24, 2025 18:50
mrnicegyu11 pushed a commit to mrnicegyu11/osparc-simcore that referenced this pull request Mar 26, 2025
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Apr 15, 2025
56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖-automerge marks PR as ready to be merged for Mergify a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants