Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/audit/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def _get_organisation(self) -> Organisation | None:

Since we're applying the base filters to the query set
"""
return ( # type: ignore[no-any-return]
return (
self.request.user.get_admin_organisations() # type: ignore[union-attr]
.select_related("subscription", "subscription_information_cache")
.first()
Expand Down
2 changes: 1 addition & 1 deletion api/core/redis_cluster.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def get_connection(self, connection_params: dict) -> RedisCluster: # type: igno
settings.REDIS_CLUSTER_READ_FROM_REPLICAS
)
# ... and then build and return the client
return RedisCluster(**client_cls_kwargs) # type: ignore[abstract]
return RedisCluster(**client_cls_kwargs) # type: ignore[no-any-return]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

🔎 Supported by static analysis

🤖 get_repo_knowledge executed:

get_repo_knowledge Flagsmith/flagsmith /tmp/coderabbit-repo-knowledge/flagsmith-flagsmith-a74d157a/learnings /tmp/coderabbit-repo-knowledge/flagsmith-flagsmith-a74d157a/architecture

Length of output: 40238


🏁 Script executed:

#!/bin/bash
set -eu
for spec in \
  "api/core/redis_cluster.py:120:138" \
  "api/experimentation/ingestion_sync_service.py:1:30" \
  "api/features/feature_states/permissions.py:1:25" \
  "api/features/import_export/permissions.py:1:70" \
  "api/organisations/permissions/permissions.py:220:240"
do
  file=${spec%%:*}
  rest=${spec#*:}
  start=${rest%%:*}
  end=${rest##*:}
  printf '\n--- %s:%s-%s ---\n' "$file" "$start" "$end"
  sed -n "${start},${end}p" "$file" | nl -ba -v "$start"
done

printf '\n--- relevant typing configuration and local ignore guidance ---\n'
rg -n --glob '!*lock*' --glob '!*.min.*' \
  'type: ignore|disallow_any|warn_return_any|mypy|pyright|typing' \
  pyproject.toml setup.cfg tox.ini .pre-commit-config.yaml .github 2>/dev/null | head -200

Repository: Flagsmith/flagsmith

Length of output: 241


🏁 Script executed:

#!/bin/bash
set -eu
for spec in \
  "api/core/redis_cluster.py:120:138" \
  "api/experimentation/ingestion_sync_service.py:1:30" \
  "api/features/feature_states/permissions.py:1:25" \
  "api/features/import_export/permissions.py:1:70" \
  "api/organisations/permissions/permissions.py:220:240"
do
  file=${spec%%:*}
  rest=${spec#*:}
  start=${rest%%:*}
  end=${rest##*:}
  printf '\n--- %s:%s-%s ---\n' "$file" "$start" "$end"
  sed -n "${start},${end}p" "$file"
done

printf '\n--- relevant typing configuration and local ignore guidance ---\n'
rg -n --glob '!*lock*' --glob '!*.min.*' \
  'type: ignore|disallow_any|warn_return_any|mypy|pyright|typing' \
  pyproject.toml setup.cfg tox.ini .pre-commit-config.yaml .github 2>/dev/null | head -200

Repository: Flagsmith/flagsmith

Length of output: 6824


🤖 get_repo_knowledge executed:

get_repo_knowledge Flagsmith/flagsmith /tmp/coderabbit-repo-knowledge/flagsmith-flagsmith-a74d157a

Length of output: 860


Document the retained # type: ignore[...] directives.

Add a short reason or removal condition to each listed suppression. The no-any-return directives suppress Redis client return typing, and the union-attr directives suppress permission-call typing on request.user. The surrounding code does not document these typing limitations.

📍 Affects 5 files
  • api/core/redis_cluster.py#L133-L133 (this comment)
  • api/experimentation/ingestion_sync_service.py#L21-L21
  • api/features/feature_states/permissions.py#L17-L17
  • api/features/import_export/permissions.py#L22-L22
  • api/features/import_export/permissions.py#L31-L31
  • api/features/import_export/permissions.py#L41-L41
  • api/features/import_export/permissions.py#L52-L52
  • api/features/import_export/permissions.py#L63-L63
  • api/organisations/permissions/permissions.py#L233-L233

Source: Learnings

except Exception as e:
# Let django redis handle the exception
raise ConnectionInterrupted(connection=None) from e
Expand Down
2 changes: 1 addition & 1 deletion api/core/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ class AuthenticatedRequest(Request):
which DRF types as possibly anonymous, wherever `IsAuthenticated` guarantees a user.
"""

user: FFAdminUser | APIKeyUser
user: FFAdminUser | APIKeyUser # type: ignore[assignment]
19 changes: 11 additions & 8 deletions api/environments/dynamodb/wrappers/environment_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from typing import Any, Iterable

import structlog
from boto3.dynamodb.conditions import Key
from boto3.dynamodb.conditions import ConditionBase, Key
from django.conf import settings
from django.core.exceptions import ObjectDoesNotExist
from django.db.models import prefetch_related_objects
Expand Down Expand Up @@ -35,7 +35,7 @@
from .base import BaseDynamoWrapper

if typing.TYPE_CHECKING:
from mypy_boto3_dynamodb.type_defs import QueryInputRequestTypeDef
from mypy_boto3_dynamodb.type_defs import QueryInputTableQueryTypeDef

from environments.models import Environment
from util.dataclasses import CompressedEnvironmentDocument
Expand Down Expand Up @@ -63,6 +63,9 @@ def _map_compressed_environment_document(
) -> "CompressedEnvironmentDocument": ...

def _write_environments(self, environments: Iterable["Environment"]) -> None:
# Materialise once: prefetch_related_objects needs a Sequence, and the write
# loop below iterates again, which would come up empty for a generator.
environments = list(environments)
openfeature_client = get_openfeature_client()
prefetch_related_objects(
environments,
Expand Down Expand Up @@ -157,8 +160,8 @@ def get_identity_overrides_key_condition_expression(
self,
environment_id: int,
feature_id: None | int,
) -> Key:
return Key(ENVIRONMENTS_V2_PARTITION_KEY).eq( # type: ignore[return-value]
) -> ConditionBase:
return Key(ENVIRONMENTS_V2_PARTITION_KEY).eq(
str(environment_id),
) & Key(ENVIRONMENTS_V2_SORT_KEY).begins_with(
get_environments_v2_identity_override_document_key(
Expand Down Expand Up @@ -202,8 +205,8 @@ def _map_compressed_environment_document(
def delete_environment(self, environment_id: int): # type: ignore[no-untyped-def]
environment_id = str(environment_id) # type: ignore[assignment]
filter_expression = Key(ENVIRONMENTS_V2_PARTITION_KEY).eq(environment_id)
query_kwargs: "QueryInputRequestTypeDef" = { # type: ignore[typeddict-item]
"KeyConditionExpression": filter_expression, # type: ignore[typeddict-item]
query_kwargs: "QueryInputTableQueryTypeDef" = {
"KeyConditionExpression": filter_expression,
"ProjectionExpression": "document_key",
}
with self.table.batch_writer() as writer: # type: ignore[union-attr]
Expand All @@ -219,8 +222,8 @@ def delete_identity_overrides(self, environment_id: int, feature_id: int) -> Non
filter_expression = self.get_identity_overrides_key_condition_expression(
environment_id=environment_id, feature_id=feature_id
)
query_kwargs: "QueryInputRequestTypeDef" = { # type: ignore[typeddict-item]
"KeyConditionExpression": filter_expression, # type: ignore[typeddict-item]
query_kwargs: "QueryInputTableQueryTypeDef" = {
"KeyConditionExpression": filter_expression,
"ProjectionExpression": "document_key",
}

Expand Down
8 changes: 4 additions & 4 deletions api/environments/dynamodb/wrappers/identity_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
if typing.TYPE_CHECKING:
from boto3.dynamodb.conditions import ConditionBase
from mypy_boto3_dynamodb.type_defs import (
QueryInputRequestTypeDef,
QueryInputTableQueryTypeDef,
QueryOutputTableTypeDef,
TableAttributeValueTypeDef,
)
Expand Down Expand Up @@ -246,15 +246,15 @@ def get_all_items(
return_consumed_capacity: bool = False,
) -> "QueryOutputTableTypeDef":
key_condition_expression = Key("environment_api_key").eq(environment_api_key)
query_kwargs: "QueryInputRequestTypeDef" = { # type: ignore[typeddict-item]
query_kwargs: "QueryInputTableQueryTypeDef" = {
"IndexName": "environment_api_key-identifier-index",
"KeyConditionExpression": key_condition_expression, # type: ignore[typeddict-item]
"KeyConditionExpression": key_condition_expression,
"Limit": limit,
}
if start_key:
query_kwargs["ExclusiveStartKey"] = start_key
if filter_expression:
query_kwargs["FilterExpression"] = filter_expression # type: ignore[typeddict-item]
query_kwargs["FilterExpression"] = filter_expression
if projection_expression:
query_kwargs["ProjectionExpression"] = projection_expression
if return_consumed_capacity:
Expand Down
2 changes: 1 addition & 1 deletion api/experimentation/ingestion_sync_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

@lru_cache(maxsize=1)
def _get_client() -> RedisCluster:
return RedisCluster.from_url( # type: ignore[no-untyped-call,no-any-return]
return RedisCluster.from_url( # type: ignore[no-any-return]
settings.INGESTION_REDIS_URL,
socket_timeout=SOCKET_TIMEOUT,
socket_keepalive=True,
Expand Down
2 changes: 1 addition & 1 deletion api/features/feature_states/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ def has_permission(self, request: Request, view: APIView) -> bool:
except Environment.DoesNotExist:
return False

return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
UPDATE_FEATURE_STATE, environment
)
10 changes: 5 additions & 5 deletions api/features/import_export/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def has_permission(self, request: Request, view: APIView) -> bool:
).get(id=view.kwargs["environment_id"])

# Since feature imports can be destructive, use project admin.
return request.user.is_project_admin(environment.project) # type: ignore[union-attr,no-any-return]
return request.user.is_project_admin(environment.project) # type: ignore[union-attr]


class CreateFeatureExportPermissions(IsAuthenticated):
Expand All @@ -28,7 +28,7 @@ def has_permission(self, request: Request, view: APIView) -> bool:
return False

environment = Environment.objects.get(id=request.data["environment_id"])
return request.user.is_environment_admin(environment) # type: ignore[union-attr,no-any-return]
return request.user.is_environment_admin(environment) # type: ignore[union-attr]


class DownloadFeatureExportPermissions(IsAuthenticated):
Expand All @@ -38,7 +38,7 @@ def has_permission(self, request: Request, view: APIView) -> bool:

feature_export = FeatureExport.objects.get(id=view.kwargs["feature_export_id"])

return request.user.is_environment_admin(feature_export.environment) # type: ignore[union-attr,no-any-return]
return request.user.is_environment_admin(feature_export.environment) # type: ignore[union-attr]


class FeatureExportListPermissions(IsAuthenticated):
Expand All @@ -49,7 +49,7 @@ def has_permission(self, request: Request, view: ListAPIView) -> bool: # type:
project = Project.objects.get(id=view.kwargs["project_pk"])
# The user will only see environment feature exports
# that the user is an environment admin.
return request.user.has_project_permission(VIEW_PROJECT, project) # type: ignore[union-attr,no-any-return]
return request.user.has_project_permission(VIEW_PROJECT, project) # type: ignore[union-attr]


class FeatureImportListPermissions(IsAuthenticated):
Expand All @@ -60,4 +60,4 @@ def has_permission(self, request: Request, view: ListAPIView) -> bool: # type:
project = Project.objects.get(id=view.kwargs["project_pk"])
# The user will only see environment feature imports
# that the user is an environment admin.
return request.user.has_project_permission(VIEW_PROJECT, project) # type: ignore[union-attr,no-any-return]
return request.user.has_project_permission(VIEW_PROJECT, project) # type: ignore[union-attr]
10 changes: 5 additions & 5 deletions api/features/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,10 @@ def has_permission(self, request: Request, view: GenericViewSet) -> bool: # typ

tag_ids = list(feature.tags.values_list("id", flat=True))

return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
required_permission,
environment,
tag_ids=tag_ids,
tag_ids=tag_ids, # type: ignore[arg-type]
)
return False

Expand All @@ -144,10 +144,10 @@ def has_object_permission(
if permission in TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS:
tag_ids = list(obj.feature.tags.values_list("id", flat=True))

return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
permission,
environment=obj.environment,
tag_ids=tag_ids,
environment=obj.environment, # type: ignore[arg-type]
tag_ids=tag_ids, # type: ignore[arg-type]
)


Expand Down
20 changes: 10 additions & 10 deletions api/features/versioning/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ def has_permission(self, request: Request, view: GenericViewSet) -> bool: # typ
feature = Feature.objects.get(id=feature_id, project=environment.project)
tag_ids = list(feature.tags.values_list("id", flat=True))

return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
permission=required_permission,
environment=environment,
tag_ids=tag_ids,
tag_ids=tag_ids, # type: ignore[arg-type]
)

def has_object_permission(
Expand All @@ -53,10 +53,10 @@ def has_object_permission(
if required_permission in TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS:
tag_ids = list(obj.feature.tags.values_list("id", flat=True))

return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
permission=required_permission,
environment=obj.environment,
tag_ids=tag_ids,
tag_ids=tag_ids, # type: ignore[arg-type]
)


Expand All @@ -73,11 +73,11 @@ def has_permission(self, request: Request, view: GenericViewSet) -> bool: # typ
environment = Environment.objects.get(id=environment_pk)

if view.action == "list":
return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
permission=VIEW_ENVIRONMENT, environment=environment
)

return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
permission=UPDATE_FEATURE_STATE, environment=environment
)

Expand All @@ -88,12 +88,12 @@ def has_object_permission(
obj: FeatureState,
) -> bool:
if view.action == "retrieve": # pragma: no cover
return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
permission=VIEW_ENVIRONMENT,
environment=obj.environment,
environment=obj.environment, # type: ignore[arg-type]
)

return request.user.has_environment_permission( # type: ignore[union-attr,no-any-return]
return request.user.has_environment_permission( # type: ignore[union-attr]
permission=UPDATE_FEATURE_STATE,
environment=obj.environment,
environment=obj.environment, # type: ignore[arg-type]
)
2 changes: 1 addition & 1 deletion api/organisations/permissions/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,4 +230,4 @@ def has_permission(self, request: Request, view: View) -> bool:
return False

# All organisation users can see api usage notifications.
return request.user.belongs_to(view.kwargs.get("organisation_pk")) # type: ignore[union-attr,no-any-return]
return request.user.belongs_to(view.kwargs.get("organisation_pk")) # type: ignore[union-attr]
2 changes: 1 addition & 1 deletion api/projects/code_references/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def has_permission(self, request: Request, view: APIView) -> bool:
assert not isinstance(request.user, AnonymousUser)

project = Project.objects.get(id=view.kwargs["project_pk"])
return request.user.has_project_permission(VIEW_PROJECT, project) # type: ignore[no-any-return]
return request.user.has_project_permission(VIEW_PROJECT, project)


class SubmitFeatureFlagCodeReferences(_BaseCodeReferencePermission):
Expand Down
12 changes: 6 additions & 6 deletions api/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ dependencies = [
"drf-spectacular>=0.28.0,<0.29.0",
"sentry-sdk>=2.24.1,<3",
"environs>=14.1.1,<15.0.0",
"django-lifecycle>=1.2.4,<1.3.0",
"django-lifecycle>=1.2.4,<1.2.5",
"drf-writable-nested>=0.6.2,<0.7.0",
"django-filter>=2.4.0,<2.5.0",
"flagsmith-flag-engine>=11.0.0,<12.0.0",
Expand Down Expand Up @@ -106,19 +106,19 @@ dev = [
"pep8>=1.7.1,<1.8.0",
"autopep8>=2.0.1,<2.1.0",
"pytest>=9.0.3,<9.1.0",
"pytest-django>=4.8.0,<5.0.0",
"pytest-django>=4.8.0,<4.9.0",
"pytest-cov>=4.1.0,<4.2.0",
"datamodel-code-generator>=0.64,<0.65",
"requests-mock>=1.11.0,<2.0.0",
"django-extensions>=3.2.3,<4.0.0",
"pdbpp>=0.10.3,<0.11.0",
"pdbpp>=0.12.1,<0.13.0",
"mypy-boto3-dynamodb>=1.33.0,<2.0.0",
"mypy-boto3-s3>=1.36.0,<2.0.0",
Comment on lines 115 to 116

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

(
  cd api
  uv lock --check
)

rg -n -A2 'name = "(boto3|boto3-stubs|mypy-boto3-dynamodb|mypy-boto3-s3)"' api/uv.lock

Repository: Flagsmith/flagsmith

Length of output: 197


Align the Boto3 stub version bounds.

boto3-stubs is limited to <1.37.0, while mypy-boto3-dynamodb and mypy-boto3-s3 allow versions below 2.0.0. A lock refresh may select incompatible stub versions and reintroduce type-checking drift. Apply the same version window to all three packages, or pin compatible versions in api/uv.lock.

"pytest-structlog>=1.1,<2.0.0",
"pyfakefs>=6,<7",
"mypy>=1.15.0,<2.0.0",
"djangorestframework-stubs>=3.15.2,<4.0.0",
"boto3-stubs>=1.36.20,<2.0.0",
"mypy>=1.15.0,<1.16.0",
"djangorestframework-stubs>=3.15.2,<3.16.0",
"boto3-stubs>=1.36.20,<1.37.0",
"types-requests>=2.32.0.20241016,<3.0.0",
"types-markdown>=3.7.0.20241204,<4.0.0",
"types-pygments>=2.19.0.20250107,<3.0.0",
Expand Down
2 changes: 1 addition & 1 deletion api/tests/unit/features/test_unit_features_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4608,7 +4608,7 @@ def test_delete_feature__dynamo_identity_overrides__deletes_overrides(

assert (
flagsmith_environments_v2_table.query(
KeyConditionExpression=dynamodb_wrapper_v2.get_identity_overrides_key_condition_expression( # type: ignore[arg-type] # noqa: E501
KeyConditionExpression=dynamodb_wrapper_v2.get_identity_overrides_key_condition_expression( # noqa: E501
environment_id=environment.id, feature_id=feature.id
)
)["Count"]
Expand Down
14 changes: 9 additions & 5 deletions api/users/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -344,15 +344,15 @@ def get_user_organisation( # type: ignore[return]
def get_permitted_projects(
self,
permission_key: str,
tag_ids: typing.List[int] = None, # type: ignore[assignment]
tag_ids: list[int] | None = None,
) -> QuerySet[Project]:
return get_permitted_projects_for_user(self, permission_key, tag_ids)
return get_permitted_projects_for_user(self, permission_key, tag_ids) # type: ignore[arg-type]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

rg -n -A10 -B2 'def get_permitted_(projects|environments)_for_user' api/permissions/permission_service.py

Repository: Flagsmith/flagsmith

Length of output: 1028


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- api/users/models.py ---'
sed -n '320,395p' api/users/models.py
printf '%s\n' '--- api/permissions/permission_service.py ---'
sed -n '1,175p' api/permissions/permission_service.py
printf '%s\n' '--- repository type/version context ---'
rg -n 'python_requires|target-version|requires-python|python_version|from __future__ import annotations' pyproject.toml setup.cfg setup.py tox.ini .pre-commit-config.yaml api/users/models.py api/permissions/permission_service.py 2>/dev/null || true

Repository: Flagsmith/flagsmith

Length of output: 9748


🤖 get_repo_knowledge executed:

get_repo_knowledge Flagsmith/flagsmith /tmp/coderabbit-repo-knowledge/flagsmith-flagsmith-a74d157a/architecture /tmp/coderabbit-repo-knowledge/flagsmith-flagsmith-a74d157a/learnings /tmp/coderabbit-repo-knowledge/flagsmith-flagsmith-a74d157a/conventions

Length of output: 46814


Align the optional tag_ids contract across the permission layer.

get_permitted_projects_for_user and get_permitted_environments_for_user accept None at runtime but declare List[int]. Change these parameters and has_environment_permission.tag_ids to list[int] | None, then remove the matching type suppressions.


def has_project_permission(
self,
permission: str,
project: Project,
tag_ids: typing.List[int] = None, # type: ignore[assignment]
tag_ids: list[int] | None = None,
) -> bool:
if self.is_project_admin(project):
return True
Expand All @@ -375,11 +375,15 @@ def get_permitted_environments(
self,
permission_key: str,
project: Project,
tag_ids: typing.List[int] = None, # type: ignore[assignment]
tag_ids: list[int] | None = None,
prefetch_metadata: bool = False,
) -> QuerySet["Environment"]:
return get_permitted_environments_for_user(
self, project, permission_key, tag_ids, prefetch_metadata=prefetch_metadata
self,
project,
permission_key,
tag_ids, # type: ignore[arg-type]
prefetch_metadata=prefetch_metadata,
)

@staticmethod
Expand Down
Loading
Loading