AAP-68990 Switch from typeguard to mypy#965
Open
AlanCoding wants to merge 3 commits intoansible:develfrom
Open
AAP-68990 Switch from typeguard to mypy#965AlanCoding wants to merge 3 commits intoansible:develfrom
AlanCoding wants to merge 3 commits intoansible:develfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds MyPy static typing to CI and developer tooling, updates tests/settings to remove typeguard import-hook, and applies numerous explicit returns and Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(52,152,219,0.5)
Actor Developer
end
rect rgba(46,204,113,0.5)
Participant GitHubActions as CI
end
rect rgba(241,196,15,0.5)
Participant Runner
end
rect rgba(155,89,182,0.5)
Participant PackageManager as apt/pip
end
Developer->>CI: Push PR with mypy config & Makefile change
CI->>Runner: Start common-tests matrix (includes api-mypy)
Runner->>PackageManager: Install system build deps for api-mypy
Runner->>PackageManager: pip install -r requirements/requirements_all.txt
Runner->>Runner: make check_mypy (runs tox -e mypy)
Runner->>PackageManager: tox env installs mypy & stubs
Runner->>Runner: mypy analyzes ansible_base
Runner-->>CI: Upload mypy results/status
CI-->>Developer: Report pass/fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ansible_base/resource_registry/registry.py (1)
112-123:⚠️ Potential issue | 🟠 MajorReturn
Noneinstead ofFalseand change function return type toOptional[ResourceRegistry].The function is annotated to return
ResourceRegistrybut returnsFalsewhen the config module is missing, with only a# type: ignore[return-value]suppressing the type error. This causes runtimeAttributeErrorat unguarded call sites:
ansible_base/resource_registry/views.py:175— directly accesses.api_config.service_typeansible_base/resource_registry/utils/resource_type_serializers.py:98— directly calls.api_config.get_processor()ansible_base/resource_registry/tasks/sync.py:411— directly calls.get_resources()ansible_base/rbac/models/role.py:373— directly accesses.registryProposed fix
-def get_registry() -> ResourceRegistry: +def get_registry() -> Optional[ResourceRegistry]:else: - return False # type: ignore[return-value] + return NoneChanging the return type to
Optional[ResourceRegistry]and returningNoneforces callers to explicitly guard against the None case, preventing silent runtime failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/resource_registry/registry.py` around lines 112 - 123, get_registry currently returns False when settings.ANSIBLE_BASE_RESOURCE_CONFIG_MODULE is missing while its signature claims to return ResourceRegistry; change the function signature to return Optional[ResourceRegistry] and replace the final "return False" with "return None", keeping the existing import_string/resource_list/api_config logic intact; then search uses of get_registry (e.g., callers that access .api_config, .get_resources, .registry) and update those call sites to explicitly guard for None (raise a clear error or handle the no-config case) so they no longer assume a ResourceRegistry is always returned.
🧹 Nitpick comments (8)
ansible_base/oauth2_provider/utils.py (1)
10-10: Consider using structural typing instead of suppressingvalid-typeThe
# type: ignore[valid-type]on line 10 bypasses type checking for the function parameter. Instead of suppressing the check, define a lightweightProtocolfor the required contract—specifically theauthenticator_usersattribute the function depends on. This maintains type safety at the boundary without suppressing validation.Suggested refactoring
-from typing import Optional +from typing import Optional, Protocol from django.contrib.auth import get_user_model from ansible_base.authentication.models import Authenticator User = get_user_model() + +class SupportsAuthenticatorUsers(Protocol): + authenticator_users: object + -def is_external_account(user: "User") -> Optional[Authenticator]: # type: ignore[valid-type] +def is_external_account(user: SupportsAuthenticatorUsers) -> Optional[Authenticator]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/oauth2_provider/utils.py` at line 10, The function is_external_account currently uses a suppressed type check; replace the ignore by defining a small Protocol (e.g., UserWithAuthenticators) that declares the attribute authenticator_users (and its element type if needed) and use that Protocol as the parameter type instead of "User"; update the function signature def is_external_account(user: UserWithAuthenticators) -> Optional[Authenticator] and adjust imports (from typing import Protocol) so mypy can verify the required attribute without suppressing valid-type.ansible_base/lib/redis/client.py (2)
273-273: Avoid index-ignore by guardingcluster_infoshape
# type: ignore[index]suppresses useful checks. A small guard (or local typed variable) keeps behavior the same while preserving type safety and readability.Suggested direction
- if response['cluster_info']['cluster_state'] != _REDIS_CLUSTER_OK_STATUS: # type: ignore[index] + cluster_info = response.get('cluster_info') + if not isinstance(cluster_info, dict) or cluster_info.get('cluster_state') != _REDIS_CLUSTER_OK_STATUS: response['status'] = STATUS_FAILED🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/lib/redis/client.py` at line 273, The comparison currently uses a type-ignore on response['cluster_info']; instead, guard the shape: retrieve and type the cluster_info (e.g., cluster_info = response.get('cluster_info')), assert it's a mapping/object and that 'cluster_state' exists, then compare cluster_info['cluster_state'] to _REDIS_CLUSTER_OK_STATUS; remove the "# type: ignore[index]" and ensure that if cluster_info is missing or not the expected type the code treats it the same as a non-OK state (or handles it explicitly) so behavior remains unchanged while preserving type safety for response and cluster_info.
43-43: Replace broad return-ignore with a typed return path
# type: ignore[return]hides a real contract mismatch and makes future refactors harder. Prefer an explicit return strategy (or a narrower override-specific suppression) so MyPy still checks this method body.Suggested direction
- def flushdb(self, asynchronous: Optional[bool] = None, **kwargs) -> ResponseT: # type: ignore[return] + def flushdb(self, asynchronous: Optional[bool] = None, **kwargs) -> ResponseT: if asynchronous is not None: logger.warning("DABRedis clients implement an ACL friendly FLUSHDB which can not be async") @@ - if all_keys: + if all_keys: # Only attempt to delete keys if we got some otherwise we will get an exception from the delete function try: - self.delete(*all_keys) + return self.delete(*all_keys) except NoPermissionError: - pass + return None + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/lib/redis/client.py` at line 43, The method flushdb currently suppresses a return-type mismatch with "# type: ignore[return]"; remove that ignore and make the method satisfy the declared return type ResponseT by returning an explicitly-typed value (or a cast) instead of silencing MyPy. In practice, update flushdb to return typing.cast(ResponseT, <actual response>) where <actual response> is the existing call you perform to execute the FLUSHDB command (e.g. the result of self.execute_command or similar), import typing.cast, and only use a narrower suppression (e.g. # type: ignore[assignment]) if you cannot avoid a temporary mismatch. Ensure the function signature def flushdb(self, asynchronous: Optional[bool] = None, **kwargs) -> ResponseT stays unchanged and reference flushdb and ResponseT in the fix.ansible_base/resource_registry/service_client.py (1)
88-90: Replace the return-value ignore with an explicit post-refresh invariant.
jwtis declared-> str; prefer proving that invariant instead of suppressing it.Proposed fix
if self._jwt is None or self._jwt_timeout is None or time.time() >= self._jwt_timeout: self.refresh_jwt() - return self._jwt # type: ignore[return-value] + assert self._jwt is not None + return self._jwtAs per coding guidelines,
**: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/resource_registry/service_client.py` around lines 88 - 90, The code currently suppresses the return-value type-check on self._jwt; instead ensure the post-refresh invariant explicitly: after calling refresh_jwt() (referenced symbol refresh_jwt), assert or raise if self._jwt is still None (check self._jwt and self._jwt_timeout as needed) so the function that returns -> str can safely return self._jwt without a type ignore; replace the comment-suppressed return with an explicit assert or RuntimeError and then return self._jwt.requirements/requirements_dev.txt (1)
15-19: Pin mypy/stubs versions to keep type-check results stable across environments.Leaving these unpinned can cause non-deterministic lint failures as mypy/stub releases change behavior.
As per coding guidelines,
**: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements/requirements_dev.txt` around lines 15 - 19, The unpinned dev dependencies in requirements_dev.txt (mypy, django-stubs[compatible-mypy], djangorestframework-stubs[compatible-mypy], types-requests, types-tabulate) can cause non-deterministic type-check failures; update requirements_dev.txt to pin each package to a specific version (or a narrow range) to stabilize mypy results—e.g., change each package entry to a fixed version or a ~= / >= and < upper-bound pattern, and commit the chosen versions (optionally include pip hashes) so CI and local environments use identical stub/mypy releases.ansible_base/lib/utils/db.py (1)
225-226: Fix the DB settings type model instead of suppressing index typing.The ignore is compensating for
dj_db_dictbeing too narrow to represent DjangoDATABASESentries (notably nestedOPTIONS).Proposed fix
-from typing import Union +from typing import Any, Union @@ -dj_db_dict = dict[str, Union[str, int]] +dj_db_dict = dict[str, Any] @@ for k, v in settings_dict2.get('OPTIONS', {}).items(): - settings_dict['OPTIONS'][k] = v # type: ignore[index] + settings_dict['OPTIONS'][k] = vAs per coding guidelines,
**: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/lib/utils/db.py` around lines 225 - 226, The loop currently uses a type ignore because the TypedDict for database settings (dj_db_dict) is too narrow to represent nested OPTIONS; update the dj_db_dict/type annotation to include an 'OPTIONS' key (e.g., Optional[Mapping[str, Any]] or Dict[str, Any]) so it can represent Django DATABASES entries, then remove the "# type: ignore[index]" from the merge loop where settings_dict2 and settings_dict are combined (the code manipulating settings_dict2.get('OPTIONS', {}) and settings_dict['OPTIONS'][k] = v). Ensure the adjusted type is imported/defined where dj_db_dict is declared so the assignment is statically correct.ansible_base/authentication/utils/authenticator_map.py (1)
190-231: Consider fixing the root type annotation instead of multiple ignores.The function has
# type: ignore[return-value]on three separate return statements (lines 195, 205, 231). This suggests theerrorsdict initialization at line 191 should be typed explicitly:errors: dict[str, TranslatedString] = {}This would eliminate all three type ignores by ensuring the dict has the correct type from initialization.
♻️ Proposed fix
def check_role_type(map_type: Optional[str], role: Optional[str], org: Optional[str], team: Optional[str]) -> dict[str, TranslatedString]: - errors = {} + errors: dict[str, TranslatedString] = {} if not _is_rbac_installed(): errors['role'] = _("You specified a role without RBAC installed ") - return errors # type: ignore[return-value] + return errorsAnd similarly remove the other
# type: ignore[return-value]comments on lines 205 and 231.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/authentication/utils/authenticator_map.py` around lines 190 - 231, The function check_role_type initializes errors without an explicit type which led to multiple "# type: ignore[return-value]" comments on its return statements; to fix this, add an explicit annotation for errors (e.g., declare errors as dict[str, TranslatedString]) at its initialization and then remove the three "# type: ignore[return-value]" comments associated with the returns in check_role_type and ensure no other returns need ignores.ansible_base/authentication/authenticator_plugins/ldap.py (1)
469-469: Consider replacing setattr with direct assignment.Static analysis flags that
setattrwith a constant attribute name offers no benefit over direct assignment.♻️ Optional refactor
- setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS'])) # type: ignore[operator] + self.GROUP_TYPE = group_type_class(**defaults['GROUP_TYPE_PARAMS']) # type: ignore[operator]Note: The
# type: ignore[operator]may still be needed if mypy complains about the call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/authentication/authenticator_plugins/ldap.py` at line 469, The code uses setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS'])) in the initializer — replace this with a direct attribute assignment on the instance (self.GROUP_TYPE = group_type_class(**defaults['GROUP_TYPE_PARAMS'])) to satisfy static analysis; retain the existing # type: ignore[operator] on the call if mypy still complains about the instantiation. Use the same symbols (GROUP_TYPE, group_type_class, defaults['GROUP_TYPE_PARAMS']) so the change is localized and preserves behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/authentication/authenticator_plugins/ldap.py`:
- Around line 485-487: The authenticate method's return type annotation
(tuple[object, dict, list] | None) is incorrect: update_user_claims returns
Optional[AbstractUser], and other plugins (local.py, tacacs.py, radius.py) use
Optional[AbstractUser]; change the authenticate signature to return
Optional[AbstractUser] (or remove the annotation), update any related imports to
include AbstractUser if missing, and remove the now-unnecessary "# type:
ignore[return-value]" comment near update_user_claims to let the type checker
validate the correct return type.
In `@ansible_base/resource_registry/resource_server.py`:
- Around line 17-20: The function get_resource_server_config currently returns a
merged dict with a type ignore, masking missing required keys; update it to
validate that the resulting object conforms to ResourceServerConfig by checking
required fields (at minimum "URL" and "SECRET_KEY") after
defaults.update(settings.RESOURCE_SERVER), and raise a clear exception (or
return a properly typed ResourceServerConfig instance) if any required keys are
missing or invalid; remove the type: ignore and ensure callers like
get_resource_server_client and get_service_token can rely on those keys being
present.
---
Outside diff comments:
In `@ansible_base/resource_registry/registry.py`:
- Around line 112-123: get_registry currently returns False when
settings.ANSIBLE_BASE_RESOURCE_CONFIG_MODULE is missing while its signature
claims to return ResourceRegistry; change the function signature to return
Optional[ResourceRegistry] and replace the final "return False" with "return
None", keeping the existing import_string/resource_list/api_config logic intact;
then search uses of get_registry (e.g., callers that access .api_config,
.get_resources, .registry) and update those call sites to explicitly guard for
None (raise a clear error or handle the no-config case) so they no longer assume
a ResourceRegistry is always returned.
---
Nitpick comments:
In `@ansible_base/authentication/authenticator_plugins/ldap.py`:
- Line 469: The code uses setattr(self, 'GROUP_TYPE',
group_type_class(**defaults['GROUP_TYPE_PARAMS'])) in the initializer — replace
this with a direct attribute assignment on the instance (self.GROUP_TYPE =
group_type_class(**defaults['GROUP_TYPE_PARAMS'])) to satisfy static analysis;
retain the existing # type: ignore[operator] on the call if mypy still complains
about the instantiation. Use the same symbols (GROUP_TYPE, group_type_class,
defaults['GROUP_TYPE_PARAMS']) so the change is localized and preserves
behavior.
In `@ansible_base/authentication/utils/authenticator_map.py`:
- Around line 190-231: The function check_role_type initializes errors without
an explicit type which led to multiple "# type: ignore[return-value]" comments
on its return statements; to fix this, add an explicit annotation for errors
(e.g., declare errors as dict[str, TranslatedString]) at its initialization and
then remove the three "# type: ignore[return-value]" comments associated with
the returns in check_role_type and ensure no other returns need ignores.
In `@ansible_base/lib/redis/client.py`:
- Line 273: The comparison currently uses a type-ignore on
response['cluster_info']; instead, guard the shape: retrieve and type the
cluster_info (e.g., cluster_info = response.get('cluster_info')), assert it's a
mapping/object and that 'cluster_state' exists, then compare
cluster_info['cluster_state'] to _REDIS_CLUSTER_OK_STATUS; remove the "# type:
ignore[index]" and ensure that if cluster_info is missing or not the expected
type the code treats it the same as a non-OK state (or handles it explicitly) so
behavior remains unchanged while preserving type safety for response and
cluster_info.
- Line 43: The method flushdb currently suppresses a return-type mismatch with
"# type: ignore[return]"; remove that ignore and make the method satisfy the
declared return type ResponseT by returning an explicitly-typed value (or a
cast) instead of silencing MyPy. In practice, update flushdb to return
typing.cast(ResponseT, <actual response>) where <actual response> is the
existing call you perform to execute the FLUSHDB command (e.g. the result of
self.execute_command or similar), import typing.cast, and only use a narrower
suppression (e.g. # type: ignore[assignment]) if you cannot avoid a temporary
mismatch. Ensure the function signature def flushdb(self, asynchronous:
Optional[bool] = None, **kwargs) -> ResponseT stays unchanged and reference
flushdb and ResponseT in the fix.
In `@ansible_base/lib/utils/db.py`:
- Around line 225-226: The loop currently uses a type ignore because the
TypedDict for database settings (dj_db_dict) is too narrow to represent nested
OPTIONS; update the dj_db_dict/type annotation to include an 'OPTIONS' key
(e.g., Optional[Mapping[str, Any]] or Dict[str, Any]) so it can represent Django
DATABASES entries, then remove the "# type: ignore[index]" from the merge loop
where settings_dict2 and settings_dict are combined (the code manipulating
settings_dict2.get('OPTIONS', {}) and settings_dict['OPTIONS'][k] = v). Ensure
the adjusted type is imported/defined where dj_db_dict is declared so the
assignment is statically correct.
In `@ansible_base/oauth2_provider/utils.py`:
- Line 10: The function is_external_account currently uses a suppressed type
check; replace the ignore by defining a small Protocol (e.g.,
UserWithAuthenticators) that declares the attribute authenticator_users (and its
element type if needed) and use that Protocol as the parameter type instead of
"User"; update the function signature def is_external_account(user:
UserWithAuthenticators) -> Optional[Authenticator] and adjust imports (from
typing import Protocol) so mypy can verify the required attribute without
suppressing valid-type.
In `@ansible_base/resource_registry/service_client.py`:
- Around line 88-90: The code currently suppresses the return-value type-check
on self._jwt; instead ensure the post-refresh invariant explicitly: after
calling refresh_jwt() (referenced symbol refresh_jwt), assert or raise if
self._jwt is still None (check self._jwt and self._jwt_timeout as needed) so the
function that returns -> str can safely return self._jwt without a type ignore;
replace the comment-suppressed return with an explicit assert or RuntimeError
and then return self._jwt.
In `@requirements/requirements_dev.txt`:
- Around line 15-19: The unpinned dev dependencies in requirements_dev.txt
(mypy, django-stubs[compatible-mypy],
djangorestframework-stubs[compatible-mypy], types-requests, types-tabulate) can
cause non-deterministic type-check failures; update requirements_dev.txt to pin
each package to a specific version (or a narrow range) to stabilize mypy
results—e.g., change each package entry to a fixed version or a ~= / >= and <
upper-bound pattern, and commit the chosen versions (optionally include pip
hashes) so CI and local environments use identical stub/mypy releases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ebc6b86-845f-4ed3-998e-a811fdbb4de7
📒 Files selected for processing (34)
.github/workflows/linting.ymlMakefileansible_base/activitystream/serializers.pyansible_base/authentication/authenticator_plugins/ldap.pyansible_base/authentication/models/authenticator.pyansible_base/authentication/models/authenticator_map.pyansible_base/authentication/models/authenticator_user.pyansible_base/authentication/utils/authentication.pyansible_base/authentication/utils/authenticator_map.pyansible_base/authentication/utils/claims.pyansible_base/jwt_consumer/common/util.pyansible_base/lib/channels/middleware.pyansible_base/lib/redis/client.pyansible_base/lib/routers/association_resource_router.pyansible_base/lib/utils/create_system_user.pyansible_base/lib/utils/db.pyansible_base/oauth2_provider/utils.pyansible_base/rbac/claims.pyansible_base/rbac/managed.pyansible_base/rbac/models/content_type.pyansible_base/rbac/models/role.pyansible_base/rbac/permission_registry.pyansible_base/rbac/validators.pyansible_base/resource_registry/registry.pyansible_base/resource_registry/resource_server.pyansible_base/resource_registry/service_client.pyansible_base/resource_registry/utils/resource_type_processor.pypyproject.tomlrequirements/requirements_dev.txttest_app/settings.pytest_app/tests/authentication/authenticator_plugins/test_ldap.pytest_app/tests/lib/utils/test_duration.pytest_app/tests/lib/utils/test_hashing.pytest_app/tests/lib/utils/test_validation.py
💤 Files with no reviewable changes (4)
- test_app/tests/lib/utils/test_hashing.py
- test_app/settings.py
- test_app/tests/lib/utils/test_validation.py
- test_app/tests/authentication/authenticator_plugins/test_ldap.py
Comment on lines
17
to
+20
| def get_resource_server_config() -> ResourceServerConfig: | ||
| defaults = {"JWT_ALGORITHM": "HS256", "VALIDATE_HTTPS": True} | ||
| defaults.update(settings.RESOURCE_SERVER) | ||
| return defaults | ||
| return defaults # type: ignore[return-value] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the ResourceServerConfig definition
find . -type f -name "*.py" | head -20Repository: ansible/django-ansible-base
Length of output: 1100
🏁 Script executed:
# Search for ResourceServerConfig TypedDict definition
rg "ResourceServerConfig" -A 10 -B 2 --max-count 5Repository: ansible/django-ansible-base
Length of output: 1588
🏁 Script executed:
# Look at the specific file in question
cat -n ansible_base/resource_registry/resource_server.pyRepository: ansible/django-ansible-base
Length of output: 1268
🏁 Script executed:
# Search for how settings.RESOURCE_SERVER is used
rg "RESOURCE_SERVER" -B 2 -A 2Repository: ansible/django-ansible-base
Length of output: 21741
🏁 Script executed:
# Search for get_service_token to see what keys it expects
rg "get_service_token" -B 3 -A 10 --max-count 5Repository: ansible/django-ansible-base
Length of output: 7064
🏁 Script executed:
# Search for usage of config["URL"] in resource_server.py and related files
rg 'config\["URL"\]|config\.get\("URL"' -B 2 -A 2Repository: ansible/django-ansible-base
Length of output: 879
🏁 Script executed:
# Find all usages of get_resource_server_config
rg 'get_resource_server_config' -B 1 -A 3Repository: ansible/django-ansible-base
Length of output: 8659
🏁 Script executed:
# Check the rest_client to see how it uses the config
rg 'rest_client' -A 5 -B 5 | head -100Repository: ansible/django-ansible-base
Length of output: 9909
🏁 Script executed:
# Look at rest_client.py specifically
find . -name "rest_client.py" -path "*/resource_registry/*"Repository: ansible/django-ansible-base
Length of output: 118
🏁 Script executed:
# Check if rest_client uses get_resource_server_config
rg -l "get_resource_server_config|ResourceServerConfig" --type pyRepository: ansible/django-ansible-base
Length of output: 515
🏁 Script executed:
# Verify cast is available in typing module
python3 << 'EOF'
from typing import cast
print("cast import works:", cast)
EOFRepository: ansible/django-ansible-base
Length of output: 123
🏁 Script executed:
# Verify ImproperlyConfigured is available
python3 << 'EOF'
from django.core.exceptions import ImproperlyConfigured
print("ImproperlyConfigured import works:", ImproperlyConfigured)
EOFRepository: ansible/django-ansible-base
Length of output: 189
🏁 Script executed:
# Check rest_client.py to see the full context of how URL is used
sed -n '1,50p' ansible_base/resource_registry/rest_client.pyRepository: ansible/django-ansible-base
Length of output: 1517
Validate required ResourceServerConfig keys instead of suppressing type mismatch.
The type: ignore masks a real config-shape risk. URL and SECRET_KEY are accessed by get_resource_server_client() and get_service_token() respectively; missing keys defer failure to runtime (KeyError) instead of failing early and clearly at initialization.
Proposed fix
-from typing import TypedDict
+from typing import TypedDict, cast
+from django.core.exceptions import ImproperlyConfigured
def get_resource_server_config() -> ResourceServerConfig:
- defaults = {"JWT_ALGORITHM": "HS256", "VALIDATE_HTTPS": True}
- defaults.update(settings.RESOURCE_SERVER)
- return defaults # type: ignore[return-value]
+ merged = {"JWT_ALGORITHM": "HS256", "VALIDATE_HTTPS": True}
+ merged.update(settings.RESOURCE_SERVER)
+ missing = {"URL", "SECRET_KEY"} - set(merged.keys())
+ if missing:
+ raise ImproperlyConfigured(f"RESOURCE_SERVER missing required keys: {sorted(missing)}")
+ return cast(ResourceServerConfig, merged)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible_base/resource_registry/resource_server.py` around lines 17 - 20, The
function get_resource_server_config currently returns a merged dict with a type
ignore, masking missing required keys; update it to validate that the resulting
object conforms to ResourceServerConfig by checking required fields (at minimum
"URL" and "SECRET_KEY") after defaults.update(settings.RESOURCE_SERVER), and
raise a clear exception (or return a properly typed ResourceServerConfig
instance) if any required keys are missing or invalid; remove the type: ignore
and ensure callers like get_resource_server_client and get_service_token can
rely on those keys being present.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ansible_base/resource_registry/resource_server.py (1)
21-25:⚠️ Potential issue | 🟠 MajorUse
ImproperlyConfiguredand remove the return-value suppression.At Line 23, raising
KeyErrormakes a settings misconfiguration look like a generic runtime dict failure; use a Django config error instead. At Line 25,# type: ignore[return-value]bypasses mypy in the exact function meant to provide typed config.Proposed fix
-from typing import TypedDict +from typing import TypedDict, cast import jwt from django.conf import settings +from django.core.exceptions import ImproperlyConfigured @@ def get_resource_server_config() -> ResourceServerConfig: defaults = {"JWT_ALGORITHM": "HS256", "VALIDATE_HTTPS": True} defaults.update(settings.RESOURCE_SERVER) for key in ("URL", "SECRET_KEY"): if key not in defaults: - raise KeyError(f"RESOURCE_SERVER setting is missing required key: {key}") + raise ImproperlyConfigured(f"RESOURCE_SERVER setting is missing required key: {key}") - return defaults # type: ignore[return-value] + return cast(ResourceServerConfig, defaults)As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/resource_registry/resource_server.py` around lines 21 - 25, The code in resource_server.py currently raises KeyError when required settings are missing and suppresses mypy with "# type: ignore[return-value]"; replace KeyError with django.core.exceptions.ImproperlyConfigured (import ImproperlyConfigured) in the validation loop that checks ("URL", "SECRET_KEY") and remove the return-value suppression so the function's return type is accurate—ensure the function (the one returning `defaults`) has a proper typed return and that callers/annotations reflect that type instead of silencing type checking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/authentication/authenticator_plugins/ldap.py`:
- Line 469: The assignment to GROUP_TYPE uses group_type_class(...) without
checking that find_class_in_modules returned a valid, callable class; remove the
"# type: ignore[operator]" and guard the call by verifying group_type_class is
not None and is callable (e.g., using if not group_type_class: or
callable(group_type_class)), then either raise a clear error or fallback before
calling it; update the code around GROUP_TYPE, group_type_class, and any use of
find_class_in_modules to perform this runtime check and handle the failure path
instead of suppressing the type-checker warning.
---
Duplicate comments:
In `@ansible_base/resource_registry/resource_server.py`:
- Around line 21-25: The code in resource_server.py currently raises KeyError
when required settings are missing and suppresses mypy with "# type:
ignore[return-value]"; replace KeyError with
django.core.exceptions.ImproperlyConfigured (import ImproperlyConfigured) in the
validation loop that checks ("URL", "SECRET_KEY") and remove the return-value
suppression so the function's return type is accurate—ensure the function (the
one returning `defaults`) has a proper typed return and that callers/annotations
reflect that type instead of silencing type checking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2b94b4c-aace-42b5-b77b-069d562c7cfa
📒 Files selected for processing (3)
ansible_base/authentication/authenticator_plugins/ldap.pyansible_base/lib/redis/client.pyansible_base/resource_registry/resource_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ansible_base/lib/redis/client.py
| # Group type needs to be an object instead of a String so instantiate it | ||
| group_type_class = find_class_in_modules(defaults["GROUP_TYPE"]) | ||
| setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS'])) | ||
| setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS'])) # type: ignore[operator] |
Contributor
There was a problem hiding this comment.
Do not suppress a callable/type-safety failure here.
find_class_in_modules(...) can return None, so group_type_class(...) has a real runtime failure path. # type: ignore[operator] hides that instead of handling it. Please guard for None (or non-callable) and remove the ignore.
Proposed fix
- group_type_class = find_class_in_modules(defaults["GROUP_TYPE"])
- setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS'])) # type: ignore[operator]
+ group_type_class = find_class_in_modules(defaults["GROUP_TYPE"])
+ if group_type_class is None:
+ raise ValueError(f"Unknown LDAP GROUP_TYPE: {defaults['GROUP_TYPE']}")
+ self.GROUP_TYPE = group_type_class(**defaults['GROUP_TYPE_PARAMS'])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS'])) # type: ignore[operator] | |
| group_type_class = find_class_in_modules(defaults["GROUP_TYPE"]) | |
| if group_type_class is None: | |
| raise ValueError(f"Unknown LDAP GROUP_TYPE: {defaults['GROUP_TYPE']}") | |
| self.GROUP_TYPE = group_type_class(**defaults['GROUP_TYPE_PARAMS']) |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 469-469: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible_base/authentication/authenticator_plugins/ldap.py` at line 469, The
assignment to GROUP_TYPE uses group_type_class(...) without checking that
find_class_in_modules returned a valid, callable class; remove the "# type:
ignore[operator]" and guard the call by verifying group_type_class is not None
and is callable (e.g., using if not group_type_class: or
callable(group_type_class)), then either raise a clear error or fallback before
calling it; update the code around GROUP_TYPE, group_type_class, and any use of
find_class_in_modules to perform this runtime check and handle the failure path
instead of suppressing the type-checker warning.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit f7f7914. Configure here.
|
|
||
| for key in ("URL", "SECRET_KEY"): | ||
| if key not in defaults: | ||
| raise KeyError(f"RESOURCE_SERVER setting is missing required key: {key}") |
There was a problem hiding this comment.
New validation adds functional behavior change to refactoring PR
Low Severity
get_resource_server_config() now raises KeyError when URL or SECRET_KEY are missing from the RESOURCE_SERVER setting. Previously, the function would silently return an incomplete dict, and the error would surface later at the point of use (e.g., config["SECRET_KEY"] in get_service_token). While this is a defensively good change, it's a behavioral change in a PR explicitly marked as "refactoring (no functional changes)." Any caller that previously caught the downstream KeyError or handled partial configs could now see a different error message and traceback origin.
Reviewed by Cursor Bugbot for commit f7f7914. Configure here.
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ansible_base/authentication/authenticator_plugins/ldap.py (1)
469-469:⚠️ Potential issue | 🟠 MajorGuard
GROUP_TYPEinstantiation instead of suppressing operator typing.Line 469 still has a real runtime failure path if
find_class_in_modules(...)returnsNone; the# type: ignore[operator]hides that.Proposed fix
# Group type needs to be an object instead of a String so instantiate it group_type_class = find_class_in_modules(defaults["GROUP_TYPE"]) - setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS'])) # type: ignore[operator] + if group_type_class is None or not callable(group_type_class): + raise ValueError(f"Unknown LDAP GROUP_TYPE: {defaults['GROUP_TYPE']}") + self.GROUP_TYPE = group_type_class(**defaults['GROUP_TYPE_PARAMS'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/authentication/authenticator_plugins/ldap.py` at line 469, The line setting GROUP_TYPE should guard against find_class_in_modules(...) returning None instead of silencing the operator type error; update the code around the GROUP_TYPE assignment (i.e., the call that resolves group_type_class and the setattr(self, 'GROUP_TYPE', group_type_class(**defaults['GROUP_TYPE_PARAMS']))) to check that group_type_class is not None, and if it is, raise a clear exception or fallback before attempting to instantiate with defaults['GROUP_TYPE_PARAMS']; this removes the need for the "# type: ignore[operator]" and prevents a runtime TypeError when group_type_class is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ansible_base/authentication/authenticator_plugins/ldap.py`:
- Line 469: The line setting GROUP_TYPE should guard against
find_class_in_modules(...) returning None instead of silencing the operator type
error; update the code around the GROUP_TYPE assignment (i.e., the call that
resolves group_type_class and the setattr(self, 'GROUP_TYPE',
group_type_class(**defaults['GROUP_TYPE_PARAMS']))) to check that
group_type_class is not None, and if it is, raise a clear exception or fallback
before attempting to instantiate with defaults['GROUP_TYPE_PARAMS']; this
removes the need for the "# type: ignore[operator]" and prevents a runtime
TypeError when group_type_class is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc09cf74-df84-459d-a1c6-c4818eb2950d
📒 Files selected for processing (34)
.github/workflows/linting.ymlMakefileansible_base/activitystream/serializers.pyansible_base/authentication/authenticator_plugins/ldap.pyansible_base/authentication/models/authenticator.pyansible_base/authentication/models/authenticator_map.pyansible_base/authentication/models/authenticator_user.pyansible_base/authentication/utils/authentication.pyansible_base/authentication/utils/authenticator_map.pyansible_base/authentication/utils/claims.pyansible_base/jwt_consumer/common/util.pyansible_base/lib/channels/middleware.pyansible_base/lib/redis/client.pyansible_base/lib/routers/association_resource_router.pyansible_base/lib/utils/create_system_user.pyansible_base/lib/utils/db.pyansible_base/oauth2_provider/utils.pyansible_base/rbac/claims.pyansible_base/rbac/managed.pyansible_base/rbac/models/content_type.pyansible_base/rbac/models/role.pyansible_base/rbac/permission_registry.pyansible_base/rbac/validators.pyansible_base/resource_registry/registry.pyansible_base/resource_registry/resource_server.pyansible_base/resource_registry/service_client.pyansible_base/resource_registry/utils/resource_type_processor.pypyproject.tomlrequirements/requirements_dev.txttest_app/settings.pytest_app/tests/authentication/authenticator_plugins/test_ldap.pytest_app/tests/lib/utils/test_duration.pytest_app/tests/lib/utils/test_hashing.pytest_app/tests/lib/utils/test_validation.py
💤 Files with no reviewable changes (4)
- test_app/tests/lib/utils/test_hashing.py
- test_app/tests/lib/utils/test_validation.py
- test_app/tests/authentication/authenticator_plugins/test_ldap.py
- test_app/settings.py
✅ Files skipped from review due to trivial changes (18)
- ansible_base/rbac/managed.py
- ansible_base/lib/channels/middleware.py
- ansible_base/resource_registry/registry.py
- ansible_base/jwt_consumer/common/util.py
- ansible_base/authentication/utils/authentication.py
- ansible_base/lib/utils/db.py
- ansible_base/activitystream/serializers.py
- ansible_base/rbac/claims.py
- ansible_base/rbac/models/content_type.py
- ansible_base/resource_registry/service_client.py
- ansible_base/resource_registry/utils/resource_type_processor.py
- ansible_base/authentication/models/authenticator_user.py
- test_app/tests/lib/utils/test_duration.py
- .github/workflows/linting.yml
- ansible_base/oauth2_provider/utils.py
- ansible_base/authentication/models/authenticator_map.py
- ansible_base/lib/utils/create_system_user.py
- ansible_base/rbac/validators.py
🚧 Files skipped from review as they are similar to previous changes (10)
- ansible_base/resource_registry/resource_server.py
- Makefile
- ansible_base/lib/routers/association_resource_router.py
- ansible_base/authentication/models/authenticator.py
- requirements/requirements_dev.txt
- ansible_base/authentication/utils/claims.py
- ansible_base/rbac/models/role.py
- ansible_base/rbac/permission_registry.py
- ansible_base/authentication/utils/authenticator_map.py
- ansible_base/lib/redis/client.py
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Obviously, mostly Claude here.
I added typeguard a long time ago. It was a creative idea, but the overall ecosystem has moved much further in the direction of static analysis and away from runtime checking.
To remain agile, we should be as consistent as possible, and typeguard was fighting against the current. It will be much easier to onboard people to DAB if we're on mypy, which this will do.
Type of Change
Self-Review Checklist
Note
Medium Risk
Mostly adds static typing tooling and mypy-driven fixes, but it also tightens some runtime behavior (e.g.,
get_resource_server_confignow raises if required settings are missing), which could surface new errors in misconfigured deployments.Overview
Switches the project from runtime type checking to static analysis with
mypyby adding a dedicatedmypytox env/config, wiring it intomake check_*and the GitHub Actions lint matrix (including installing extra system/python deps for the mypy job), and updating dev requirements with mypy + Django/DRF stubs.To satisfy mypy, updates code across authentication/RBAC/redis/jwt/resource registry to be more explicit about
Nonereturns and to add targetedtype: ignore[...]annotations, and removestypeguard-based import hooks/suppressions fromtest_appsettings and tests. Also adds validation inget_resource_server_config()to requireRESOURCE_SERVER.URLandRESOURCE_SERVER.SECRET_KEY.Reviewed by Cursor Bugbot for commit 6a541b3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit