From 82cdf929115019b396c773707f8477017cdb652f Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Sat, 7 Dec 2024 20:11:23 +0000 Subject: [PATCH 1/9] add PGH ruleset --- ruff.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruff.toml b/ruff.toml index e789980ef..274d4ad7a 100644 --- a/ruff.toml +++ b/ruff.toml @@ -3,7 +3,7 @@ line-length = 120 target-version = "py311" [lint] -select = ["E", "F", "W", "I", "ASYNC", "UP", "FLY", "PERF", "FURB", "ERA", "ANN", "PIE", "PL"] +select = ["E", "F", "W", "I", "ASYNC", "UP", "FLY", "PERF", "FURB", "ERA", "ANN", "PIE", "PL", "PGH"] ignore = ["E501", "PLR", "PLW"] From 6c4967eafd44eeea32aeac8ab614c601a9eafc09 Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Sat, 7 Dec 2024 20:11:55 +0000 Subject: [PATCH 2/9] be more explicit with ignores --- src/eduid/userdb/db/sync_db.py | 2 +- src/eduid/userdb/element.py | 10 +++++----- src/eduid/webapp/authn/app.py | 2 +- src/eduid/webapp/common/api/decorators.py | 2 +- src/eduid/webapp/common/authn/utils.py | 2 +- src/eduid/webapp/eidas/proofing.py | 2 +- src/eduid/workers/am/tests/test_index.py | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/eduid/userdb/db/sync_db.py b/src/eduid/userdb/db/sync_db.py index 0fb13271d..fad8cd9af 100644 --- a/src/eduid/userdb/db/sync_db.py +++ b/src/eduid/userdb/db/sync_db.py @@ -324,7 +324,7 @@ def legacy_save(self, doc: dict[str, Any]) -> str: if "_id" in doc: self._coll.replace_one({"_id": doc["_id"]}, doc, upsert=True) return doc["_id"] - res = self._coll.insert_one(doc) # type: ignore + res = self._coll.insert_one(doc) # type: ignore[arg-type] return res.inserted_id def _save( diff --git a/src/eduid/userdb/element.py b/src/eduid/userdb/element.py index afc707de1..d1594de40 100644 --- a/src/eduid/userdb/element.py +++ b/src/eduid/userdb/element.py @@ -402,7 +402,7 @@ def verified(self) -> list[ListElement]: verified_elements = [e for e in self.elements if isinstance(e, VerifiedElement) and e.is_verified] # mypy figures out the real type of `verified_elements' since isinstance() is used above and complains # error: Incompatible return value type (got "List[VerifiedElement]", expected "List[ListElement]") - return verified_elements # type: ignore + return verified_elements # type: ignore[return-value] class PrimaryElementList(VerifiedElementList[ListElement], Generic[ListElement], ABC): @@ -452,7 +452,7 @@ def primary(self) -> ListElement | None: # mypy figures out the real type of match since isinstance() is used above and complains # error: Incompatible return value type (got "PrimaryElement", expected "Optional[ListElement]") - return match # type: ignore + return match # type: ignore[return-value] def set_primary(self, key: ElementKey) -> None: """ @@ -486,7 +486,7 @@ def set_primary(self, key: ElementKey) -> None: # mypy figures out the real type of `new' since isinstance() is used above and complains # error: Incompatible types in assignment (expression has type "List[PrimaryElement]", # variable has type "List[ListElement]") - self.elements = new # type: ignore + self.elements = new # type: ignore[assignment] @classmethod def _get_primary(cls, elements: list[ListElement]) -> ListElement | None: @@ -513,8 +513,8 @@ def _get_primary(cls, elements: list[ListElement]) -> ListElement | None: raise PrimaryElementViolation("Primary element is not verified") # mypy figures out the real type of `res[0]' since isinstance() is used above and complains - # error: Incompatible return value type (got "PrimaryElement", expected "Optional[ListElement]") - return res[0] # type: ignore + # error: Incompatible return value type (got "PrimaryElement", expected "ListElement | None") + return res[0] # type: ignore[return-value] def remove(self, key: ElementKey) -> None: """ diff --git a/src/eduid/webapp/authn/app.py b/src/eduid/webapp/authn/app.py index ef344552b..1590fefca 100644 --- a/src/eduid/webapp/authn/app.py +++ b/src/eduid/webapp/authn/app.py @@ -20,7 +20,7 @@ def __init__(self, config: AuthnConfig, **kwargs: Any) -> None: def get_current_app() -> AuthnApp: """Teach pycharm about AuthnApp""" - return current_app # type: ignore + return current_app # type: ignore[return-value] current_authn_app = get_current_app() diff --git a/src/eduid/webapp/common/api/decorators.py b/src/eduid/webapp/common/api/decorators.py index 48c6c3f5f..7476e6a38 100644 --- a/src/eduid/webapp/common/api/decorators.py +++ b/src/eduid/webapp/common/api/decorators.py @@ -245,6 +245,6 @@ def unmarshal_decorator( if isinstance(ret, FluxData): raise TypeError("Wrong order of decorators, UnmarshalWith must be the first decorator") # Uh, don't know how to check for Awaitable[FluxData], so for now we just ignore the type error below - return ret # type: ignore + return ret # type: ignore[return-value] return unmarshal_decorator diff --git a/src/eduid/webapp/common/authn/utils.py b/src/eduid/webapp/common/authn/utils.py index d325b683a..ef96c926d 100644 --- a/src/eduid/webapp/common/authn/utils.py +++ b/src/eduid/webapp/common/authn/utils.py @@ -38,7 +38,7 @@ def get_saml2_config(module_path: str, name: str = "SAML_CONFIG") -> SPConfig: if spec is None: raise RuntimeError(f"Failed loading saml2_settings module: {module_path}") module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) # type: ignore + spec.loader.exec_module(module) # type: ignore[union-attr] conf = SPConfig() conf.load(module.__getattribute__(name)) diff --git a/src/eduid/webapp/eidas/proofing.py b/src/eduid/webapp/eidas/proofing.py index 9a0e3e598..03a08aa99 100644 --- a/src/eduid/webapp/eidas/proofing.py +++ b/src/eduid/webapp/eidas/proofing.py @@ -116,7 +116,7 @@ def _match_identity_for_mfa( current_app.logger.debug( f"Current identity unique value: {current_unique_value}. Asserted unique value: {asserted_unique_value}" ) - current_app.logger.debug(f"Asserted attributes: {self.session_info.attributes}") # type: ignore + current_app.logger.debug(f"Asserted attributes: {self.session_info.attributes}") # type: ignore[attr-defined] return MatchResult(matched=mfa_success, credential_used=credential_used) diff --git a/src/eduid/workers/am/tests/test_index.py b/src/eduid/workers/am/tests/test_index.py index ebd0ed936..fa106b68d 100644 --- a/src/eduid/workers/am/tests/test_index.py +++ b/src/eduid/workers/am/tests/test_index.py @@ -24,7 +24,7 @@ def test_index_setup(self) -> None: "mobile-index-v1": {"key": [("mobile.mobile", 1), ("mobile.verified", 1)]}, "mailAliases-index-v1": {"key": [("mailAliases.email", 1), ("mailAliases.verified", 1)]}, } - db = UserDB(self.settings.mongo_uri) # type: ignore + db = UserDB(self.settings.mongo_uri) # type: ignore[call-arg,var-annotated,attr-defined] print(db._coll.index_information()) db.setup_indexes(indexes) current_indexes = db._coll.index_information() From 0e06a97f05a9f30f2b7b216ad521a7b620ebdfab Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Sat, 7 Dec 2024 20:16:56 +0000 Subject: [PATCH 3/9] remove unused legacy_save --- src/eduid/userdb/db/sync_db.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/eduid/userdb/db/sync_db.py b/src/eduid/userdb/db/sync_db.py index fad8cd9af..ea21026ae 100644 --- a/src/eduid/userdb/db/sync_db.py +++ b/src/eduid/userdb/db/sync_db.py @@ -316,17 +316,6 @@ def setup_indexes(self, indexes: Mapping[str, Any]) -> None: params["name"] = name self._coll.create_index(key, **params) - def legacy_save(self, doc: dict[str, Any]) -> str: - """ - Only used in tests and should probably be removed when time allows. - pymongo removed the save method in version 4.0. - """ - if "_id" in doc: - self._coll.replace_one({"_id": doc["_id"]}, doc, upsert=True) - return doc["_id"] - res = self._coll.insert_one(doc) # type: ignore[arg-type] - return res.inserted_id - def _save( self, data: TUserDbDocument, From e1db2209b69d2b431227a010f44e42a56e072072 Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Sat, 7 Dec 2024 20:24:28 +0000 Subject: [PATCH 4/9] remove redundant response_model arguments --- src/eduid/vccs/server/endpoints/add_creds.py | 2 +- src/eduid/vccs/server/endpoints/authenticate.py | 4 ++-- src/eduid/vccs/server/endpoints/misc.py | 4 ++-- src/eduid/vccs/server/endpoints/revoke_creds.py | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/eduid/vccs/server/endpoints/add_creds.py b/src/eduid/vccs/server/endpoints/add_creds.py index 542cd1d8d..ff0042c77 100644 --- a/src/eduid/vccs/server/endpoints/add_creds.py +++ b/src/eduid/vccs/server/endpoints/add_creds.py @@ -28,7 +28,7 @@ class AddCredsFormResponse(BaseModel): add_creds_response: AddCredsResponseV1 -@add_creds_router.post("/add_creds", response_model=AddCredsFormResponse) +@add_creds_router.post("/add_creds") async def add_creds_legacy(req: Request, request: str = Form(...)) -> AddCredsFormResponse: req.app.logger.debug(f"Add credentials (using form): {request}") diff --git a/src/eduid/vccs/server/endpoints/authenticate.py b/src/eduid/vccs/server/endpoints/authenticate.py index 5c54753b4..7ec8eb664 100644 --- a/src/eduid/vccs/server/endpoints/authenticate.py +++ b/src/eduid/vccs/server/endpoints/authenticate.py @@ -29,7 +29,7 @@ class AuthenticateFormResponse(BaseModel): auth_response: AuthenticateResponseV1 -@authenticate_router.post("/authenticate", response_model=AuthenticateFormResponse) +@authenticate_router.post("/authenticate") async def authenticate_legacy(req: Request, request: str = Form(...)) -> AuthenticateFormResponse: req.app.logger.debug(f"Authenticate (using form): {request}") @@ -48,7 +48,7 @@ class AuthenticateInnerRequest(BaseModel): return response -@authenticate_router.post("/v2/authenticate", response_model=AuthenticateResponseV1) +@authenticate_router.post("/v2/authenticate") async def authenticate(req: Request, request: AuthenticateRequestV1) -> AuthenticateResponseV1: """ Handle a password authentication request, along the following pseudo-code : diff --git a/src/eduid/vccs/server/endpoints/misc.py b/src/eduid/vccs/server/endpoints/misc.py index 1848b1426..31f037e9a 100644 --- a/src/eduid/vccs/server/endpoints/misc.py +++ b/src/eduid/vccs/server/endpoints/misc.py @@ -19,7 +19,7 @@ class StatusResponse(BaseModel): version: int = 1 -@misc_router.get("/status/healthy", response_model=StatusResponse) +@misc_router.get("/status/healthy") async def status(request: Request) -> StatusResponse: _test_keyhandle = request.app.state.config.add_creds_password_key_handle res = StatusResponse(status=Status.OK) @@ -42,7 +42,7 @@ class HMACResponse(BaseModel): hmac: str -@misc_router.get("/hmac/{keyhandle}/{data}", response_model=HMACResponse) +@misc_router.get("/hmac/{keyhandle}/{data}") async def hmac(request: Request, keyhandle: int, data: bytes) -> HMACResponse: hmac = await request.app.state.hasher.hmac_sha1(key_handle=keyhandle, data=data) return HMACResponse(keyhandle=keyhandle, hmac=hmac.hex()) diff --git a/src/eduid/vccs/server/endpoints/revoke_creds.py b/src/eduid/vccs/server/endpoints/revoke_creds.py index 93db930e7..60d049465 100644 --- a/src/eduid/vccs/server/endpoints/revoke_creds.py +++ b/src/eduid/vccs/server/endpoints/revoke_creds.py @@ -28,7 +28,7 @@ class RevokeCredsFormResponse(BaseModel): revoke_creds_response: RevokeCredsResponseV1 -@revoke_creds_router.post("/revoke_creds", response_model=RevokeCredsFormResponse) +@revoke_creds_router.post("/revoke_creds") async def revoke_creds_legacy(req: Request, request: str = Form(...)) -> RevokeCredsFormResponse: req.app.logger.debug(f"Revoke credentials (using form): {request}") From ea038955f9575bca79dac789b1a1603e31be7c05 Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Sat, 7 Dec 2024 20:28:35 +0000 Subject: [PATCH 5/9] follow recommendations from FastAPI documentation --- src/eduid/vccs/server/endpoints/add_creds.py | 3 ++- src/eduid/vccs/server/endpoints/authenticate.py | 3 ++- src/eduid/vccs/server/endpoints/revoke_creds.py | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/eduid/vccs/server/endpoints/add_creds.py b/src/eduid/vccs/server/endpoints/add_creds.py index ff0042c77..12e3ab5d1 100644 --- a/src/eduid/vccs/server/endpoints/add_creds.py +++ b/src/eduid/vccs/server/endpoints/add_creds.py @@ -1,4 +1,5 @@ import json +from typing import Annotated from fastapi import APIRouter, Form, Request from pydantic.main import BaseModel @@ -29,7 +30,7 @@ class AddCredsFormResponse(BaseModel): @add_creds_router.post("/add_creds") -async def add_creds_legacy(req: Request, request: str = Form(...)) -> AddCredsFormResponse: +async def add_creds_legacy(req: Request, request: Annotated[str, Form(...)]) -> AddCredsFormResponse: req.app.logger.debug(f"Add credentials (using form): {request}") class AddCredsInnerRequest(BaseModel): diff --git a/src/eduid/vccs/server/endpoints/authenticate.py b/src/eduid/vccs/server/endpoints/authenticate.py index 7ec8eb664..bc1a8d1b8 100644 --- a/src/eduid/vccs/server/endpoints/authenticate.py +++ b/src/eduid/vccs/server/endpoints/authenticate.py @@ -1,4 +1,5 @@ import json +from typing import Annotated from fastapi import APIRouter, Form, Request from pydantic.main import BaseModel @@ -30,7 +31,7 @@ class AuthenticateFormResponse(BaseModel): @authenticate_router.post("/authenticate") -async def authenticate_legacy(req: Request, request: str = Form(...)) -> AuthenticateFormResponse: +async def authenticate_legacy(req: Request, request: Annotated[str, Form(...)]) -> AuthenticateFormResponse: req.app.logger.debug(f"Authenticate (using form): {request}") class AuthenticateInnerRequest(BaseModel): diff --git a/src/eduid/vccs/server/endpoints/revoke_creds.py b/src/eduid/vccs/server/endpoints/revoke_creds.py index 60d049465..be9e4b314 100644 --- a/src/eduid/vccs/server/endpoints/revoke_creds.py +++ b/src/eduid/vccs/server/endpoints/revoke_creds.py @@ -1,4 +1,5 @@ import json +from typing import Annotated from fastapi import APIRouter, Form, Request from pydantic.main import BaseModel @@ -29,7 +30,7 @@ class RevokeCredsFormResponse(BaseModel): @revoke_creds_router.post("/revoke_creds") -async def revoke_creds_legacy(req: Request, request: str = Form(...)) -> RevokeCredsFormResponse: +async def revoke_creds_legacy(req: Request, request: Annotated[str, Form(...)]) -> RevokeCredsFormResponse: req.app.logger.debug(f"Revoke credentials (using form): {request}") class RevokeCredsInnerRequest(BaseModel): From f5fb17de22fcd0da11da0afd10f6382ddac5ef2f Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Sat, 7 Dec 2024 20:28:58 +0000 Subject: [PATCH 6/9] add FAST rules --- ruff.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruff.toml b/ruff.toml index 274d4ad7a..75cfbf326 100644 --- a/ruff.toml +++ b/ruff.toml @@ -3,7 +3,7 @@ line-length = 120 target-version = "py311" [lint] -select = ["E", "F", "W", "I", "ASYNC", "UP", "FLY", "PERF", "FURB", "ERA", "ANN", "PIE", "PL", "PGH"] +select = ["E", "F", "W", "I", "ASYNC", "UP", "FLY", "PERF", "FURB", "ERA", "ANN", "PIE", "PL", "PGH", "FAST"] ignore = ["E501", "PLR", "PLW"] From 5092aa2f02ec6f41eb44aaa72c6a990713cdfef2 Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Mon, 9 Dec 2024 19:44:42 +0000 Subject: [PATCH 7/9] use cast to remove an ignore --- src/eduid/webapp/authn/app.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/eduid/webapp/authn/app.py b/src/eduid/webapp/authn/app.py index 1590fefca..804572180 100644 --- a/src/eduid/webapp/authn/app.py +++ b/src/eduid/webapp/authn/app.py @@ -1,5 +1,5 @@ from collections.abc import Mapping -from typing import Any +from typing import Any, cast from flask import current_app @@ -20,7 +20,7 @@ def __init__(self, config: AuthnConfig, **kwargs: Any) -> None: def get_current_app() -> AuthnApp: """Teach pycharm about AuthnApp""" - return current_app # type: ignore[return-value] + return cast(AuthnApp, current_app) current_authn_app = get_current_app() From f3c252a1a361f739061023a2f6cb1585bcde1a6f Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Mon, 9 Dec 2024 20:07:44 +0000 Subject: [PATCH 8/9] use cast to satisfy mypy instead of ignoring type errors --- src/eduid/userdb/element.py | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/eduid/userdb/element.py b/src/eduid/userdb/element.py index d1594de40..77d736342 100644 --- a/src/eduid/userdb/element.py +++ b/src/eduid/userdb/element.py @@ -51,7 +51,7 @@ from collections.abc import Mapping from datetime import datetime from enum import Enum -from typing import Any, Generic, NewType, TypeVar +from typing import Any, Generic, NewType, TypeVar, cast from pydantic import BaseModel, ConfigDict, Field, field_validator @@ -399,10 +399,7 @@ def verified(self) -> list[ListElement]: Get all the verified elements in the ElementList. """ - verified_elements = [e for e in self.elements if isinstance(e, VerifiedElement) and e.is_verified] - # mypy figures out the real type of `verified_elements' since isinstance() is used above and complains - # error: Incompatible return value type (got "List[VerifiedElement]", expected "List[ListElement]") - return verified_elements # type: ignore[return-value] + return cast(list[ListElement], [e for e in self.elements if isinstance(e, VerifiedElement) and e.is_verified]) class PrimaryElementList(VerifiedElementList[ListElement], Generic[ListElement], ABC): @@ -450,9 +447,7 @@ def primary(self) -> ListElement | None: if not isinstance(match, PrimaryElement): raise UserDBValueError(f"Primary element {repr(match)} is not of type PrimaryElement") - # mypy figures out the real type of match since isinstance() is used above and complains - # error: Incompatible return value type (got "PrimaryElement", expected "Optional[ListElement]") - return match # type: ignore[return-value] + return cast(ListElement, match) def set_primary(self, key: ElementKey) -> None: """ @@ -483,10 +478,7 @@ def set_primary(self, key: ElementKey) -> None: raise UserDBValueError(f"Element {repr(this)} is not of type PrimaryElement") this.is_primary = bool(this.key == key) new += [this] - # mypy figures out the real type of `new' since isinstance() is used above and complains - # error: Incompatible types in assignment (expression has type "List[PrimaryElement]", - # variable has type "List[ListElement]") - self.elements = new # type: ignore[assignment] + self.elements = cast(list[ListElement], new) @classmethod def _get_primary(cls, elements: list[ListElement]) -> ListElement | None: @@ -512,9 +504,7 @@ def _get_primary(cls, elements: list[ListElement]) -> ListElement | None: if not primary.is_verified: raise PrimaryElementViolation("Primary element is not verified") - # mypy figures out the real type of `res[0]' since isinstance() is used above and complains - # error: Incompatible return value type (got "PrimaryElement", expected "ListElement | None") - return res[0] # type: ignore[return-value] + return cast(ListElement, res[0]) def remove(self, key: ElementKey) -> None: """ From c0453040a6cd03375879131e9fde73d9b2ec5861 Mon Sep 17 00:00:00 2001 From: Lasse Yledahl Date: Tue, 10 Dec 2024 13:26:51 +0000 Subject: [PATCH 9/9] use the better named variable for return --- src/eduid/userdb/element.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eduid/userdb/element.py b/src/eduid/userdb/element.py index 77d736342..e2a390b9b 100644 --- a/src/eduid/userdb/element.py +++ b/src/eduid/userdb/element.py @@ -504,7 +504,7 @@ def _get_primary(cls, elements: list[ListElement]) -> ListElement | None: if not primary.is_verified: raise PrimaryElementViolation("Primary element is not verified") - return cast(ListElement, res[0]) + return cast(ListElement, primary) def remove(self, key: ElementKey) -> None: """