Conversation
📝 WalkthroughWalkthroughAdds persistent validator configuration: DB migration and SQLModel table, CRUD service and singleton, FastAPI routes wired into main router, new schemas/enums/constants, request payload utilities, import reorganizations, and unit + integration tests plus test infra updates (and .env.test removal). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "FastAPI Router\n(/guardrails/validators/configs)"
participant Auth as "AuthDep"
participant CRUD as "validator_config_crud"
participant DB as "Postgres (validator_config)"
Client->>API: POST /guardrails/validators/configs (payload)
API->>Auth: validate auth (AuthDep)
Auth-->>API: auth OK
API->>CRUD: create(session, org_id, project_id, payload)
CRUD->>DB: INSERT validator_config (base fields + JSONB config)
DB-->>CRUD: INSERT OK (row)
CRUD-->>API: flattened dict
API-->>Client: 201 Created (ValidatorResponse)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/utils.py (1)
42-53:⚠️ Potential issue | 🟡 MinorGuard against non‑dict error items in
failure_response.If a caller passes a list of strings (or non‑dict items),
err.get(...)will raise an AttributeError. A small defensive formatter keeps this robust.🛠️ Suggested fix
- if isinstance(error, list): # to handle cases when error is a list of errors - error_message = "\n".join( - [f"{err.get('loc', 'unknown')}: {err.get('msg', str(err))}" for err in error] - ) + if isinstance(error, list): # to handle cases when error is a list of errors + def _fmt(err: Any) -> str: + if isinstance(err, dict): + return f"{err.get('loc', 'unknown')}: {err.get('msg', str(err))}" + return str(err) + error_message = "\n".join(_fmt(err) for err in error)
🤖 Fix all issues with AI agents
In `@backend/app/alembic/versions/003_added_validator_config.py`:
- Around line 3-6: The Alembic file header is inconsistent: the top docstring
line "Revises: 001" doesn't match the module's down_revision value ("002");
update the header to "Revises: 002" so it matches the down_revision variable in
the migration (ensure Revision ID: 003 remains unchanged and that down_revision
is still "002").
In `@backend/app/api/routes/validator_configs.py`:
- Around line 126-134: The debug print in flatten_validator is leaking config
data; remove the line that prints "FLATTENED:" (the print(...) call) from the
flatten_validator(row: ValidatorConfig) function and ensure no other plain
stdout prints remain in that function; if logging is required, replace with a
structured logger that redacts sensitive fields from row.config before logging.
- Around line 156-174: The update_validator_config function doesn't update the
model's updated_at timestamp; before session.add/commit/refresh set
obj.updated_at to the current time (e.g., datetime.utcnow()) so PATCHs bump the
timestamp, and add the required import (from datetime import datetime) at the
top of the file; keep the existing merge of obj.config and then set updated_at
right before persisting in update_validator_config.
- Around line 1-11: The module currently uses a star import from
app.schemas.validator_config and typing.List; replace the star import with
explicit schema names that this file actually uses (e.g., the create/read/update
Pydantic/SQLModel classes such as ValidatorConfigCreate, ValidatorConfigRead,
ValidatorConfigUpdate — import whichever specific symbols appear in this file)
and remove List from typing and any references to typing.List by switching
annotations to the built-in list type; also delete the stray debug print() on
line 133. Ensure the top imports reflect these changes and update any function
signatures or type hints that used List to use list instead.
In `@backend/app/models/config/validator_config_table.py`:
- Around line 77-82: Add a trailing newline at end of file to satisfy the W292
lint warning; open the file containing the __table_args__ declaration (the
UniqueConstraint with name "uq_validator_identity") and ensure the file ends
with a single newline character (save with a newline-terminated EOF).
- Around line 71-75: The updated_at Field in validator_config_table (updated_at:
datetime = Field(...)) only uses default_factory=now, so PATCH updates won't
change it; add an onupdate entry to sa_column_kwargs that calls the same now
function (e.g., sa_column_kwargs={"comment": "...", "onupdate": now}) so the
SQLAlchemy column updates updated_at automatically on UPDATE operations.
In `@backend/app/tests/conftest.py`:
- Around line 29-34: The clean_db fixture currently uses session.exec(...) to
run bulk DELETEs against SQLModel.metadata.sorted_tables; replace those calls
with session.execute(...) to run the table.delete() expressions (since these are
bulk SQL operations, not ORM SELECTs). Update the loop inside clean_db (which
opens with with Session(test_engine) as session:) to call
session.execute(table.delete()) for each table, then commit as before; keep
using SQLModel.metadata.sorted_tables and the existing Session/test_engine
setup.
🧹 Nitpick comments (3)
backend/app/crud/validator_log_repo.py (1)
1-1: Unused import:uuid4The
uuid4function is imported but not used in this file. OnlyUUIDis needed.🧹 Proposed cleanup
-from uuid import UUID, uuid4 +from uuid import UUIDbackend/app/crud/__init__.py (1)
1-1: Add trailing newline and consider exportingValidatorLogCrud.
- Ruff flagged a missing trailing newline (W292).
- For consistency, consider also exporting
ValidatorLogCrudfrom this__init__.pysince both CRUD classes follow the same pattern.🧹 Proposed fix
-from app.crud.request_log_repo import RequestLogCrud +from app.crud.request_log_repo import RequestLogCrud +from app.crud.validator_log_repo import ValidatorLogCrud +backend/app/tests/conftest.py (1)
19-21: Consider using a context manager or ensuring proper session cleanup.The
override_sessiongenerator yields a session but doesn't explicitly handle exceptions. While thewith Session(...)context manager handles basic cleanup, you may want to add explicit rollback on exceptions to ensure clean state between tests.🛡️ Defensive session handling
def override_session(): with Session(test_engine) as session: - yield session + try: + yield session + except Exception: + session.rollback() + raise
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/crud/validator_config_crud.py`:
- Around line 81-98: The update() method on ValidatorConfig currently applies
base and config changes but never updates the ValidatorConfig.updated_at
timestamp; before session.commit() set obj.updated_at = now() (or the project's
canonical timestamp function) so the model reflects the modification time; keep
using split_validator_payload(...) to build base/config and preserve existing
merge logic for obj.config, then commit, refresh and return self._flatten(obj).
In `@backend/app/tests/test_validator_configs.py`:
- Around line 21-30: The clear_database pytest fixture defined as clear_database
in this file is unused and redundant because tests here use mock_session and
sample_validator and conftest.py already provides an autouse clean_db cleanup;
remove the clear_database fixture declaration entirely from
backend/app/tests/test_validator_configs.py to eliminate dead code and avoid
confusion (leave the separate clear_database in
test_validator_configs_integration.py intact).
🧹 Nitpick comments (7)
backend/app/crud/validator_config_crud.py (3)
1-1: Use built-inlistinstead oftyping.List.
typing.Listis deprecated since Python 3.9. Use the built-inlisttype directly.♻️ Suggested fix
-from typing import List, Optional +from typing import OptionalThen update line 52:
- ) -> List[dict]: + ) -> list[dict]:
14-20: Add type annotation forpayloadparameter.The
payloadparameter lacks a type hint, reducing code clarity and IDE support. Based on usage, it should beValidatorCreate.♻️ Suggested fix
+from app.schemas.validator_config import ValidatorCreate + class ValidatorConfigCrud: def create( self, session: Session, org_id: int, project_id: int, - payload + payload: ValidatorCreate ):
104-106: Consider renaming_flattenor exposing it as public.The method is named with a leading underscore (private convention), but it's called directly from the routes layer (
validator_config_crud._flatten(obj)at line 57 ofvalidator_configs.py). Either make it public by removing the underscore or add a public wrapper.backend/app/api/routes/validator_configs.py (2)
30-42: Avoid shadowing built-intype.The parameter
typeshadows Python's built-intype()function. Consider renaming tovalidator_typefor clarity.♻️ Suggested fix
async def list_validators( org_id: int, project_id: int, session: SessionDep, _: AuthDep, stage: Optional[Stage] = None, - type: Optional[ValidatorType] = None, + validator_type: Optional[ValidatorType] = None, ): - return validator_config_crud.list(session, org_id, project_id, stage, type) + return validator_config_crud.list(session, org_id, project_id, stage, validator_type)
45-57: Accessing private method_flattenfrom outside the class.Line 57 calls
validator_config_crud._flatten(obj), which is a private method by Python convention. Either rename it toflattenin the CRUD class or add a publicgetmethod that returns a flattened dict directly.♻️ Suggested fix - Option A: Use a wrapper in the route
async def get_validator( id: UUID, org_id: int, project_id: int, session: SessionDep, _: AuthDep, ): obj = validator_config_crud.get_or_404(session, id, org_id, project_id) - return validator_config_crud._flatten(obj) + base = obj.model_dump(exclude={"config"}) + return {**base, **(obj.config or {})}♻️ Suggested fix - Option B: Make flatten public in CRUD class
In
validator_config_crud.py:- def _flatten(self, row: ValidatorConfig) -> dict: + def flatten(self, row: ValidatorConfig) -> dict:Then in this file:
- return validator_config_crud._flatten(obj) + return validator_config_crud.flatten(obj)backend/app/tests/test_validator_configs.py (2)
93-104: Assert specific exception typeHTTPException.The test catches a generic
Exception, butget_or_404raisesHTTPException. This could mask other unexpected exceptions. Usepytest.raises(HTTPException)for precision.♻️ Suggested fix
+from fastapi import HTTPException + def test_not_found(self, mock_session): mock_session.get.return_value = None - with pytest.raises(Exception) as exc: + with pytest.raises(HTTPException) as exc: validator_config_crud.get_or_404( mock_session, TEST_VALIDATOR_ID, TEST_ORG_ID, TEST_PROJECT_ID, ) - assert "Validator not found" in str(exc.value) + assert exc.value.status_code == 404 + assert exc.value.detail == "Validator not found"
1-18: Consider adding tests forcreate,list, anddeletemethods.The test file covers
_flatten,get_or_404, andupdate, but missing coverage forcreate,list, anddeleteCRUD operations. The integration tests may cover these, but unit tests with mocks would provide faster feedback.Would you like me to generate unit test stubs for the missing CRUD methods?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/crud/validator_config_crud.py`:
- Around line 88-96: The current logic sets obj.updated_at only when config is
present; move or add the updated timestamp assignment so that obj.updated_at =
now() is set whenever any update occurs (whether base fields or config) after
applying base via split_validator_payload and before session.commit();
specifically, after the for k,v in base: setattr(obj,k,v) loop (and still
update/merge config when config exists), ensure obj.updated_at is updated
unconditionally so updates to base fields like type/stage/on_fail_action bump
the timestamp.
In `@backend/app/tests/conftest.py`:
- Around line 13-17: The test suite is using settings.SQLALCHEMY_DATABASE_URI
for test_engine which reads POSTGRES_DB and currently points to the same
database as dev; update configuration so tests use an isolated DB: set
POSTGRES_DB to a distinct name (e.g., kaapi-guardrails-test) in test env
(.env.test or .env.test.example) or change the settings builder to detect
ENVIRONMENT=="testing" and append/replace the database name when constructing
SQLALCHEMY_DATABASE_URI; ensure references to test_engine,
settings.SQLALCHEMY_DATABASE_URI, and any teardown helpers like
clean_db/setup_test_db operate against that distinct DB to avoid accidental
deletion of development data.
🧹 Nitpick comments (4)
backend/app/tests/conftest.py (1)
29-34: Consider adding cleanup after test execution for complete isolation.The
clean_dbfixture cleans data before each test but doesn't yield, so cleanup doesn't run after the test. While this works for isolation (each test starts clean), adding ayieldwould ensure cleanup after test failures that might leave partial data.♻️ Optional improvement
`@pytest.fixture`(scope="function", autouse=True) def clean_db(): + yield with Session(test_engine) as session: for table in reversed(SQLModel.metadata.sorted_tables): session.execute(table.delete()) session.commit()backend/app/tests/test_validator_configs.py (2)
9-9: Remove unused import.
engineis imported fromapp.core.dbbut never used in this test file.♻️ Suggested fix
-from app.core.db import engine
80-91: Consider catchingHTTPExceptionspecifically instead of genericException.The test catches a generic
Exceptionbutget_or_404raisesHTTPException. Using the specific exception type makes the test more precise and documents the expected behavior.♻️ Optional improvement
+from fastapi import HTTPException + ... def test_not_found(self, mock_session): mock_session.get.return_value = None - with pytest.raises(Exception) as exc: + with pytest.raises(HTTPException) as exc: validator_config_crud.get_or_404( mock_session, TEST_VALIDATOR_ID, TEST_ORG_ID, TEST_PROJECT_ID, ) - assert "Validator not found" in str(exc.value) + assert exc.value.status_code == 404 + assert "Validator not found" in exc.value.detailbackend/app/api/routes/validator_configs.py (1)
81-91: Consider adding a response model for the delete endpoint.The delete endpoint returns
{"success": True}but lacks aresponse_model. While functional, adding a response model improves API documentation and type safety.♻️ Optional improvement
+from pydantic import BaseModel + +class DeleteResponse(BaseModel): + success: bool + -@router.delete("/{id}") +@router.delete("/{id}", response_model=DeleteResponse) async def delete_validator(
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.env.test.example:
- Around line 12-15: The dotenv-linter fails due to key ordering: move
POSTGRES_DB so it appears before POSTGRES_PORT in the environment block; update
the ordering of POSTGRES_SERVER, POSTGRES_DB, POSTGRES_PORT, POSTGRES_USER
(keeping their current values) so POSTGRES_DB precedes POSTGRES_PORT to satisfy
the linter and keep keys POSTGRES_SERVER, POSTGRES_DB, POSTGRES_PORT,
POSTGRES_USER in the correct order.
In `@backend/app/crud/__init__.py`:
- Line 1: Add a trailing newline at the end of the file that imports
RequestLogCrud to satisfy formatting (avoid W292); in the file containing the
import statement "from app.crud.request_log_repo import RequestLogCrud" simply
ensure the file ends with a newline character (i.e., blank line after the last
line).
🧹 Nitpick comments (3)
backend/app/schemas/validator_config.py (1)
32-35: Consider addingcreated_atandupdated_attoValidatorResponse.The CRUD's
flatten()method returns the full model dump including timestamps, butValidatorResponsedoesn't declare these fields. SinceValidatorBaseusesmodel_config = {"extra": "allow"}, FastAPI will still include them in responses, but they won't be documented in OpenAPI schemas.📝 Suggested enhancement
+from datetime import datetime + class ValidatorResponse(ValidatorBase): id: UUID org_id: int project_id: int + created_at: datetime + updated_at: datetimebackend/app/api/routes/validator_configs.py (1)
22-29: Async route handlers calling synchronous CRUD methods.The route handlers are defined as
async defbut invoke synchronousvalidator_config_crudmethods. This works in FastAPI (it runs sync code in a threadpool), but it's slightly less efficient than using sync handlers (def) for purely synchronous operations, or making the CRUD layer async.This is a minor consideration and may not warrant changes if the codebase convention is to use async handlers uniformly.
backend/app/tests/test_validator_configs_integration.py (1)
4-4: Unused importOperationalError.
OperationalErroris imported but never used in the test file.🧹 Suggested fix
import pytest -from sqlalchemy.exc import OperationalError from sqlmodel import Session, delete
AkhileshNegi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.env.test.example:
- Around line 12-16: The environment keys in the .env.test.example file are not
fully alphabetized; reorder the POSTGRES_* lines so they are in strict
alphabetical order (place POSTGRES_DB before POSTGRES_SERVER) to satisfy
dotenv-linter; specifically adjust the sequence of POSTGRES_DB,
POSTGRES_PASSWORD, POSTGRES_PORT, POSTGRES_SERVER, POSTGRES_USER (or alphabetize
all POSTGRES_* keys) so keys like POSTGRES_DB and POSTGRES_SERVER appear in
correct lexical order.
In `@backend/app/api/routes/validator_configs.py`:
- Around line 22-29: Change all route handlers from async def to def so they run
synchronously: update create_validator, list_validators, get_validator,
update_validator, and delete_validator to use def instead of async def; ensure
you remove any awaits inside those functions (there should be none since they
call synchronous CRUD methods like validator_config_crud.create / .get / .update
/ .delete and the SessionDep methods), keep signatures and return values the
same so FastAPI will run them in a threadpool.
In `@backend/app/models/config/validator_config_table.py`:
- Around line 48-58: The model's config field (SQLField named config) uses
default_factory=dict but its sa_column Column(JSONB, ...) lacks the
server_default present in the migration; update the Column(...) in
validator_config_table.py (the sa_column for config) to include
server_default=sa.text("'{}'::jsonb") so the database-level default matches
migration and the Python default remains unchanged.
🧹 Nitpick comments (15)
backend/app/alembic/versions/001_added_request_log.py (1)
17-19: Inconsistent type annotation style.Line 17 uses PEP 604 (
str | None) while lines 18–19 still useUnion[...]. This is purely cosmetic, but if you're modernizing annotations in this file, consider updating all three for consistency.backend/app/alembic/versions/002_added_validator_log.py (1)
17-19: Same annotation style inconsistency as in migration 001.
down_revisionuses the modern plainstrannotation whilebranch_labelsanddepends_onremainUnion[...]. Same optional cleanup opportunity as noted in the 001 migration.backend/app/crud/__init__.py (1)
1-1: Consider re-exportingvalidator_config_crudfor consistency.Other modules import
RequestLogCrudthrough this__init__.py. The newvalidator_config_crudsingleton is imported directly from its module elsewhere. If this file is meant to be the public surface for thecrudpackage, consider addingfrom app.crud.validator_config_crud import validator_config_crudhere as well.backend/app/alembic/versions/003_added_validator_config.py (2)
21-46: Migration looks solid overall.The table structure, JSONB default, unique constraint, and indexes are well-defined. A few observations:
created_atandupdated_atlackserver_defaultvalues, so any direct SQL inserts (outside the ORM) would fail with NOT NULL violations. The ORM model providesdefault_factory=now, but addingserver_default=sa.func.now()in the migration would be a useful safety net for non-ORM writes.The four single-column indexes (
idx_validator_organization,idx_validator_project,idx_validator_type,idx_validator_stage) add write overhead. The unique composite constraintuq_validator_identityalready creates an index on(organization_id, project_id, type, stage). If queries primarily filter by the leading columns of that composite index, the individual indexes onorganization_idandproject_idmay be partially redundant. Consider whether all four are needed based on actual query patterns.
49-50: Downgrade does not drop the individually-created indexes.
op.drop_tablewill cascade-drop the indexes tied to the table, so this works correctly in practice. However, for explicitness and symmetry withupgrade(), you could addop.drop_index(...)calls beforedrop_table.backend/app/tests/test_validator_configs.py (2)
79-90: CatchHTTPExceptioninstead of the broadException.
get_or_404raisesHTTPException(404, ...). CatchingExceptionis overly broad and could silently pass if a different unexpected exception is thrown (e.g.,AttributeError). Additionally, asserting on the status code would strengthen this test.Proposed fix
+from fastapi import HTTPException + ... def test_not_found(self, mock_session): mock_session.get.return_value = None - with pytest.raises(Exception) as exc: + with pytest.raises(HTTPException) as exc: validator_config_crud.get_or_404( mock_session, TEST_VALIDATOR_ID, TEST_ORGANIZATION_ID, TEST_PROJECT_ID, ) - assert "Validator not found" in str(exc.value) + assert exc.value.status_code == 404 + assert "Validator not found" in exc.value.detail
65-90: Missing test for org/project ID mismatch inget_or_404.The
get_or_404implementation (invalidator_config_crud.py, lines 74–76) also returns 404 when the record exists butorganization_idorproject_iddon't match. This authorization-adjacent check has no test coverage. Consider adding a test wheresession.getreturns a validator with a differentorganization_idorproject_idto verify the 404 is raised.backend/app/schemas/validator_config.py (1)
10-16:extra = "allow"on schemas accepts arbitrary unvalidated fields.Both
ValidatorBaseandValidatorUpdateusemodel_config = {"extra": "allow"}to funnel extra fields into the JSONBconfigcolumn. While this provides flexibility, it means any misspelled or unexpected field name will be silently accepted and stored. There's no schema-level validation or allow-list for which config keys are valid for a givenValidatorType.If this is intentional (truly open-ended config), consider at least documenting this behavior. If certain config keys are expected per validator type, a discriminated union or runtime validation would catch errors early.
backend/app/models/config/validator_config_table.py (1)
7-8: RedundantField as SQLFieldalias.Both
SQLField(line 7) andField(line 8) resolve tosqlmodel.Field. The alias adds confusion — readers may thinkSQLFieldis a distinct import. Only theconfigfield (line 48) usesSQLField; the rest useField.Proposed fix
-from sqlmodel import Field as SQLField -from sqlmodel import SQLModel, Field +from sqlmodel import SQLModel, FieldThen on line 48, replace
SQLField(withField(:- config: dict[str, Any] = SQLField( + config: dict[str, Any] = Field(backend/app/crud/validator_config_crud.py (2)
59-63: Truthiness checks on enum filters could silently skip a valid value with falsy representation.
if stage:andif type:use truthiness. If an enum member ever has a falsy value (e.g.,0or""), the filter would be silently skipped. Usingis not Noneis a safer, more explicit check.Suggested fix
- if stage: + if stage is not None: query = query.where(ValidatorConfig.stage == stage) - if type: + if type is not None: query = query.where(ValidatorConfig.type == type)
106-108:flattenletsconfigkeys silently overwrite base model fields.The merge order
{**base, **(row.config or {})}means any key inconfigthat collides with a base column name (e.g.,"type","stage") will shadow the real value. Whilesplit_validator_payloadshould prevent this on write paths, it may be worth adding a defensive assertion or filtering out known base-field keys fromconfigbefore merging.backend/app/api/routes/validator_configs.py (2)
29-30: Missing blank line betweencreate_validatorand the next route decorator.Line 29 (end of
create_validatorbody) flows directly into line 30 (@router.get). Add a blank line for readability, consistent with the other endpoint separations in this file.
80-90:deleteendpoint returns an untyped dict — consider adding aresponse_model.All other endpoints specify
response_model, butdeletereturns{"success": True}without one. A simple response model would keep the OpenAPI docs consistent and provide response validation.backend/app/tests/test_validator_configs_integration.py (2)
17-40:"lexical_slur"and"minimal"share the same unique key — maintenance trap.Both payloads have
type: "uli_slur_match"andstage: "input". If any future test creates both for the same org/project, it will hit the unique constraint and fail unexpectedly. Consider giving"minimal"a distincttypeorstageto avoid this footgun.
123-133: Setup creates are not asserted — silent failures would cause misleading test results.Lines 126–127 call
self.create_validator(...)without checking the response status. If a create fails (e.g., DB issue),test_list_validators_successwould still pass by seeing 0 results ≠ 2 — but the failure message would be confusing. The same pattern appears in other test methods (test_list_validators_filter_by_stage,test_list_validators_filter_by_type).Suggested fix (example for one test)
def test_list_validators_success(self, integration_client, clear_database): """Test successful validator listing.""" # Create validators first - self.create_validator(integration_client, "lexical_slur") - self.create_validator(integration_client, "pii_remover_input") + r1 = self.create_validator(integration_client, "lexical_slur") + assert r1.status_code == 200 + r2 = self.create_validator(integration_client, "pii_remover_input") + assert r2.status_code == 200
| POSTGRES_SERVER=localhost | ||
| POSTGRES_DB=kaapi_guardrails_testing | ||
| POSTGRES_PORT=5432 | ||
| POSTGRES_DB=kaapi-guardrails | ||
| POSTGRES_USER=postgres | ||
| POSTGRES_PASSWORD=postgres |
There was a problem hiding this comment.
Complete the alphabetical key ordering to resolve linter warnings.
The past review partially addressed the key ordering, but dotenv-linter still expects full alphabetical order. Currently POSTGRES_DB (line 13) should appear before POSTGRES_SERVER (line 12).
🔧 Complete alphabetical reordering fix
-POSTGRES_SERVER=localhost
POSTGRES_DB=kaapi_guardrails_testing
+POSTGRES_PASSWORD=postgres
POSTGRES_PORT=5432
+POSTGRES_SERVER=localhost
POSTGRES_USER=postgres
-POSTGRES_PASSWORD=postgres📝 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.
| POSTGRES_SERVER=localhost | |
| POSTGRES_DB=kaapi_guardrails_testing | |
| POSTGRES_PORT=5432 | |
| POSTGRES_DB=kaapi-guardrails | |
| POSTGRES_USER=postgres | |
| POSTGRES_PASSWORD=postgres | |
| POSTGRES_DB=kaapi_guardrails_testing | |
| POSTGRES_PASSWORD=postgres | |
| POSTGRES_PORT=5432 | |
| POSTGRES_SERVER=localhost | |
| POSTGRES_USER=postgres |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 13-13: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_SERVER key
(UnorderedKey)
[warning] 14-14: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
(UnorderedKey)
[warning] 16-16: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
(UnorderedKey)
🤖 Prompt for AI Agents
In @.env.test.example around lines 12 - 16, The environment keys in the
.env.test.example file are not fully alphabetized; reorder the POSTGRES_* lines
so they are in strict alphabetical order (place POSTGRES_DB before
POSTGRES_SERVER) to satisfy dotenv-linter; specifically adjust the sequence of
POSTGRES_DB, POSTGRES_PASSWORD, POSTGRES_PORT, POSTGRES_SERVER, POSTGRES_USER
(or alphabetize all POSTGRES_* keys) so keys like POSTGRES_DB and
POSTGRES_SERVER appear in correct lexical order.
| async def create_validator( | ||
| payload: ValidatorCreate, | ||
| session: SessionDep, | ||
| organization_id: int, | ||
| project_id: int, | ||
| _: AuthDep, | ||
| ): | ||
| return validator_config_crud.create(session, organization_id, project_id, payload) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
FastAPI async def endpoints synchronous database calls blocking event loop
💡 Result:
Yes—calling a synchronous (blocking) database client inside an async def FastAPI endpoint will block the event loop, reducing concurrency for that worker (other requests handled by the same event loop can’t make progress until the blocking DB call returns). (fastapi.tiangolo.com)
What to do instead (common patterns):
- Use an async DB driver / async ORM path (so you can truly
awaitthe DB I/O). - If your DB library is sync-only, either:
- Make the path operation sync:
def endpoint(...): ...so Starlette/FastAPI runs it in a thread pool rather than on the event loop. (starlette.dev) - Keep
async def, but offload the blocking DB call:await run_in_threadpool(sync_db_call, ...)(Starlette usesanyio.to_thread.run_syncunder the hood). (starlette.dev)
- Make the path operation sync:
Important caveat: the thread pool is limited (Starlette’s default limiter is 40 “tokens”), so heavy sync DB usage can still bottleneck—just in the pool instead of the event loop. (starlette.dev)
Citations:
- 1: https://fastapi.tiangolo.com/ja/async/?utm_source=openai
- 2: https://starlette.dev/threadpool/?utm_source=openai
- 3: https://starlette.dev/threadpool/?utm_source=openai
- 4: https://starlette.dev/threadpool/?utm_source=openai
🏁 Script executed:
# Find SessionDep definition
rg -A 5 "SessionDep" backend/app --max-count 5Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 4390
🏁 Script executed:
# Examine the validator_configs.py file to understand the imports and dependencies
cat -n backend/app/api/routes/validator_configs.py | head -40Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1373
🏁 Script executed:
# Find and inspect the validator_config_crud module
find backend -name "*crud*" -type f | head -20Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 119
🏁 Script executed:
cat -n backend/app/api/deps.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1438
🏁 Script executed:
cat -n backend/app/crud/validator_config_crud.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 3920
Remove async from all route handlers — async def with synchronous database calls blocks the event loop.
All five route handlers use async def but invoke synchronous CRUD methods (session.commit(), session.exec(), etc.). Since SessionDep provides a synchronous SQLModel Session, FastAPI will run these async def endpoints on the main event loop where blocking I/O prevents other concurrent requests from progressing.
Change all handlers to def (sync) so FastAPI automatically offloads them to a threadpool:
Suggested fix
-async def create_validator(
+def create_validator(
payload: ValidatorCreate,
session: SessionDep,
organization_id: int,
project_id: int,
_: AuthDep,
):
return validator_config_crud.create(session, organization_id, project_id, payload)Apply the same change to list_validators, get_validator, update_validator, and delete_validator.
📝 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.
| async def create_validator( | |
| payload: ValidatorCreate, | |
| session: SessionDep, | |
| organization_id: int, | |
| project_id: int, | |
| _: AuthDep, | |
| ): | |
| return validator_config_crud.create(session, organization_id, project_id, payload) | |
| def create_validator( | |
| payload: ValidatorCreate, | |
| session: SessionDep, | |
| organization_id: int, | |
| project_id: int, | |
| _: AuthDep, | |
| ): | |
| return validator_config_crud.create(session, organization_id, project_id, payload) |
🤖 Prompt for AI Agents
In `@backend/app/api/routes/validator_configs.py` around lines 22 - 29, Change all
route handlers from async def to def so they run synchronously: update
create_validator, list_validators, get_validator, update_validator, and
delete_validator to use def instead of async def; ensure you remove any awaits
inside those functions (there should be none since they call synchronous CRUD
methods like validator_config_crud.create / .get / .update / .delete and the
SessionDep methods), keep signatures and return values the same so FastAPI will
run them in a threadpool.
| config: dict[str, Any] = SQLField( | ||
| default_factory=dict, | ||
| sa_column=Column( | ||
| JSONB, | ||
| nullable=False, | ||
| comment="Configuration for the validator", | ||
| ), | ||
| description=( | ||
| "Configuration for the validator" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, run the provided verification script
rg -n "config" backend/app/crud/validator_config_crud.py -B2 -A2Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1237
🏁 Script executed:
#!/bin/bash
# Check the model file to confirm current state
cat -n backend/app/models/config/validator_config_table.py | head -70Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 2514
🏁 Script executed:
#!/bin/bash
# Find and check migration files related to validator_config
fd -e py "migration" --search-path . | grep -i validator | head -20Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 60
🏁 Script executed:
#!/bin/bash
# Search for migration files more broadly
fd "*.py" -path "*/migration*" | head -20Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 246
🏁 Script executed:
#!/bin/bash
# Search for alembic migrations or any migration-related files
find . -type f -name "*.py" | xargs grep -l "server_default.*jsonb" 2>/dev/null | head -10Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Look for migration files in common locations
find . -type d -name "migrations" -o -name "alembic" 2>/dev/nullRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for any reference to validator_config table creation or schema
rg -n "validator_config" --type py -A5 -B5 | grep -A10 -B10 "server_default\|alembic\|migration" | head -50Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 3978
🏁 Script executed:
#!/bin/bash
# Check if there are any env.py or other alembic setup files
find . -type f -name "env.py" -o -name "script.py.mako" 2>/dev/nullRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Read the migration file completely
cat -n backend/app/alembic/versions/003_added_validator_config.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 2223
config field has inconsistent defaults between model and migration.
The migration (003_added_validator_config.py, line 33) sets server_default=sa.text("'{}'::jsonb") on the config column, but the model definition here (line 50-54) uses sa_column=Column(JSONB, ...) without a server_default. The default_factory=dict provides only a Python-side default. Add server_default=sa.text("'{}'::jsonb") to the Column(...) definition to align with the migration.
🤖 Prompt for AI Agents
In `@backend/app/models/config/validator_config_table.py` around lines 48 - 58,
The model's config field (SQLField named config) uses default_factory=dict but
its sa_column Column(JSONB, ...) lacks the server_default present in the
migration; update the Column(...) in validator_config_table.py (the sa_column
for config) to include server_default=sa.text("'{}'::jsonb") so the
database-level default matches migration and the Python default remains
unchanged.
| def upgrade() -> None: | ||
| op.create_table('validator_config', | ||
| sa.Column('id', sa.Uuid(), nullable=False), | ||
| sa.Column('org_id', sa.Integer(), nullable=False), |
There was a problem hiding this comment.
name this to "organization_id"
| # revision identifiers, used by Alembic. | ||
| revision: str = '003' | ||
| down_revision: str = '002' | ||
| branch_labels: Union[str, Sequence[str], None] = None |
There was a problem hiding this comment.
what are these variables for , "branch labels" and "depends_on", if they are not being used in any way why are they there
| session: SessionDep, | ||
| _: AuthDep, | ||
| ): | ||
| obj = validator_config_crud.get_or_404(session, id, organization_id, project_id) |
There was a problem hiding this comment.
you can just let it ne "get" , it does not have to be "get or 404", thats making the name of crud function long for no reason
|
|
||
| @router.post( | ||
| "/", | ||
| response_model=ValidatorResponse |
There was a problem hiding this comment.
This should be APIRESPONSE[ValidatorResponse] to maintain the consistency of response bodies being returned everywhere, and if you dont think that we need this here then why
| @@ -1 +1 @@ | |||
| from app.crud.request_log import RequestLogCrud No newline at end of file | |||
| from app.crud.request_log_repo import RequestLogCrud | |||
There was a problem hiding this comment.
why does it need to have the word "repo" here, it does not make sense to name crud files with the word repo
| @@ -0,0 +1,85 @@ | |||
| from datetime import datetime | |||
There was a problem hiding this comment.
if you are making the design choice of having a seperate models folder and a seperate schema folder, then here also you dont need to specify with the filename that this file takes care of the table defination, you can emove the word table
| pass | ||
|
|
||
|
|
||
| class ValidatorUpdate(SQLModel): |
There was a problem hiding this comment.
are all these parameters not in validatorbase, this class can just inherit from that
| organization_id: int, | ||
| project_id: int, | ||
| payload: ValidatorCreate | ||
| ): |
There was a problem hiding this comment.
this does not have any return type
4 participants
Summary
Target issue is #31.
Explain the motivation for making this change. What existing problem does the pull request solve?
Currently, we have no way to manage validator config for each NGOs. So, we want to build APIs which manage validator configs.
The following APIs will be added -
Have also updated code organization to make it cleaner.
File-by-File Breakdown
APIs
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
New Features
Tests