From f24d78a85f0ba30652394f9e0cf1031dec39be96 Mon Sep 17 00:00:00 2001 From: Filipe Mondaini Date: Tue, 11 Mar 2025 15:27:23 -0300 Subject: [PATCH 1/5] Enforcing typing on app - handlers.py - pagination.py - utils.py - views.py --- api/app/handlers.py | 12 +++++++++--- api/app/pagination.py | 18 ++++++++++++------ api/app/utils.py | 37 +++++++++++++++++++++---------------- api/app/views.py | 4 ++-- 4 files changed, 44 insertions(+), 27 deletions(-) diff --git a/api/app/handlers.py b/api/app/handlers.py index 069e5b2b82b5..279396522bf2 100644 --- a/api/app/handlers.py +++ b/api/app/handlers.py @@ -3,7 +3,7 @@ import os -def mkdir_p(path): # type: ignore[no-untyped-def] +def mkdir_p(path: str) -> None: """http://stackoverflow.com/a/600612/190597 (tzot)""" try: os.makedirs(path, exist_ok=True) # Python>3.2 @@ -18,6 +18,12 @@ def mkdir_p(path): # type: ignore[no-untyped-def] class MakeFileHandler(logging.FileHandler): - def __init__(self, filename, mode="a", encoding=None, delay=0): # type: ignore[no-untyped-def] - mkdir_p(os.path.dirname(filename)) # type: ignore[no-untyped-call] + def __init__( + self, + filename: str, + mode: str = "a", + encoding: str | None = None, + delay: bool = False, + ) -> None: + mkdir_p(os.path.dirname(filename)) logging.FileHandler.__init__(self, filename, mode, encoding, delay) diff --git a/api/app/pagination.py b/api/app/pagination.py index 5a941a94414f..440e30e9d90b 100644 --- a/api/app/pagination.py +++ b/api/app/pagination.py @@ -1,11 +1,12 @@ import base64 import json from collections import OrderedDict +from typing import Any, List, Optional, Type from drf_yasg import openapi # type: ignore[import-untyped] from drf_yasg.inspectors import PaginatorInspector # type: ignore[import-untyped] from flag_engine.identities.models import IdentityModel -from rest_framework.pagination import PageNumberPagination +from rest_framework.pagination import BasePagination, PageNumberPagination from rest_framework.response import Response @@ -16,7 +17,9 @@ class CustomPagination(PageNumberPagination): class EdgeIdentityPaginationInspector(PaginatorInspector): # type: ignore[misc] - def get_paginator_parameters(self, paginator): # type: ignore[no-untyped-def] + def get_paginator_parameters( + self, paginator: Type[BasePagination] + ) -> list[openapi.Parameter]: """ :param BasePagination paginator: the paginator :rtype: list[openapi.Parameter] @@ -38,13 +41,14 @@ def get_paginator_parameters(self, paginator): # type: ignore[no-untyped-def] ), ] - def get_paginated_response(self, paginator, response_schema): # type: ignore[no-untyped-def] + def get_paginated_response( + self, paginator: Type[BasePagination], response_schema: openapi.Schema + ) -> openapi.Schema: """ :param BasePagination paginator: the paginator :param openapi.Schema response_schema: the response schema that must be paged. :rtype: openapi.Schema """ - return openapi.Schema( type=openapi.TYPE_OBJECT, properties=OrderedDict( @@ -67,7 +71,9 @@ class EdgeIdentityPagination(CustomPagination): max_page_size = 100 page_size = 100 - def paginate_queryset(self, dynamo_queryset, request, view=None): # type: ignore[no-untyped-def] + def paginate_queryset( + self, dynamo_queryset: Any, request: Any, view: Optional[Any] = None + ) -> Optional[List[Any]]: last_evaluated_key = dynamo_queryset.get("LastEvaluatedKey") if last_evaluated_key: self.last_evaluated_key = base64.b64encode( @@ -79,7 +85,7 @@ def paginate_queryset(self, dynamo_queryset, request, view=None): # type: ignor for identity_document in dynamo_queryset["Items"] ] - def get_paginated_response(self, data) -> Response: # type: ignore[no-untyped-def] + def get_paginated_response(self, data: Any) -> Response: """ Note: "If the size of the Query result set is larger than 1 MB, ScannedCount and Count represent only a partial count of the total items" diff --git a/api/app/utils.py b/api/app/utils.py index 7119ff0faa81..c1cfef74d6c5 100644 --- a/api/app/utils.py +++ b/api/app/utils.py @@ -18,6 +18,10 @@ class VersionInfo(TypedDict): is_saas: bool +class VersionInfoDict(VersionInfo): + package_versions: dict[str, str] + + def create_hash() -> str: """Helper function to create a short hash""" return shortuuid.uuid() @@ -44,27 +48,28 @@ def has_email_provider() -> bool: @lru_cache -def get_version_info() -> VersionInfo: +def get_version_info() -> VersionInfo | VersionInfoDict: """Reads the version info baked into src folder of the docker container""" - version_json = {} - image_tag = UNKNOWN - manifest_versions_content: str = _get_file_contents(VERSIONS_INFO_FILE_LOCATION) if manifest_versions_content != UNKNOWN: manifest_versions = json.loads(manifest_versions_content) - version_json["package_versions"] = manifest_versions - image_tag = manifest_versions["."] - - version_json = version_json | { - "ci_commit_sha": _get_file_contents("./CI_COMMIT_SHA"), - "image_tag": image_tag, - "has_email_provider": has_email_provider(), - "is_enterprise": is_enterprise(), - "is_saas": is_saas(), - } - - return version_json # type: ignore[return-value] + return VersionInfoDict( + ci_commit_sha=_get_file_contents("./CI_COMMIT_SHA"), + image_tag=manifest_versions["."], + has_email_provider=has_email_provider(), + is_enterprise=is_enterprise(), + is_saas=is_saas(), + package_versions=manifest_versions, + ) + + return VersionInfo( + ci_commit_sha=_get_file_contents("./CI_COMMIT_SHA"), + image_tag=UNKNOWN, + has_email_provider=has_email_provider(), + is_enterprise=is_enterprise(), + is_saas=is_saas(), + ) def _get_file_contents(file_path: str) -> str: diff --git a/api/app/views.py b/api/app/views.py index fbb751c39653..e96853b5d539 100644 --- a/api/app/views.py +++ b/api/app/views.py @@ -17,7 +17,7 @@ def version_info(request: Request) -> JsonResponse: @csrf_exempt -def index(request): # type: ignore[no-untyped-def] +def index(request: Request) -> HttpResponse: if request.method != "GET": logger.warning( "Invalid request made to %s with method %s", request.path, request.method @@ -28,7 +28,7 @@ def index(request): # type: ignore[no-untyped-def] return HttpResponse(template.render(request=request)) -def project_overrides(request): # type: ignore[no-untyped-def] +def project_overrides(request: Request) -> HttpResponse: """ Build and return the dictionary of front-end relevant environment variables for configuration. It gets loaded as a script tag in the head of the browser when the frontend application starts up. From 0092a6fc3f9c029b692313d9144e6bfac7163913 Mon Sep 17 00:00:00 2001 From: Filipe Mondaini Date: Tue, 11 Mar 2025 15:28:44 -0300 Subject: [PATCH 2/5] Enforcing typing on app: routers.py Added explicit None returns for readability. --- api/app/routers.py | 31 +++++++++++++++------ api/tests/unit/app/test_unit_app_routers.py | 14 +++++----- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/api/app/routers.py b/api/app/routers.py index 691b384432c3..fa4106c2ce2e 100644 --- a/api/app/routers.py +++ b/api/app/routers.py @@ -1,10 +1,12 @@ import logging import random from enum import Enum +from typing import Any, Type from django.conf import settings from django.core.cache import cache from django.db import connections +from django.db.models import Model from .exceptions import ImproperlyConfiguredError @@ -47,7 +49,7 @@ def connection_check(database: str) -> bool: class PrimaryReplicaRouter: - def db_for_read(self, model, **hints): # type: ignore[no-untyped-def] + def db_for_read(self, model: Type[Model], **hints: Any) -> str | None: if settings.NUM_DB_REPLICAS == 0: return "default" @@ -75,10 +77,12 @@ def db_for_read(self, model, **hints): # type: ignore[no-untyped-def] ) return "default" - def db_for_write(self, model, **hints): # type: ignore[no-untyped-def] + def db_for_write(self, model: Type[Model], **hints: Any) -> str | None: return "default" - def allow_relation(self, obj1, obj2, **hints): # type: ignore[no-untyped-def] + def allow_relation( + self, obj1: Type[Model], obj2: Type[Model], **hints: Any + ) -> bool | None: """ Relations between objects are allowed if both objects are in the primary/replica pool. @@ -95,10 +99,12 @@ def allow_relation(self, obj1, obj2, **hints): # type: ignore[no-untyped-def] return True return None - def allow_migrate(self, db, app_label, model_name=None, **hints): # type: ignore[no-untyped-def] + def allow_migrate( + self, db: str, app_label: str, model_name: str | None = None, **hints: Any + ) -> bool | None: return db == "default" - def _get_replica(self, replicas: list[str]) -> None | str: # type: ignore[return] + def _get_replica(self, replicas: list[str]) -> None | str: while replicas: if settings.REPLICA_READ_STRATEGY == ReplicaReadStrategy.DISTRIBUTED: database = random.choice(replicas) @@ -119,11 +125,14 @@ def _get_replica(self, replicas: list[str]) -> None | str: # type: ignore[retur if connection_check(database): return database + # If no replicas are available, return None + return None + class AnalyticsRouter: route_app_labels = ["app_analytics"] - def db_for_read(self, model, **hints): # type: ignore[no-untyped-def] + def db_for_read(self, model: Type[Model], **hints: Any) -> str | None: """ Attempts to read analytics models go to 'analytics' database. """ @@ -131,7 +140,7 @@ def db_for_read(self, model, **hints): # type: ignore[no-untyped-def] return "analytics" return None - def db_for_write(self, model, **hints): # type: ignore[no-untyped-def] + def db_for_write(self, model: Type[Model], **hints: Any) -> str | None: """ Attempts to write analytics models go to 'analytics' database. """ @@ -139,7 +148,9 @@ def db_for_write(self, model, **hints): # type: ignore[no-untyped-def] return "analytics" return None - def allow_relation(self, obj1, obj2, **hints): # type: ignore[no-untyped-def] + def allow_relation( + self, obj1: Type[Model], obj2: Type[Model], **hints: Any + ) -> bool | None: """ Relations between objects are allowed if both objects are in the analytics database. @@ -151,7 +162,9 @@ def allow_relation(self, obj1, obj2, **hints): # type: ignore[no-untyped-def] return True return None - def allow_migrate(self, db, app_label, model_name=None, **hints): # type: ignore[no-untyped-def] + def allow_migrate( + self, db: str, app_label: str, model_name: str | None = None, **hints: Any + ) -> bool | None: """ Make sure the analytics app only appears in the 'analytics' database """ diff --git a/api/tests/unit/app/test_unit_app_routers.py b/api/tests/unit/app/test_unit_app_routers.py index eb45d30d5fe4..b9b9bdfe20ef 100644 --- a/api/tests/unit/app/test_unit_app_routers.py +++ b/api/tests/unit/app/test_unit_app_routers.py @@ -39,12 +39,12 @@ def test_replica_router_db_for_read_with_one_offline_replica( router = PrimaryReplicaRouter() # When - result = router.db_for_read(FFAdminUser) # type: ignore[no-untyped-call] + result = router.db_for_read(FFAdminUser) # Then # Read strategy DISTRIBUTED is random, so just this is a check # against loading the primary or one of the cross region replicas - assert result.startswith("replica_") + assert result is not None and result.startswith("replica_") # Check that the number of replica call counts is as expected. conn_call_count = 2 @@ -85,12 +85,12 @@ def test_replica_router_db_for_read_with_local_offline_replicas( router = PrimaryReplicaRouter() # When - result = router.db_for_read(FFAdminUser) # type: ignore[no-untyped-call] + result = router.db_for_read(FFAdminUser) # Then # Read strategy DISTRIBUTED is random, so just this is a check # against loading the primary or one of the cross region replicas - assert result.startswith("cross_region_replica_") + assert result is not None and result.startswith("cross_region_replica_") # Check that the number of replica call counts is as expected. conn_call_count = 6 @@ -120,7 +120,7 @@ def test_replica_router_db_for_read_with_all_offline_replicas( router = PrimaryReplicaRouter() # When - result = router.db_for_read(FFAdminUser) # type: ignore[no-untyped-call] + result = router.db_for_read(FFAdminUser) # Then # Fallback to primary database if all replicas are offline. @@ -154,7 +154,7 @@ def test_replica_router_db_with_sequential_read( router = PrimaryReplicaRouter() # When - result = router.db_for_read(FFAdminUser) # type: ignore[no-untyped-call] + result = router.db_for_read(FFAdminUser) # Then # Fallback from first replica to second one. @@ -186,7 +186,7 @@ def test_replica_router_db_no_replicas( router = PrimaryReplicaRouter() # When - result = router.db_for_read(FFAdminUser) # type: ignore[no-untyped-call] + result = router.db_for_read(FFAdminUser) # Then # Should always use primary database. From db47af9233ffd05f1a8af35cae3fa07c54a19e63 Mon Sep 17 00:00:00 2001 From: Filipe Mondaini Date: Tue, 11 Mar 2025 19:18:06 -0300 Subject: [PATCH 3/5] Remove unused handlers.py file. --- api/app/handlers.py | 29 ----------------------------- 1 file changed, 29 deletions(-) delete mode 100644 api/app/handlers.py diff --git a/api/app/handlers.py b/api/app/handlers.py deleted file mode 100644 index 279396522bf2..000000000000 --- a/api/app/handlers.py +++ /dev/null @@ -1,29 +0,0 @@ -import errno -import logging -import os - - -def mkdir_p(path: str) -> None: - """http://stackoverflow.com/a/600612/190597 (tzot)""" - try: - os.makedirs(path, exist_ok=True) # Python>3.2 - except TypeError: - try: - os.makedirs(path) - except OSError as exc: # Python >2.5 - if exc.errno == errno.EEXIST and os.path.isdir(path): - pass - else: - raise - - -class MakeFileHandler(logging.FileHandler): - def __init__( - self, - filename: str, - mode: str = "a", - encoding: str | None = None, - delay: bool = False, - ) -> None: - mkdir_p(os.path.dirname(filename)) - logging.FileHandler.__init__(self, filename, mode, encoding, delay) From 433b0d81723b21bce71648e62152bb5db153e61b Mon Sep 17 00:00:00 2001 From: Filipe Mondaini Date: Thu, 20 Mar 2025 17:41:18 -0300 Subject: [PATCH 4/5] Router classes subclassing from TypedDatabasedRouter --- api/app/routers.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/api/app/routers.py b/api/app/routers.py index fa4106c2ce2e..a55e5a061d9f 100644 --- a/api/app/routers.py +++ b/api/app/routers.py @@ -1,12 +1,13 @@ import logging import random from enum import Enum -from typing import Any, Type +from typing import Any, Optional, Type from django.conf import settings from django.core.cache import cache from django.db import connections from django.db.models import Model +from django_stubs_ext.db.router import TypedDatabaseRouter from .exceptions import ImproperlyConfiguredError @@ -48,8 +49,8 @@ def connection_check(database: str) -> bool: return usable -class PrimaryReplicaRouter: - def db_for_read(self, model: Type[Model], **hints: Any) -> str | None: +class PrimaryReplicaRouter(TypedDatabaseRouter): + def db_for_read(self, model: Type[Model], **hints: Any) -> Optional[str]: if settings.NUM_DB_REPLICAS == 0: return "default" @@ -77,12 +78,12 @@ def db_for_read(self, model: Type[Model], **hints: Any) -> str | None: ) return "default" - def db_for_write(self, model: Type[Model], **hints: Any) -> str | None: + def db_for_write(self, model: Type[Model], **hints: Any) -> Optional[str]: return "default" def allow_relation( self, obj1: Type[Model], obj2: Type[Model], **hints: Any - ) -> bool | None: + ) -> Optional[bool]: """ Relations between objects are allowed if both objects are in the primary/replica pool. @@ -101,7 +102,7 @@ def allow_relation( def allow_migrate( self, db: str, app_label: str, model_name: str | None = None, **hints: Any - ) -> bool | None: + ) -> Optional[bool]: return db == "default" def _get_replica(self, replicas: list[str]) -> None | str: @@ -129,10 +130,10 @@ def _get_replica(self, replicas: list[str]) -> None | str: return None -class AnalyticsRouter: +class AnalyticsRouter(TypedDatabaseRouter): route_app_labels = ["app_analytics"] - def db_for_read(self, model: Type[Model], **hints: Any) -> str | None: + def db_for_read(self, model: Type[Model], **hints: Any) -> Optional[str]: """ Attempts to read analytics models go to 'analytics' database. """ @@ -140,7 +141,7 @@ def db_for_read(self, model: Type[Model], **hints: Any) -> str | None: return "analytics" return None - def db_for_write(self, model: Type[Model], **hints: Any) -> str | None: + def db_for_write(self, model: Type[Model], **hints: Any) -> Optional[str]: """ Attempts to write analytics models go to 'analytics' database. """ @@ -150,7 +151,7 @@ def db_for_write(self, model: Type[Model], **hints: Any) -> str | None: def allow_relation( self, obj1: Type[Model], obj2: Type[Model], **hints: Any - ) -> bool | None: + ) -> Optional[bool]: """ Relations between objects are allowed if both objects are in the analytics database. @@ -164,7 +165,7 @@ def allow_relation( def allow_migrate( self, db: str, app_label: str, model_name: str | None = None, **hints: Any - ) -> bool | None: + ) -> Optional[bool]: """ Make sure the analytics app only appears in the 'analytics' database """ From bd42100dc04372141ea4b812f00527fd369cecdf Mon Sep 17 00:00:00 2001 From: Filipe Mondaini Date: Tue, 8 Apr 2025 13:04:37 -0300 Subject: [PATCH 5/5] Added django_stubs_ext dependency --- api/poetry.lock | 5 +++-- api/pyproject.toml | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/api/poetry.lock b/api/poetry.lock index 0b557ae4d5d8..4f4588edb9a2 100644 --- a/api/poetry.lock +++ b/api/poetry.lock @@ -1544,7 +1544,7 @@ version = "5.1.3" description = "Monkey-patching and extensions for django-stubs" optional = false python-versions = ">=3.8" -groups = ["dev"] +groups = ["main", "dev"] files = [ {file = "django_stubs_ext-5.1.3-py3-none-any.whl", hash = "sha256:64561fbc53e963cc1eed2c8eb27e18b8e48dcb90771205180fe29fc8a59e55fd"}, {file = "django_stubs_ext-5.1.3.tar.gz", hash = "sha256:3e60f82337f0d40a362f349bf15539144b96e4ceb4dbd0239be1cd71f6a74ad0"}, @@ -4137,6 +4137,7 @@ files = [ {file = "PyYAML-6.0.1-cp311-cp311-win_amd64.whl", hash = "sha256:bf07ee2fef7014951eeb99f56f39c9bb4af143d8aa3c21b1677805985307da34"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"}, + {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a08c6f0fe150303c1c6b71ebcd7213c2858041a7e01975da3a99aed1e7a378ef"}, {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"}, {file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"}, {file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"}, @@ -5213,4 +5214,4 @@ files = [ [metadata] lock-version = "2.1" python-versions = ">3.11,<3.13" -content-hash = "ebd0216a0c486e3251081146995a6fffb3cdc7687d7b08e41441ba18223cdc13" +content-hash = "2f88cbe5492964ca9bbf48f75b6570648cf28360296e9c885330449f9b3895c6" diff --git a/api/pyproject.toml b/api/pyproject.toml index 8edb42c8d4c9..84c7a3880b0e 100644 --- a/api/pyproject.toml +++ b/api/pyproject.toml @@ -160,6 +160,7 @@ tzdata = "^2024.1" djangorestframework-simplejwt = "^5.3.1" structlog = "^24.4.0" prometheus-client = "^0.21.1" +django-stubs-ext = "^5.1.3" [tool.poetry.group.auth-controller] optional = true