Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 25, 2025

What do these changes do?

The app_module_setup decorator is typically used in web applications (like those built with aiohttp) to define setup functions that initialize parts of the application — e.g., routes, background tasks, signals, middleware, etc. These setup functions must be idempotent: they should only run once, even if they are called multiple times (e.g., by different parts of the app or due to reuse of shared components).

✅ Purpose of app_module_setup

  • Marks a function as a module setup function.
  • Ensures it only runs once per app instance.
  • Used to register or configure aspects of the application.
  • May allow toggle/setup through config (e.g., optional plugins).

♻️ Why Refactor into Smaller Decorators (e.g., ensure_single_setup)

Refactoring app_module_setup into smaller decorators makes it:

  • Composable: Combine different setup constraints or conditions.
  • Re-usable: Use ensure_single_setup to guard any function (not just module setup) against multiple calls.
  • Split-friendly: Break one big setup into logically separated functions while still ensuring each part only runs once.

🧩 Example: Split Setup with Reusable Guarantees

# Split setup parts
@ensure_single_setup
def setup_routes(app):
    # register routes
    app.router.add_get("/", lambda request: web.Response(text="Hello"))
    
@ensure_single_setup
def setup_events(app):
    # register startup/shutdown signals
    async def on_startup(app): ...
    app.on_startup.append(on_startup)

# High-level setup composed from parts
@app_module_setup("A", ModuleCategory.ADDON, settings_name="WEBSERVER_A")
def setup_module_a(app):
    setup_module_b(app)
    setup_routes(app)
    setup_events(app)
  

# Then during application bootstrapping:
app = web.Application()
setup_module_a(app)
setup_module_b(app) # this is not executed because it was exectued in a

Related issue/s

How to test

cd packages/service-lib
make "install-dev[aiohttp]"
pytest -vv tests/aiohttp/test_application_setup.py

Dev-ops

None

@pcrespov pcrespov self-assigned this Jun 25, 2025
@codecov
Copy link

codecov bot commented Jun 25, 2025

Codecov Report

Attention: Patch coverage is 86.76471% with 9 lines in your changes missing coverage. Please review.

Project coverage is 87.77%. Comparing base (64695c3) to head (76f3cfb).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7982      +/-   ##
==========================================
- Coverage   87.88%   87.77%   -0.12%     
==========================================
  Files        1841     1423     -418     
  Lines       71051    59192   -11859     
  Branches     1228      627     -601     
==========================================
- Hits        62444    51953   -10491     
+ Misses       8252     7028    -1224     
+ Partials      355      211     -144     
Flag Coverage Δ
integrationtests 64.30% <100.00%> (+0.10%) ⬆️
unittests 86.08% <86.76%> (-0.40%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.63% <86.15%> (+0.06%) ⬆️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.05% <ø> (-0.06%) ⬇️
agent 96.29% <ø> (ø)
api_server 92.64% <ø> (ø)
autoscaling 96.03% <ø> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 92.35% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (ø)
director_v2 91.08% <ø> (-0.03%) ⬇️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.09% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.60% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 89.10% <ø> (+0.10%) ⬆️
storage 86.23% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 87.63% <100.00%> (+<0.01%) ⬆️

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 64695c3...76f3cfb. 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 added a:webserver webserver's codebase. Assigning the area is particularly useful for bugs a:services-library issues on packages/service-libs labels Jun 25, 2025
@pcrespov pcrespov added this to the Engage milestone Jun 25, 2025
@pcrespov pcrespov marked this pull request as ready for review June 25, 2025 21:30
@pcrespov pcrespov enabled auto-merge (squash) June 25, 2025 21:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the legacy app_module_setup decorator into smaller, composable decorators to improve modularity and enforce idempotent application setup routines.

  • Introduces ensure_single_setup to guard any setup function against multiple executions per app.
  • Adds _SetupTimingContext for consistent timing and logging around setups.
  • Rewrites app_module_setup to compose the new decorators and simplify internal logic.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/service-library/tests/aiohttp/test_application_setup.py Updated and extended tests to cover ensure_single_setup, adjusted fixtures.
packages/service-library/src/servicelib/aiohttp/application_setup.py Extracted idempotency logic into ensure_single_setup, added timing context, and restructured app_module_setup.
Comments suppressed due to low confidence (5)

packages/service-library/src/servicelib/aiohttp/application_setup.py:23

  • There is an extra space before the dot in the f-string, resulting in a storage key with an unintended space. It should be f"{__name__}.setup".
APP_SETUP_COMPLETED_KEY: Final[str] = f"{__name__ }.setup"

packages/service-library/src/servicelib/aiohttp/application_setup.py:180

  • [nitpick] Use the instance logger (self.logger) instead of the module‐level _logger here so that exit logs go through the same logger passed into the context.
            _logger.info("%s completed [Elapsed: %3.1f secs]", self.head_msg, elapsed)

packages/service-library/src/servicelib/aiohttp/application_setup.py:206

  • The decorator uses functools.wraps but functools is not imported in this module, causing a NameError. Add import functools at the top.
        @functools.wraps(setup_func)

packages/service-library/src/servicelib/aiohttp/application_setup.py:305

  • [nitpick] This assertion forces all setup functions to start with setup_, which conflicts with fixtures or helpers named _setup_*. Consider relaxing or removing this constraint or renaming impacted functions.
        assert setup_func.__name__.startswith(  # nosec

packages/service-library/tests/aiohttp/test_application_setup.py:168

  • [nitpick] The test hardcodes '<Application. Avoid logging objects like this>' as the app repr, which won’t match actual output. Either mock the app’s __repr__ or loosen the assertion to avoid a fragile placeholder.
        "'test.module' was already initialized in <Application. Avoid logging objects like this>. Setup can only be executed once per app.",

@pcrespov pcrespov force-pushed the is7781/setup-plugin-decorator branch from 0bca481 to 5b9cf94 Compare June 26, 2025 08:04
@pcrespov pcrespov requested a review from GitHK as a code owner June 26, 2025 08:04
@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Jun 26, 2025
@pcrespov
Copy link
Member Author

@mergify queue

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Jun 26, 2025
@mergify
Copy link
Contributor

mergify bot commented Jun 26, 2025

queue

🛑 The pull request has been merged manually

The pull request has been merged manually at e7d9064

@sonarqubecloud
Copy link

@pcrespov pcrespov merged commit e7d9064 into ITISFoundation:master Jun 26, 2025
96 of 98 checks passed
@pcrespov pcrespov deleted the is7781/setup-plugin-decorator branch June 26, 2025 10:43
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 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:services-library issues on packages/service-libs a:webserver webserver's codebase. Assigning the area is particularly useful for bugs t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants