Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Sep 23, 2025

What do these changes do?

This PR updates all application keys from string-based keys to type-safe web.AppKey instances as recommended by aiohttp for type safety. In other words, this means that mypy will recognize the type of the value returned by that app's state.

This is the pattern used

Before:

from typing import Final

APP_MY_KEY: Final[str] = f"{__name__}.APP_MY_KEY"
app[APP_MY_KEY] = value
data = request.app[APP_MY_KEY]

After:

from typing import Final
from aiohttp import web

MY_APPKEY: Final = web.AppKey("MY", SomeSpecificType)
app[MY_APPKEY] = value
data = request.app[MY_APPKEY]  # Now type-safe with proper type 

Type Improvements

Where possible, we've used specific types instead of generic object:

  • APP_AIOPG_ENGINE_KEY uses Engine (from aiopg)
  • APP_CLIENT_SESSION_KEY uses aiohttp.ClientSession
  • APP_DOCKER_ENGINE_KEY uses aiodocker.Docker
  • APP_REDIS_CLIENT_KEY uses RedisClientsManager
  • APP_CONFIG_KEY uses dict[str, object]
  • APP_JSON_SCHEMA_SPECS_KEY uses dict[str, object]
  • APP_FIRE_AND_FORGET_TASKS_KEY uses set[object]
  • APP_SLOW_CALLBACKS_MONITOR_KEY uses LimitedOrderedStack[SlowCallback]

Benefits

  1. Type Safety: mypy can now properly type-check app key access with specific types
  2. Better IDE Support: IDEs can provide better autocomplete and error detection
  3. Runtime Safety: aiohttp validates key types at runtime
  4. Documentation: Key types are self-documenting and more precise

Related issue/s

How to test

mypy will consider also types returned by app[SOME_KEY]

@pcrespov pcrespov self-assigned this Sep 23, 2025
@codecov
Copy link

codecov bot commented Sep 23, 2025

Codecov Report

❌ Patch coverage is 98.74477% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.82%. Comparing base (4235aab) to head (63de04f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8405      +/-   ##
==========================================
+ Coverage   87.38%   87.82%   +0.44%     
==========================================
  Files        1953     1526     -427     
  Lines       76047    63401   -12646     
  Branches     1341      679     -662     
==========================================
- Hits        66452    55683   -10769     
+ Misses       9194     7482    -1712     
+ Partials      401      236     -165     
Flag Coverage Δ
integrationtests 64.02% <97.08%> (+3.66%) ⬆️
unittests 86.22% <98.74%> (-0.38%) ⬇️
Components Coverage Δ
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 72.63% <100.00%> (+0.07%) ⬆️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 84.93% <ø> (-0.06%) ⬇️
agent 93.53% <ø> (ø)
api_server 91.96% <50.00%> (ø)
autoscaling 95.74% <ø> (ø)
catalog 92.36% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 92.38% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 75.81% <ø> (ø)
director_v2 91.01% <ø> (+5.69%) ⬆️
dynamic_scheduler 96.30% <ø> (ø)
dynamic_sidecar 90.43% <ø> (ø)
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.62% <ø> (ø)
resource_usage_tracker 92.13% <ø> (-0.22%) ⬇️
storage 86.66% <ø> (-0.09%) ⬇️
webclient ∅ <ø> (∅)
webserver 88.05% <99.02%> (+0.05%) ⬆️

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 4235aab...63de04f. 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.

@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2025

🧪 CI Insights

Here's what we observed from your CI run for 63de04f.

🟢 All jobs passed!

But CI Insights is watching 👀

@pcrespov pcrespov added this to the Cheops milestone Sep 24, 2025
@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Sep 24, 2025
@pcrespov pcrespov force-pushed the is7734/maintenance-not-app-key branch from 1d3bf93 to c56a1e1 Compare September 25, 2025 09:34
@pcrespov pcrespov changed the title WIP: Is7734/maintenance not app key 🎨 Updates all aiohttp state application keys from string-based keys to type-safe web.AppKey instances Sep 25, 2025
@pcrespov pcrespov marked this pull request as ready for review September 25, 2025 10:14
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 migrates all aiohttp application keys from string-based keys to type-safe web.AppKey instances as recommended by aiohttp for enhanced type safety and mypy support. The change enables mypy to properly type-check app key access and provides better IDE support.

Key changes:

  • Replace string-based application keys with web.AppKey instances throughout the codebase
  • Import path updates: move from servicelib to local relative imports for constants
  • Define keys with specific types where possible (e.g., Engine, ClientSession, RedisClientsManager)

Reviewed Changes

Copilot reviewed 113 out of 113 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
services/web/server/src/simcore_service_webserver/constants.py Define type-safe application keys using web.AppKey
services/web/server/src/simcore_service_webserver/application_setup.py Create wrapper for servicelib app setup with proper app settings key
Multiple plugin files Update imports to use relative paths and new app setup function
Multiple service files Replace string key imports with type-safe key imports
packages/service-library/src/servicelib/aiohttp/application_keys.py Update to use web.AppKey for type safety

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@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 Sep 25, 2025
@pcrespov pcrespov force-pushed the is7734/maintenance-not-app-key branch from cb4c368 to d5dca47 Compare September 25, 2025 10:17
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.

and you forgot to say that it will reduce the zillion warnings in the webserver

@sonarqubecloud
Copy link

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Sep 25, 2025
@pcrespov
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Sep 25, 2025

queue

🛑 Configuration not compatible with a branch protection setting

The branch protection setting Require branches to be up to date before merging is not compatible with max_parallel_checks>1, queue_conditions != merge_conditions and must be unset.

@pcrespov pcrespov merged commit 407f10e into ITISFoundation:master Sep 25, 2025
95 checks passed
@pcrespov pcrespov deleted the is7734/maintenance-not-app-key branch September 25, 2025 11:28
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.

Address aiohttp NotAppKeyWarning: It is recommended to use web.AppKey instances for keys

3 participants