Conversation
📝 WalkthroughWalkthroughAdds persistent validator configuration support: DB migration and SQLModel table, CRUD service and singleton, FastAPI routes, schemas, enums/constants, utility helpers, import reorganizations, and unit + integration tests. 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 + 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
| POSTGRES_SERVER=localhost | ||
| POSTGRES_PORT=5432 | ||
| POSTGRES_DB=kaapi-guardrails | ||
| POSTGRES_DB=kaapi_guardrails_testing | ||
| POSTGRES_USER=postgres |
There was a problem hiding this comment.
Fix dotenv-linter key order to avoid lint failure.
The linter expects POSTGRES_DB before POSTGRES_PORT. Consider reordering to keep lint green.
🔧 Suggested reorder
POSTGRES_SERVER=localhost
-POSTGRES_PORT=5432
POSTGRES_DB=kaapi_guardrails_testing
+POSTGRES_PORT=5432
POSTGRES_USER=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_PORT=5432 | |
| POSTGRES_DB=kaapi-guardrails | |
| POSTGRES_DB=kaapi_guardrails_testing | |
| POSTGRES_USER=postgres | |
| POSTGRES_DB=kaapi_guardrails_testing | |
| POSTGRES_PORT=5432 | |
| POSTGRES_SERVER=localhost | |
| POSTGRES_USER=postgres |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 13-13: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
(UnorderedKey)
[warning] 14-14: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
(UnorderedKey)
🤖 Prompt for AI Agents
In @.env.test.example around lines 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.
| @@ -1 +1 @@ | |||
| from app.crud.request_log import RequestLogCrud No newline at end of file | |||
| from app.crud.request_log_repo import RequestLogCrud No newline at end of file | |||
There was a problem hiding this comment.
Add trailing newline at EOF.
This avoids the W292 formatting warning.
✅ Suggested fix
from app.crud.request_log_repo import RequestLogCrud
+📝 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.
| from app.crud.request_log_repo import RequestLogCrud | |
| from app.crud.request_log_repo import RequestLogCrud | |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 1-1: No newline at end of file
Add trailing newline
(W292)
🤖 Prompt for AI Agents
In `@backend/app/crud/__init__.py` at 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).
2 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