Feature-Flag: org/project feature flags with DB#789
Open
Conversation
- Added `service.py` to handle assessment evaluation orchestration, including starting and retrying assessments. - Introduced utility functions for exporting assessment results in various formats (CSV, XLSX, JSON) in `export.py`. - Created parsing utilities for handling batch results in `parsing.py`. - Implemented validation for dataset file uploads in `validators.py`. - Updated batch job model to include assessment type. - Enhanced evaluation run model to reference parent assessments. - Added new fields for reasoning effort and summary preferences in LLM request model.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a comprehensive feature flag system with database schema, data models, CRUD operations, API routes, and permission gating. The system supports organization- and project-scoped feature flags (starting with ASSESSMENT) persisted in the database, exposed via API endpoints, and integrated into route dependencies and user context. ChangesFeature Flag System Implementation
Sequence DiagramsequenceDiagram
participant Client
participant APIRoute as Feature Flag Route
participant Auth as Auth Context
participant CRUD as Feature Flag CRUD
participant Database as DB
Client->>APIRoute: POST /features (create flag)
APIRoute->>Auth: Check SUPERUSER permission
Auth-->>APIRoute: ✓ Authorized
APIRoute->>CRUD: create_feature_flag(key, org, project, enabled)
CRUD->>Database: Query existing flag (uniqueness check)
alt Flag exists
Database-->>CRUD: Found
CRUD-->>APIRoute: HTTPException 409
APIRoute-->>Client: 409 Conflict
else Flag not found
Database-->>CRUD: Not found
CRUD->>Database: INSERT feature_flag
Database-->>CRUD: Created record
CRUD-->>APIRoute: FeatureFlagPublic
APIRoute-->>Client: 200 OK + flag details
end
Note over Client,Database: Later: Check flag during request
Client->>APIRoute: GET /assessments (gated by ASSESSMENT flag)
APIRoute->>Auth: Extract org/project from context
Auth-->>APIRoute: org_id, project_id
APIRoute->>CRUD: is_enabled(ASSESSMENT, org_id, project_id)
CRUD->>Database: SELECT enabled FROM feature_flag
Database-->>CRUD: enabled: true/false
CRUD-->>APIRoute: boolean result
alt Flag enabled
APIRoute->>APIRoute: Process request normally
APIRoute-->>Client: 200 OK + data
else Flag disabled
APIRoute-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The PR spans multiple architectural layers (database schema, models, CRUD, permissions, API routes, integrations) with heterogeneous logic: database migration with triggers, StrEnum migration, permission system changes, route gating dependencies, and API integration. While most individual functions follow standard patterns, the interconnected nature of changes and logic density (uniqueness checks, scoping validation, permission integration) demand careful review across all layers. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…n model - Introduced AssessmentRun model to replace EvaluationRun for assessment-specific runs. - Updated CRUD operations to manage Assessment and AssessmentRun entities. - Refactored processing logic to accommodate new AssessmentRun structure. - Adjusted logging and error handling to reflect changes in run management. - Modified routes and services to utilize AssessmentRun for assessment evaluations. - Enhanced export functionality to support new AssessmentRun model. - Removed EvaluationRun references where applicable and ensured backward compatibility.
…ssessment modules
…ous evaluation processing
vprashrex
changed the title
vprashrex
changed the title
vprashrex
changed the title
vprashrex
changed the title
AkhileshNegi
requested changes
May 5, 2026
| payload: FeatureFlagCreate, | ||
| ) -> FeatureFlagPublic: | ||
| validate_organization(session=session, org_id=payload.organization_id) | ||
| validate_project_belongs_to_organization( |
Collaborator
There was a problem hiding this comment.
why do we need this check? if it is not present already then add in utils since this is quite generic
Comment on lines
+83
to
+86
| if organization_id is not None: | ||
| validate_organization(session=session, org_id=organization_id) | ||
| if project_id is not None: | ||
| assert organization_id is not None |
Collaborator
There was a problem hiding this comment.
why assert? and then validate organization also
Comment on lines
+84
to
+88
| op.create_index( | ||
| op.f("ix_feature_flag_organization_id"), | ||
| "feature_flag", | ||
| ["organization_id"], | ||
| unique=False, |
Collaborator
There was a problem hiding this comment.
need similar index for project_id
| from enum import StrEnum | ||
|
|
||
|
|
||
| class FeatureFlag(StrEnum): |
Collaborator
There was a problem hiding this comment.
can we use somthing like FeatureFlagKeyEnum to avoid confusion with FeatureFlag class
… safety - Added HTTPException raises for not found cases in `get_assessment_by_id`, `get_assessment_run_by_id`, and `get_assessment_dataset_by_id` functions to improve error handling. - Updated return types for several functions to ensure they always return the expected model instead of None. - Refactored logging statements in assessment evaluation polling to use parameterized logging for better readability and performance. - Introduced utility functions for attachment resolution, including MIME type detection and URL normalization. - Improved the handling of assessment run statuses and batch processing, ensuring proper error logging and status updates. - Enhanced test coverage for CRUD operations and mappers, ensuring that exceptions are raised and handled correctly. - Updated assessment models to use stricter type definitions for status fields and optional attributes.
…ments and core logic
…y in Assessment models
…n validation and error handling
…ments and formatting
AkhileshNegi
approved these changes
May 6, 2026
| api_router.include_router(assessment_routes.router) | ||
| api_router.include_router(assistants.router) | ||
| api_router.include_router(collections.router) | ||
| api_router.include_router(auth.router) |
Collaborator
There was a problem hiding this comment.
api_router.include_router(auth.router) already exist in line:49
can you make all these alphabetical to ensure same is not repeated by other developers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
backend/app/crud/feature_flag.py (1)
23-35: 💤 Low value
get_feature_flag_enabledconflates "missing" and "disabled" at call sites — document or simplify.The signature
bool | Noneis technically accurate (None= row not found,False= row exists but disabled,True= enabled), but every caller that just wants a boolean must remember this trinary. If no caller actually distinguishes the two falsy cases, consider returningbooland defaulting toFalsewhen no row exists; otherwise add a docstring spelling out the three states so callers don't accidentally treatNoneas "enabled" via a misuse ofor.-) -> bool | None: +) -> bool | None: + """Returns True/False if a flag row exists, or None when no row matches."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/crud/feature_flag.py` around lines 23 - 35, get_feature_flag_enabled currently returns bool | None which conflates "row missing" (None) and "flag disabled" (False); either document the three-state behavior or—preferably if callers never distinguish—simplify to always return bool by defaulting a missing row to False. Update the get_feature_flag_enabled function to clarify semantics (add a docstring stating None means row not found vs False means explicit disabled) and, if you choose the simpler API, change the return type to bool and have the query result be interpreted as False when session.exec(...).first() is None so callers get a plain boolean; reference FeatureFlagModel.enabled in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/alembic/versions/057_create_feature_flag_table.py`:
- Line 21: Add explicit return type annotations for the migration entrypoints by
changing the function signatures for upgrade and downgrade to include "-> None";
update the def upgrade() and def downgrade() functions in this migration module
to be def upgrade() -> None: and def downgrade() -> None: so they comply with
the repository's requirement for explicit function return typing.
- Line 12: The migration currently imports a runtime enum FeatureFlag and leaves
upgrade() and downgrade() without return type hints; replace the import of
app.core.feature_flags.constants.FeatureFlag with frozen, literal enum values
(e.g., a hard-coded tuple/list of the exact string names/values used in the SQL
DDL and insert statements) so the migration is self-contained and will not break
if the application enum changes, and add explicit return type annotations to
both upgrade() -> None and downgrade() -> None; update any references in the
migration (e.g., inside CREATE TYPE or INSERT INTO feature_flag, and the symbols
FeatureFlag used at lines around 110 and 128) to use the frozen literals instead
of the runtime enum.
In `@backend/app/api/permissions.py`:
- Around line 87-106: The _check_with_session function conflates missing
org/project context with a disabled feature; change it so that if
auth_context.organization or auth_context.project is missing (org_id or
project_id is None) you route to the appropriate permission requirement (e.g.,
invoke require_permission(REQUIRE_ORGANIZATION/REQUIRE_PROJECT) or raise a
distinct HTTPException message like "Missing organization/project scope")
instead of the generic disabled message, and only call is_enabled(...) and raise
the existing HTTPException("Feature 'X' is not enabled...") when both ids exist
and is_enabled(...) returns false; update the HTTPException branches around
is_enabled and the org/project checks accordingly.
- Around line 74-108: Add explicit return type hints for both require_feature
and its inner function _check_with_session and import Callable from
collections.abc; specifically annotate _check_with_session to return None (it
raises or returns None) and annotate require_feature to return a Callable that
accepts the dependency parameters (AuthContextDep, SessionDep) and returns None
(e.g., Callable[[AuthContextDep, SessionDep], None]) so type checkers know the
factory yields a dependency callable.
In `@backend/app/api/routes/features.py`:
- Around line 30-54: The POST endpoint is failing because the migration 057
already inserts an ASSESSMENT flag per project and create_feature_flag()
currently returns 409 on duplicate, making create_feature_flag_route unusable
for the only defined FeatureFlag; modify create_feature_flag (used by
create_feature_flag_route) to be idempotent: detect existing flag by
(project_id, key) before inserting or catch the unique-constraint/IntegrityError
on insert and return the existing FeatureFlag instead of raising 409. Ensure
this logic references the same unique key used by the DB and keep responses
consistent with APIResponse.success_response for the existing record.
In `@backend/app/crud/feature_flag.py`:
- Around line 46-64: The create_feature_flag flow currently performs a
read-then-write leading to a TOCTOU race; keep the existence check but make the
insert atomic by catching a DB unique-constraint failure: wrap the
session.add/commit/refresh sequence in a try/except that catches
sqlalchemy.exc.IntegrityError, calls session.rollback(), and raises
HTTPException(status_code=409, detail="Feature flag already exists"); reference
the existing create_feature_flag function, the FeatureFlagModel construction,
session.commit(), and get_feature_flag, and add the
sqlalchemy.exc.IntegrityError import if missing.
In `@backend/app/models/feature_flag.py`:
- Around line 27-31: The FeatureFlag model's project_id Field is missing
index=True which causes schema drift versus the migration; update the Field
declaration for project_id in the FeatureFlag class to include index=True (i.e.,
add index=True to the Field(...) args for the project_id attribute) so the ORM
metadata matches the existing migration-created ix_feature_flag_project_id
index.
In `@scripts/python/invoke-cron.py`:
- Around line 39-40: The code currently hard-codes self.interval_minutes = 1;
change it to read the configured cron interval (e.g., an environment/config
value like CRON_INTERVAL_MINUTES or the existing config accessor) and fall back
to the expected default (5) if missing/invalid, then set self.interval_seconds =
int(self.interval_minutes) * 60; ensure you parse/validate the env/config value
to an integer and reference the same config key used elsewhere so the cron
cadence matches the rest of the system (modify the initializer where
self.interval_minutes and self.interval_seconds are set).
---
Nitpick comments:
In `@backend/app/crud/feature_flag.py`:
- Around line 23-35: get_feature_flag_enabled currently returns bool | None
which conflates "row missing" (None) and "flag disabled" (False); either
document the three-state behavior or—preferably if callers never
distinguish—simplify to always return bool by defaulting a missing row to False.
Update the get_feature_flag_enabled function to clarify semantics (add a
docstring stating None means row not found vs False means explicit disabled)
and, if you choose the simpler API, change the return type to bool and have the
query result be interpreted as False when session.exec(...).first() is None so
callers get a plain boolean; reference FeatureFlagModel.enabled in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97d20c4f-f871-4c9a-a8bf-8684af137f16
📒 Files selected for processing (27)
backend/app/alembic/versions/057_create_feature_flag_table.pybackend/app/api/docs/features/create_flag.mdbackend/app/api/docs/features/delete_flag.mdbackend/app/api/docs/features/list_flags.mdbackend/app/api/docs/features/update_flag.mdbackend/app/api/docs/users/get_me.mdbackend/app/api/main.pybackend/app/api/permissions.pybackend/app/api/routes/assessment/__init__.pybackend/app/api/routes/features.pybackend/app/api/routes/users.pybackend/app/core/feature_flags/__init__.pybackend/app/core/feature_flags/constants.pybackend/app/crud/__init__.pybackend/app/crud/assessment/dataset.pybackend/app/crud/assessment/processing.pybackend/app/crud/feature_flag.pybackend/app/crud/project.pybackend/app/models/__init__.pybackend/app/models/feature_flag.pybackend/app/models/user.pybackend/app/tests/api/routes/test_features.pybackend/app/tests/api/test_permissions.pybackend/app/tests/assessment/test_crud.pybackend/app/tests/crud/test_feature_flag.pybackend/app/utils.pyscripts/python/invoke-cron.py
| import sqlalchemy as sa | ||
| from alembic import op | ||
|
|
||
| from app.core.feature_flags.constants import FeatureFlag as FeatureFlagKeyEnum |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '^from app\.|^import app\.' backend/app/alembic/versions
fd '057_create_feature_flag_table\.py$' backend/app/alembic/versions --exec sed -n '1,140p' {}Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 3960
🏁 Script executed:
#!/bin/bash
# Get complete file with line numbers
fd '057_create_feature_flag_table\.py$' backend/app/alembic/versions --exec wc -l {} \;Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Read complete file
fd '057_create_feature_flag_table\.py$' backend/app/alembic/versions --exec cat -n {} \;Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 5604
Add return type hints and freeze enum values in the migration.
This migration imports a runtime application symbol (app.core.feature_flags.constants.FeatureFlag) and uses it in SQL (lines 110, 128). Future changes to the enum's location, values, or definition will break database bootstrapping from this historical revision. Alembic migrations must be self-contained with frozen values.
Additionally, both upgrade() (line 21) and downgrade() (line 148) functions lack return type annotations, violating the type hints requirement.
Suggested fix
-from app.core.feature_flags.constants import FeatureFlag as FeatureFlagKeyEnum
+DEFAULT_FEATURE_FLAG_KEY = "ASSESSMENT"
-def upgrade():
+def upgrade() -> None:
op.execute(
sa.text(
f"""
INSERT INTO feature_flag
(key, organization_id, project_id, enabled, inserted_at, updated_at)
- SELECT '{FeatureFlagKeyEnum.ASSESSMENT.value}'::featureflagkey,
+ SELECT '{DEFAULT_FEATURE_FLAG_KEY}'::featureflagkey,
p.organization_id, p.id, FALSE, NOW(), NOW()
FROM project p
"""
)
)
op.execute(
sa.text(
f"""
CREATE OR REPLACE FUNCTION seed_default_feature_flag()
RETURNS trigger
LANGUAGE plpgsql
AS $$
BEGIN
INSERT INTO feature_flag
(key, organization_id, project_id, enabled, inserted_at, updated_at)
VALUES
- ('{FeatureFlagKeyEnum.ASSESSMENT.value}'::featureflagkey,
+ ('{DEFAULT_FEATURE_FLAG_KEY}'::featureflagkey,
NEW.organization_id, NEW.id, FALSE, NOW(), NOW());
RETURN NEW;
END;
$$;
"""
)
)
-def downgrade():
+def downgrade() -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/alembic/versions/057_create_feature_flag_table.py` at line 12,
The migration currently imports a runtime enum FeatureFlag and leaves upgrade()
and downgrade() without return type hints; replace the import of
app.core.feature_flags.constants.FeatureFlag with frozen, literal enum values
(e.g., a hard-coded tuple/list of the exact string names/values used in the SQL
DDL and insert statements) so the migration is self-contained and will not break
if the application enum changes, and add explicit return type annotations to
both upgrade() -> None and downgrade() -> None; update any references in the
migration (e.g., inside CREATE TYPE or INSERT INTO feature_flag, and the symbols
FeatureFlag used at lines around 110 and 128) to use the frozen literals instead
of the runtime enum.
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade(): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit -> None to the migration entrypoints.
These are new Python functions, and the repo guideline requires explicit parameter and return typing.
Suggested fix
-def upgrade():
+def upgrade() -> None:
...
-def downgrade():
+def downgrade() -> None:As per coding guidelines, "Always add type hints to all function parameters and return values in Python code."
Also applies to: 148-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/alembic/versions/057_create_feature_flag_table.py` at line 21,
Add explicit return type annotations for the migration entrypoints by changing
the function signatures for upgrade and downgrade to include "-> None"; update
the def upgrade() and def downgrade() functions in this migration module to be
def upgrade() -> None: and def downgrade() -> None: so they comply with the
repository's requirement for explicit function return typing.
Comment on lines
+74
to
+108
| def require_feature(flag: FeatureFlag): | ||
| """Dependency factory that gates a route behind a feature flag. | ||
|
|
||
| Returns 403 when the flag is disabled for the caller's org/project, | ||
| so callers can explicitly handle feature access denial. | ||
|
|
||
| Usage:: | ||
|
|
||
| router = APIRouter( | ||
| dependencies=[Depends(require_feature(FeatureFlag.ASSESSMENT))] | ||
| ) | ||
| """ | ||
|
|
||
| def _check_with_session(auth_context: AuthContextDep, session: SessionDep): | ||
| from app.core.feature_flags import is_enabled | ||
|
|
||
| org_id = auth_context.organization.id if auth_context.organization else None | ||
| project_id = auth_context.project.id if auth_context.project else None | ||
|
|
||
| if ( | ||
| org_id is None | ||
| or project_id is None | ||
| or not is_enabled( | ||
| session=session, | ||
| flag=flag.value, | ||
| organization_id=org_id, | ||
| project_id=project_id, | ||
| ) | ||
| ): | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail=f"Feature '{flag.value}' is not enabled for this org or project.", | ||
| ) | ||
|
|
||
| return _check_with_session |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add return type annotations per coding guidelines.
Both require_feature and the inner _check_with_session are missing return type hints. The inner check returns None and the factory returns the dependency callable.
✏️ Proposed annotations
-def require_feature(flag: FeatureFlag):
+def require_feature(flag: FeatureFlag) -> Callable[[AuthContextDep, SessionDep], None]:
"""Dependency factory that gates a route behind a feature flag.
...
"""
- def _check_with_session(auth_context: AuthContextDep, session: SessionDep):
+ def _check_with_session(auth_context: AuthContextDep, session: SessionDep) -> None:
from app.core.feature_flags import is_enabledAdd from collections.abc import Callable to imports.
As per coding guidelines: "Always add type hints to all function parameters and return values in Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/permissions.py` around lines 74 - 108, Add explicit return
type hints for both require_feature and its inner function _check_with_session
and import Callable from collections.abc; specifically annotate
_check_with_session to return None (it raises or returns None) and annotate
require_feature to return a Callable that accepts the dependency parameters
(AuthContextDep, SessionDep) and returns None (e.g., Callable[[AuthContextDep,
SessionDep], None]) so type checkers know the factory yields a dependency
callable.
Comment on lines
+87
to
+106
| def _check_with_session(auth_context: AuthContextDep, session: SessionDep): | ||
| from app.core.feature_flags import is_enabled | ||
|
|
||
| org_id = auth_context.organization.id if auth_context.organization else None | ||
| project_id = auth_context.project.id if auth_context.project else None | ||
|
|
||
| if ( | ||
| org_id is None | ||
| or project_id is None | ||
| or not is_enabled( | ||
| session=session, | ||
| flag=flag.value, | ||
| organization_id=org_id, | ||
| project_id=project_id, | ||
| ) | ||
| ): | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail=f"Feature '{flag.value}' is not enabled for this org or project.", | ||
| ) |
There was a problem hiding this comment.
Distinguish "missing context" from "feature disabled" in the 403 response.
When org_id or project_id is None, the handler returns the same "Feature 'X' is not enabled..." message used when the flag is genuinely disabled. This is misleading for callers debugging an auth/context issue (e.g., a token without project scope) — they will think the feature is off when in fact the request lacks the required scope. Consider routing missing org/project through require_permission(REQUIRE_ORGANIZATION/REQUIRE_PROJECT) (or returning a distinct message), and only emitting the "not enabled" response when is_enabled(...) returns falsy.
🔧 Suggested split
- if (
- org_id is None
- or project_id is None
- or not is_enabled(
- session=session,
- flag=flag.value,
- organization_id=org_id,
- project_id=project_id,
- )
- ):
+ if org_id is None or project_id is None:
+ raise HTTPException(
+ status_code=403,
+ detail="Organization and project context are required to evaluate feature flags.",
+ )
+
+ if not is_enabled(
+ session=session,
+ flag=flag.value,
+ organization_id=org_id,
+ project_id=project_id,
+ ):
raise HTTPException(
status_code=403,
detail=f"Feature '{flag.value}' is not enabled for this org or project.",
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/permissions.py` around lines 87 - 106, The
_check_with_session function conflates missing org/project context with a
disabled feature; change it so that if auth_context.organization or
auth_context.project is missing (org_id or project_id is None) you route to the
appropriate permission requirement (e.g., invoke
require_permission(REQUIRE_ORGANIZATION/REQUIRE_PROJECT) or raise a distinct
HTTPException message like "Missing organization/project scope") instead of the
generic disabled message, and only call is_enabled(...) and raise the existing
HTTPException("Feature 'X' is not enabled...") when both ids exist and
is_enabled(...) returns false; update the HTTPException branches around
is_enabled and the org/project checks accordingly.
Comment on lines
+30
to
+54
| @router.post( | ||
| "/feature-flags", | ||
| description=load_description("features/create_flag.md"), | ||
| response_model=APIResponse[FeatureFlagPublic], | ||
| dependencies=[Depends(require_permission(Permission.SUPERUSER))], | ||
| ) | ||
| def create_feature_flag_route( | ||
| session: SessionDep, | ||
| payload: FeatureFlagCreate, | ||
| ) -> APIResponse[FeatureFlagPublic]: | ||
| project = validate_project(session=session, project_id=payload.project_id) | ||
| created = create_feature_flag( | ||
| session=session, | ||
| key=payload.key, | ||
| organization_id=project.organization_id, | ||
| project_id=payload.project_id, | ||
| enabled=payload.enabled, | ||
| ) | ||
| logger.info( | ||
| "[create_feature_flag_route] Created flag=%s enabled=%s project=%s", | ||
| payload.key, | ||
| payload.enabled, | ||
| payload.project_id, | ||
| ) | ||
| return APIResponse.success_response(data=created) |
There was a problem hiding this comment.
POST cannot create the only supported flag in normal state.
057_create_feature_flag_table.py backfills ASSESSMENT for every existing project and adds a trigger to insert the same row for every new project, while create_feature_flag() returns 409 when that row already exists. Since app.core.feature_flags.constants.FeatureFlag currently only defines ASSESSMENT, this endpoint is effectively unusable unless the row was deleted first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/routes/features.py` around lines 30 - 54, The POST endpoint
is failing because the migration 057 already inserts an ASSESSMENT flag per
project and create_feature_flag() currently returns 409 on duplicate, making
create_feature_flag_route unusable for the only defined FeatureFlag; modify
create_feature_flag (used by create_feature_flag_route) to be idempotent: detect
existing flag by (project_id, key) before inserting or catch the
unique-constraint/IntegrityError on insert and return the existing FeatureFlag
instead of raising 409. Ensure this logic references the same unique key used by
the DB and keep responses consistent with APIResponse.success_response for the
existing record.
Comment on lines
+46
to
+64
| feature_flag = get_feature_flag( | ||
| session=session, | ||
| key=key, | ||
| organization_id=organization_id, | ||
| project_id=project_id, | ||
| ) | ||
| if feature_flag is not None: | ||
| raise HTTPException(status_code=409, detail="Feature flag already exists") | ||
|
|
||
| feature_flag = FeatureFlagModel( | ||
| key=key, | ||
| organization_id=organization_id, | ||
| project_id=project_id, | ||
| enabled=enabled, | ||
| ) | ||
| session.add(feature_flag) | ||
| session.commit() | ||
| session.refresh(feature_flag) | ||
| return feature_flag |
There was a problem hiding this comment.
TOCTOU race in create_feature_flag.
The existence check (get_feature_flag → raise 409) and the subsequent insert are not atomic. Two concurrent calls with the same (key, organization_id, project_id) can both pass the check and both attempt INSERT, with the second failing on the composite unique index from migration 057_create_feature_flag_table.py and surfacing as an unhandled IntegrityError (HTTP 500) instead of a clean 409. Catch IntegrityError after the insert and translate it, so the unique index — not the read-then-write check — is the source of truth.
🔒 Suggested fix
+from sqlalchemy.exc import IntegrityError
@@
feature_flag = FeatureFlagModel(
key=key,
organization_id=organization_id,
project_id=project_id,
enabled=enabled,
)
session.add(feature_flag)
- session.commit()
- session.refresh(feature_flag)
+ try:
+ session.commit()
+ except IntegrityError:
+ session.rollback()
+ raise HTTPException(status_code=409, detail="Feature flag already exists")
+ session.refresh(feature_flag)
return feature_flag🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/crud/feature_flag.py` around lines 46 - 64, The
create_feature_flag flow currently performs a read-then-write leading to a
TOCTOU race; keep the existence check but make the insert atomic by catching a
DB unique-constraint failure: wrap the session.add/commit/refresh sequence in a
try/except that catches sqlalchemy.exc.IntegrityError, calls session.rollback(),
and raises HTTPException(status_code=409, detail="Feature flag already exists");
reference the existing create_feature_flag function, the FeatureFlagModel
construction, session.commit(), and get_feature_flag, and add the
sqlalchemy.exc.IntegrityError import if missing.
Comment on lines
+27
to
+31
| project_id: int = Field( | ||
| foreign_key="project.id", | ||
| nullable=False, | ||
| ondelete="CASCADE", | ||
| sa_column_kwargs={"comment": "Project scope for this feature flag"}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd 'feature_flag\.py$' backend/app/models --exec sed -n '20,40p' {}
fd '057_create_feature_flag_table\.py$' backend/app/alembic/versions --exec sed -n '86,100p' {}Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1104
Add index=True to project_id to match the migration.
The migration creates an index ix_feature_flag_project_id for this column, but the model field doesn't declare it. This creates schema drift that will cause the next Alembic autogenerate pass to detect the mismatch. Keep the model metadata aligned with the actual migration.
Suggested fix
project_id: int = Field(
foreign_key="project.id",
+ index=True,
nullable=False,
ondelete="CASCADE",
sa_column_kwargs={"comment": "Project scope for this feature flag"},
)📝 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
| project_id: int = Field( | |
| foreign_key="project.id", | |
| nullable=False, | |
| ondelete="CASCADE", | |
| sa_column_kwargs={"comment": "Project scope for this feature flag"}, | |
| project_id: int = Field( | |
| foreign_key="project.id", | |
| index=True, | |
| nullable=False, | |
| ondelete="CASCADE", | |
| sa_column_kwargs={"comment": "Project scope for this feature flag"}, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/models/feature_flag.py` around lines 27 - 31, The FeatureFlag
model's project_id Field is missing index=True which causes schema drift versus
the migration; update the Field declaration for project_id in the FeatureFlag
class to include index=True (i.e., add index=True to the Field(...) args for the
project_id attribute) so the ORM metadata matches the existing migration-created
ix_feature_flag_project_id index.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
3 participants
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.
Summary
Target issue is #790
Explain the motivation for making this change. What existing problem does the pull request solve?
This PR introduces a persistent feature-flag system scoped by organization_id and project_id and applies it to control access to Feature flagged APIs.
Previously, Assessment functionality was not centrally gated per tenant/project, so rollout control was limited and risky. This change solves that by adding:
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements