diff --git a/docs/coding-conventions.md b/docs/coding-conventions.md index 495be5f4c20..e885d0052c5 100644 --- a/docs/coding-conventions.md +++ b/docs/coding-conventions.md @@ -72,7 +72,18 @@ In short we use the following naming convention ( roughly [PEP8](https://peps.p We should avoid merging PRs with ``TODO:`` and ``FIXME:`` into master. One of our bots detects those and flag them as code-smells. If we still want to keep this idea/fix noted in the code, those can be rewritten as ``NOTE:`` and should be extended with a link to a github issue with more details. For a context, see [discussion here](https://github.com/ITISFoundation/osparc-simcore/pull/3380#discussion_r979893502). - +### CC2: No commented code + +Avoid commented code, but if you *really* want to keep it then add an explanatory `NOTE:` +```python +import os +# import bar +# x = "not very useful" + +# NOTE: I need to keep this becase ... +# import foo +# x = "ok" +``` diff --git a/services/web/server/src/simcore_service_webserver/activity/handlers.py b/services/web/server/src/simcore_service_webserver/activity/handlers.py index 1eb809af3d2..9f2f1c80adc 100644 --- a/services/web/server/src/simcore_service_webserver/activity/handlers.py +++ b/services/web/server/src/simcore_service_webserver/activity/handlers.py @@ -58,7 +58,7 @@ async def get_status(request: aiohttp.web.Request): mem_usage = get_prometheus_result_or_default(results[1], []) metric = get_prometheus_result_or_default(results[2], []) - res = defaultdict(dict) + res: dict = defaultdict(dict) for node in cpu_usage: node_id = node["metric"]["container_label_node_id"] usage = float(node["value"][1]) diff --git a/services/web/server/src/simcore_service_webserver/application_settings.py b/services/web/server/src/simcore_service_webserver/application_settings.py index 88e7ff9a095..685c63c1e89 100644 --- a/services/web/server/src/simcore_service_webserver/application_settings.py +++ b/services/web/server/src/simcore_service_webserver/application_settings.py @@ -44,7 +44,7 @@ from .storage.settings import StorageSettings from .studies_dispatcher.settings import StudiesDispatcherSettings -log = logging.getLogger(__name__) +_logger = logging.getLogger(__name__) class ApplicationSettings(BaseCustomSettings, MixinLoggingSettings): @@ -254,16 +254,19 @@ def enable_only_if_dev_features_allowed(cls, v, values, field: ModelField): if values["WEBSERVER_DEV_FEATURES_ENABLED"]: return v if v: - log.warning("%s still under development and will be disabled.", field.name) + _logger.warning( + "%s still under development and will be disabled.", field.name + ) return None if field.allow_none else False @cached_property def log_level(self) -> int: - return getattr(logging, self.WEBSERVER_LOGLEVEL.upper()) + level: int = getattr(logging, self.WEBSERVER_LOGLEVEL.upper()) + return level @validator("WEBSERVER_LOGLEVEL") @classmethod - def valid_log_level(cls, value) -> str: + def valid_log_level(cls, value): return cls.validate_log_level(value) @validator("SC_HEALTHCHECK_TIMEOUT", pre=True) @@ -323,7 +326,7 @@ def _export_by_alias(self, **kwargs) -> dict[str, Any]: } config_alias_generator = lambda s: s.lower() - data = self.dict(**kwargs) + data: dict[str, Any] = self.dict(**kwargs) current_keys = list(data.keys()) for key in current_keys: @@ -375,8 +378,9 @@ def to_client_statics(self) -> dict[str, Any]: def setup_settings(app: web.Application) -> ApplicationSettings: - app[APP_SETTINGS_KEY] = settings = ApplicationSettings.create_from_envs() - log.info( + settings: ApplicationSettings = ApplicationSettings.create_from_envs() + app[APP_SETTINGS_KEY] = settings + _logger.debug( "Captured app settings:\n%s", app[APP_SETTINGS_KEY].json(indent=1, sort_keys=True), ) @@ -384,4 +388,6 @@ def setup_settings(app: web.Application) -> ApplicationSettings: def get_settings(app: web.Application) -> ApplicationSettings: - return app[APP_SETTINGS_KEY] + settings: ApplicationSettings = app[APP_SETTINGS_KEY] + assert settings, "Forgot to setup plugin?" # nosec + return settings diff --git a/services/web/server/src/simcore_service_webserver/application_settings_utils.py b/services/web/server/src/simcore_service_webserver/application_settings_utils.py index 80ad640d4f0..6c47ffc0822 100644 --- a/services/web/server/src/simcore_service_webserver/application_settings_utils.py +++ b/services/web/server/src/simcore_service_webserver/application_settings_utils.py @@ -12,7 +12,7 @@ from .application_settings import ApplicationSettings -log = logging.getLogger(__name__) +_logger = logging.getLogger(__name__) def convert_to_app_config(app_settings: ApplicationSettings) -> dict[str, Any]: diff --git a/services/web/server/src/simcore_service_webserver/login/handlers_confirmation.py b/services/web/server/src/simcore_service_webserver/login/handlers_confirmation.py index 8f6ef3406d9..8fa5678582d 100644 --- a/services/web/server/src/simcore_service_webserver/login/handlers_confirmation.py +++ b/services/web/server/src/simcore_service_webserver/login/handlers_confirmation.py @@ -17,7 +17,7 @@ from ..security.api import encrypt_password from ..session.access_policies import session_access_required from ..utils import MINUTE -from ..utils_aiohttp import create_redirect_response +from ..utils_aiohttp import create_redirect_to_page_response from ..utils_rate_limiting import global_rate_limit_route from ._2fa import delete_2fa_code, get_2fa_code from ._confirmation import validate_confirmation_code @@ -123,7 +123,7 @@ async def validate_confirmation_and_redirect(request: web.Request): f"{error_code}", extra={"error_code": error_code}, ) - raise create_redirect_response( + raise create_redirect_to_page_response( request.app, page="error", message=f"Sorry, we cannot confirm your {action}." diff --git a/services/web/server/src/simcore_service_webserver/long_running_tasks.py b/services/web/server/src/simcore_service_webserver/long_running_tasks.py index f85c6f2843d..102c0bacc57 100644 --- a/services/web/server/src/simcore_service_webserver/long_running_tasks.py +++ b/services/web/server/src/simcore_service_webserver/long_running_tasks.py @@ -17,8 +17,8 @@ class _RequestContext(BaseModel): - user_id: UserID = Field(..., alias=RQT_USERID_KEY) - product_name: str = Field(..., alias=RQ_PRODUCT_KEY) + user_id: UserID = Field(..., alias=RQT_USERID_KEY) # type: ignore[pydantic-alias] + product_name: str = Field(..., alias=RQ_PRODUCT_KEY) # type: ignore[pydantic-alias] def _webserver_request_context_decorator(handler: Handler): diff --git a/services/web/server/src/simcore_service_webserver/rabbitmq_settings.py b/services/web/server/src/simcore_service_webserver/rabbitmq_settings.py index cc1e4194bd4..a05929f1c1b 100644 --- a/services/web/server/src/simcore_service_webserver/rabbitmq_settings.py +++ b/services/web/server/src/simcore_service_webserver/rabbitmq_settings.py @@ -4,7 +4,6 @@ - settings """ -from typing import Optional from aiohttp.web import Application from settings_library.rabbit import RabbitSettings @@ -13,7 +12,13 @@ def get_plugin_settings(app: Application) -> RabbitSettings: - settings: Optional[RabbitSettings] = app[APP_SETTINGS_KEY].WEBSERVER_RABBITMQ + settings: RabbitSettings | None = app[APP_SETTINGS_KEY].WEBSERVER_RABBITMQ assert settings, "setup_settings not called?" # nosec assert isinstance(settings, RabbitSettings) # nosec return settings + + +__all__: tuple[str, ...] = ( + "RabbitSettings", + "get_plugin_settings", +) diff --git a/services/web/server/src/simcore_service_webserver/resource_manager/plugin.py b/services/web/server/src/simcore_service_webserver/resource_manager/plugin.py index 73a352ccdf5..803632e7d63 100644 --- a/services/web/server/src/simcore_service_webserver/resource_manager/plugin.py +++ b/services/web/server/src/simcore_service_webserver/resource_manager/plugin.py @@ -15,14 +15,14 @@ from ._constants import APP_CLIENT_SOCKET_REGISTRY_KEY, APP_RESOURCE_MANAGER_TASKS_KEY from .registry import RedisResourceRegistry -logger = logging.getLogger(__name__) +_logger = logging.getLogger(__name__) @app_module_setup( "simcore_service_webserver.resource_manager", ModuleCategory.SYSTEM, settings_name="WEBSERVER_RESOURCE_MANAGER", - logger=logger, + logger=_logger, ) def setup_resource_manager(app: web.Application) -> bool: """Sets up resource manager subsystem in the application""" diff --git a/services/web/server/src/simcore_service_webserver/resource_manager/registry.py b/services/web/server/src/simcore_service_webserver/resource_manager/registry.py index ad336d2bb09..ac774ce66d8 100644 --- a/services/web/server/src/simcore_service_webserver/resource_manager/registry.py +++ b/services/web/server/src/simcore_service_webserver/resource_manager/registry.py @@ -23,13 +23,13 @@ from ..redis import get_redis_resources_client from ._constants import APP_CLIENT_SOCKET_REGISTRY_KEY -log = logging.getLogger(__name__) +_logger = logging.getLogger(__name__) ALIVE_SUFFIX = "alive" RESOURCE_SUFFIX = "resources" -class RegistryKeyPrefixDict(TypedDict): +class RegistryKeyPrefixDict(TypedDict, total=False): """Parts of the redis key w/o suffix""" user_id: str | int @@ -61,7 +61,7 @@ def app(self) -> web.Application: @classmethod def _hash_key(cls, key: RegistryKeyPrefixDict) -> str: - hash_key = ":".join(f"{item[0]}={item[1]}" for item in key.items()) + hash_key = ":".join(f"{k}={v}" for k, v in key.items()) return hash_key @classmethod @@ -72,7 +72,7 @@ def _decode_hash_key(cls, hash_key: str) -> RegistryKeyPrefixDict: else hash_key[: -len(f":{ALIVE_SUFFIX}")] ) key = dict(x.split("=") for x in tmp_key.split(":")) - return RegistryKeyPrefixDict(**key) + return RegistryKeyPrefixDict(**key) # type: ignore @property def client(self) -> aioredis.Redis: @@ -89,7 +89,7 @@ async def set_resource( async def get_resources(self, key: RegistryKeyPrefixDict) -> ResourcesValueDict: hash_key = f"{self._hash_key(key)}:{RESOURCE_SUFFIX}" fields = await self.client.hgetall(hash_key) - return ResourcesValueDict(**fields) + return ResourcesValueDict(**fields) # type: ignore async def remove_resource( self, key: RegistryKeyPrefixDict, resource_name: str @@ -109,7 +109,7 @@ async def find_resources( return resources async def find_keys(self, resource: tuple[str, str]) -> list[RegistryKeyPrefixDict]: - keys = [] + keys: list[RegistryKeyPrefixDict] = [] if not resource: return keys @@ -153,4 +153,6 @@ async def get_all_resource_keys( def get_registry(app: web.Application) -> RedisResourceRegistry: - return app[APP_CLIENT_SOCKET_REGISTRY_KEY] + client: RedisResourceRegistry = app[APP_CLIENT_SOCKET_REGISTRY_KEY] + assert isinstance(client, RedisResourceRegistry) # nosec + return client diff --git a/services/web/server/src/simcore_service_webserver/resource_manager/websocket_manager.py b/services/web/server/src/simcore_service_webserver/resource_manager/websocket_manager.py index 27c845d11b0..050e34ec139 100644 --- a/services/web/server/src/simcore_service_webserver/resource_manager/websocket_manager.py +++ b/services/web/server/src/simcore_service_webserver/resource_manager/websocket_manager.py @@ -22,14 +22,18 @@ from aiohttp import web from servicelib.logging_utils import get_log_record_extra, log_context -from .registry import get_registry +from .registry import RegistryKeyPrefixDict, ResourcesValueDict, get_registry from .settings import ResourceManagerSettings, get_plugin_settings -log = logging.getLogger(__name__) +_logger = logging.getLogger(__name__) + SOCKET_ID_KEY = "socket_id" PROJECT_ID_KEY = "project_id" +assert SOCKET_ID_KEY in ResourcesValueDict.__annotations__.keys() # nosec +assert PROJECT_ID_KEY in ResourcesValueDict.__annotations__.keys() # nosec + def get_service_deletion_timeout(app: web.Application) -> int: settings: ResourceManagerSettings = get_plugin_settings(app) @@ -59,16 +63,14 @@ class WebsocketRegistry: client_session_id: str | None app: web.Application - def _resource_key(self) -> dict[str, str]: - return { - "user_id": f"{self.user_id}", - "client_session_id": self.client_session_id - if self.client_session_id - else "*", - } + def _resource_key(self) -> RegistryKeyPrefixDict: + return RegistryKeyPrefixDict( + user_id=f"{self.user_id}", + client_session_id=self.client_session_id or "*", + ) async def set_socket_id(self, socket_id: str) -> None: - log.debug( + _logger.debug( "user %s/tab %s adding socket %s in registry...", self.user_id, self.client_session_id, @@ -83,14 +85,15 @@ async def set_socket_id(self, socket_id: str) -> None: await registry.set_key_alive(self._resource_key(), timeout) async def get_socket_id(self) -> str | None: - log.debug( + _logger.debug( "user %s/tab %s getting socket from registry...", self.user_id, self.client_session_id, ) registry = get_registry(self.app) resources = await registry.get_resources(self._resource_key()) - return resources.get(SOCKET_ID_KEY, None) + key: str | None = resources.get("socket_id", None) + return key async def user_pressed_disconnect(self) -> None: """When the user disconnects expire as soon as possible the alive key @@ -99,7 +102,7 @@ async def user_pressed_disconnect(self) -> None: await registry.set_key_alive(self._resource_key(), 1) async def remove_socket_id(self) -> None: - log.debug( + _logger.debug( "user %s/tab %s removing socket from registry...", self.user_id, self.client_session_id, @@ -119,7 +122,7 @@ async def set_heartbeat(self) -> None: ) async def find_socket_ids(self) -> list[str]: - log.debug( + _logger.debug( "user %s/tab %s finding %s from registry...", self.user_id, self.client_session_id, @@ -134,7 +137,7 @@ async def find_socket_ids(self) -> list[str]: async def find_all_resources_of_user(self, key: str) -> list[str]: with log_context( - log, + _logger, logging.DEBUG, msg=f"{self.user_id=} finding all {key} from registry", extra=get_log_record_extra(user_id=self.user_id), @@ -145,7 +148,7 @@ async def find_all_resources_of_user(self, key: str) -> list[str]: return resources async def find(self, key: str) -> list[str]: - log.debug( + _logger.debug( "user %s/tab %s finding %s from registry...", self.user_id, self.client_session_id, @@ -157,7 +160,7 @@ async def find(self, key: str) -> list[str]: return user_resources async def add(self, key: str, value: str) -> None: - log.debug( + _logger.debug( "user %s/tab %s adding %s:%s in registry...", self.user_id, self.client_session_id, @@ -169,7 +172,7 @@ async def add(self, key: str, value: str) -> None: await registry.set_resource(self._resource_key(), (key, value)) async def remove(self, key: str) -> None: - log.debug( + _logger.debug( "user %s/tab %s removing %s from registry...", self.user_id, self.client_session_id, @@ -180,7 +183,7 @@ async def remove(self, key: str) -> None: await registry.remove_resource(self._resource_key(), key) async def find_users_of_resource(self, key: str, value: str) -> list[UserSessionID]: - log.debug( + _logger.debug( "user %s/tab %s finding %s:%s in registry...", self.user_id, self.client_session_id, @@ -205,7 +208,7 @@ def managed_resource( registry = WebsocketRegistry(int(user_id), client_session_id, app) yield registry except Exception: - log.exception( + _logger.exception( "Error in web-socket for user:%s, session:%s", user_id, client_session_id, diff --git a/services/web/server/src/simcore_service_webserver/studies_dispatcher/_projects_permalinks.py b/services/web/server/src/simcore_service_webserver/studies_dispatcher/_projects_permalinks.py index 9771dfb4707..fb5868b13bc 100644 --- a/services/web/server/src/simcore_service_webserver/studies_dispatcher/_projects_permalinks.py +++ b/services/web/server/src/simcore_service_webserver/studies_dispatcher/_projects_permalinks.py @@ -4,6 +4,7 @@ import sqlalchemy as sa from aiohttp import web from models_library.projects import ProjectID, ProjectIDStr +from pydantic import HttpUrl, parse_obj_as from simcore_postgres_database.models.projects import ProjectType, projects from ..db import get_database_engine @@ -56,9 +57,9 @@ def create_permalink_for_study( # create url_for = create_url_for_function(request) - - permalink: str = url_for( - route_name="get_redirection_to_study_page", id=f"{project_uuid}" + permalink = parse_obj_as( + HttpUrl, + url_for(route_name="get_redirection_to_study_page", id=f"{project_uuid}"), ) return ProjectPermalink( diff --git a/services/web/server/src/simcore_service_webserver/studies_dispatcher/_redirects_handlers.py b/services/web/server/src/simcore_service_webserver/studies_dispatcher/_redirects_handlers.py index 2a822b6effd..b239e12e13e 100644 --- a/services/web/server/src/simcore_service_webserver/studies_dispatcher/_redirects_handlers.py +++ b/services/web/server/src/simcore_service_webserver/studies_dispatcher/_redirects_handlers.py @@ -17,7 +17,7 @@ from ..products.plugin import get_product_name from ..utils import compose_support_error_msg -from ..utils_aiohttp import create_redirect_response +from ..utils_aiohttp import create_redirect_to_page_response from ._catalog import ValidService, validate_requested_service from ._constants import MSG_UNEXPECTED_ERROR from ._core import validate_requested_file, validate_requested_viewer @@ -46,7 +46,7 @@ def _create_redirect_response_to_view_page( file_size: int | str | None, ) -> web.HTTPFound: # NOTE: these are 'view' page params and need to be interpreted by front-end correctly! - return create_redirect_response( + return create_redirect_to_page_response( app, page="view", project_id=f"{project_id}", @@ -60,7 +60,7 @@ def _create_redirect_response_to_error_page( app: web.Application, message: str, status_code: int ) -> web.HTTPFound: # NOTE: these are 'error' page params and need to be interpreted by front-end correctly! - return create_redirect_response( + return create_redirect_to_page_response( app, page="error", message=message, diff --git a/services/web/server/src/simcore_service_webserver/studies_dispatcher/_studies_access.py b/services/web/server/src/simcore_service_webserver/studies_dispatcher/_studies_access.py index 2e9cdbf5cb0..cddcc634d00 100644 --- a/services/web/server/src/simcore_service_webserver/studies_dispatcher/_studies_access.py +++ b/services/web/server/src/simcore_service_webserver/studies_dispatcher/_studies_access.py @@ -38,7 +38,7 @@ from ..security.api import is_anonymous, remember from ..storage.api import copy_data_folders_from_project from ..utils import compose_support_error_msg -from ..utils_aiohttp import create_redirect_response +from ..utils_aiohttp import create_redirect_to_page_response from ._constants import ( MSG_PROJECT_NOT_FOUND, MSG_PROJECT_NOT_PUBLISHED, @@ -287,7 +287,7 @@ async def wrapper(request: web.Request) -> web.StreamResponse: return await handler(request) except ProjectNotFoundError as err: - raise create_redirect_response( + raise create_redirect_to_page_response( request.app, page="error", message=compose_support_error_msg( @@ -298,7 +298,7 @@ async def wrapper(request: web.Request) -> web.StreamResponse: ) from err except RedirectToFrontEndPageError as err: - raise create_redirect_response( + raise create_redirect_to_page_response( request.app, page="error", message=err.human_readable_message, @@ -312,7 +312,7 @@ async def wrapper(request: web.Request) -> web.StreamResponse: f"{error_code}", extra={"error_code": error_code}, ) - raise create_redirect_response( + raise create_redirect_to_page_response( request.app, page="error", message=compose_support_error_msg( diff --git a/services/web/server/src/simcore_service_webserver/utils_aiohttp.py b/services/web/server/src/simcore_service_webserver/utils_aiohttp.py index 9e59a5fd7d0..96c8515a50e 100644 --- a/services/web/server/src/simcore_service_webserver/utils_aiohttp.py +++ b/services/web/server/src/simcore_service_webserver/utils_aiohttp.py @@ -1,12 +1,12 @@ import io import logging -from typing import Any, Callable, Generic, Literal, TypeVar +from typing import Any, Callable, Generic, Literal, TypeAlias, TypeVar from aiohttp import web from aiohttp.web_exceptions import HTTPError, HTTPException from aiohttp.web_routedef import RouteDef, RouteTableDef from models_library.generics import Envelope -from pydantic import Field +from pydantic import BaseModel, Field from pydantic.generics import GenericModel from servicelib.common_headers import X_FORWARDED_PROTO from servicelib.json_serialization import json_dumps @@ -36,13 +36,13 @@ def get_routes_view(routes: RouteTableDef) -> str: def create_url_for_function(request: web.Request) -> Callable: app = request.app - def url_for(route_name: str, **params: dict[str, Any]) -> str: + def _url_for(route_name: str, **params: dict[str, Any]) -> str: """Reverse URL constructing using named resources""" try: rel_url: URL = app.router[route_name].url_for( **{k: f"{v}" for k, v in params.items()} ) - url = ( + url: URL = ( request.url.origin() .with_scheme( # Custom header by traefik. See labels in docker-compose as: @@ -59,7 +59,7 @@ def url_for(route_name: str, **params: dict[str, Any]) -> str: "Check name spelling or whether the router was not registered" ) from err - return url_for + return _url_for def envelope_json_response( @@ -82,9 +82,11 @@ def envelope_json_response( # Special models and responses for the front-end # +PageStr: TypeAlias = Literal["view", "error"] -def create_redirect_response( - app: web.Application, page: Literal["view", "error"], **parameters + +def create_redirect_to_page_response( + app: web.Application, page: PageStr, **parameters ) -> web.HTTPFound: """ Returns a redirect response to the front-end with information on page @@ -112,7 +114,7 @@ def create_redirect_response( return web.HTTPFound(location=redirect_url) -PageParameters = TypeVar("PageParameters") +PageParameters = TypeVar("PageParameters", bound=BaseModel) class NextPage(GenericModel, Generic[PageParameters]): @@ -124,10 +126,7 @@ class NextPage(GenericModel, Generic[PageParameters]): using a path+query in the fragment of the URL """ - name: str = Field(..., description="Code name to the front-end page") + name: str = Field( + ..., description="Code name to the front-end page. Ideally a PageStr" + ) parameters: PageParameters | None = None - - def as_redirect_response(self, app: web.Application) -> web.HTTPFound: - return create_redirect_response( - app=app, page=self.name, **self.parameters.dict() - ) diff --git a/services/web/server/src/simcore_service_webserver/utils_rate_limiting.py b/services/web/server/src/simcore_service_webserver/utils_rate_limiting.py index b5d06e606c2..61064c2459f 100644 --- a/services/web/server/src/simcore_service_webserver/utils_rate_limiting.py +++ b/services/web/server/src/simcore_service_webserver/utils_rate_limiting.py @@ -3,7 +3,7 @@ from datetime import datetime, timedelta from functools import wraps from math import ceil -from typing import NamedTuple +from typing import Callable, NamedTuple from aiohttp.web_exceptions import HTTPTooManyRequests @@ -26,7 +26,7 @@ def global_rate_limit_route(number_of_requests: int, interval_seconds: float): """ # compute the amount of requests per - def decorator(decorated_function): + def _decorator(decorated_function: Callable): @dataclass class _Context: max_allowed: int # maximum allowed requests per interval @@ -40,7 +40,7 @@ class _Context: ) @wraps(decorated_function) - async def wrapper(*args, **kwargs): + async def _wrapper(*args, **kwargs): utc_now = datetime.utcnow() utc_now_timestamp = datetime.timestamp(utc_now) @@ -75,7 +75,7 @@ async def wrapper(*args, **kwargs): context.remaining -= 1 return await decorated_function(*args, **kwargs) - wrapper.rate_limit_setup = RateLimitSetup(number_of_requests, interval_seconds) - return wrapper + _wrapper.rate_limit_setup = RateLimitSetup(number_of_requests, interval_seconds) # type: ignore + return _wrapper - return decorator + return _decorator