Add Config Management System with Version Control for LLM Providers #435
Conversation
…ncluding listing, retrieving, and deleting configurations and their versions.
…ding unique name validation and version retrieval.
…' across models and CRUD operations, and update version retrieval to use version numbers instead of IDs.
- List configurations ordered by updated_at in descending order. - List versions ordered by version number in descending order.
…n, reading, and deletion scenarios
WalkthroughAdds project-scoped configuration management: new Config and ConfigVersion SQLModel models, CRUD classes and package exports, API routes mounted at /configs (including version subroutes) with project permission checks, Alembic migration for two tables, test helpers, and comprehensive unit + integration tests covering CRUD, versions, pagination, soft-delete, and isolation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Config API
participant Auth as Auth/Permission
participant CRUD as ConfigCrud
participant DB as Database
rect `#E6F7FF`
Client->>API: POST /configs (ConfigCreate)
API->>Auth: require_permission(REQUIRE_PROJECT)
Auth-->>API: project context (project_id)
API->>CRUD: create_or_raise(ConfigCreate, project_id)
CRUD->>CRUD: _check_unique_name_or_raise
CRUD->>DB: INSERT Config
CRUD->>DB: INSERT ConfigVersion (version=1)
DB-->>CRUD: commit
CRUD-->>API: ConfigWithVersion
API-->>Client: 201 Created
end
sequenceDiagram
participant Client
participant API as Version API
participant Auth as Auth/Permission
participant VCRUD as ConfigVersionCrud
participant DB as Database
rect `#FFF4E6`
Client->>API: POST /configs/{id}/versions (VersionCreate)
API->>Auth: require_permission(REQUIRE_PROJECT)
Auth-->>API: project context
API->>VCRUD: create_or_raise(config_id, VersionCreate)
VCRUD->>VCRUD: _config_exists_or_raise -> _get_next_version
VCRUD->>DB: INSERT ConfigVersion (version = latest+1)
DB-->>VCRUD: commit
VCRUD-->>API: ConfigVersionPublic
API-->>Client: 201 Created
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 5
🧹 Nitpick comments (2)
backend/app/tests/utils/test_data.py (1)
276-282: Annotateconfig_idparameter.
config_idflows intoConfigVersionCrud, which expects aUUID. Leaving it untyped weakens type checking and obscures the intent. Please add the explicitUUIDannotation (and corresponding import) so the helper stays consistent with the rest of the module. As per coding guidelines.Apply this diff:
+from uuid import UUID @@ -def create_test_version( +def create_test_version( db: Session, - config_id, + config_id: UUID, project_id: int,backend/app/crud/config/version.py (1)
134-137: Return the config instance from_config_exists
The signature advertises aConfigreturn type, but the function currently returnsNone. Propagate theConfigCrud.existsresult so callers can rely on the declared type.- config_crud = ConfigCrud(session=self.session, project_id=self.project_id) - config_crud.exists(config_id) + config_crud = ConfigCrud(session=self.session, project_id=self.project_id) + return config_crud.exists(config_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/app/alembic/versions/e51b082aa9b3_config_management_tables.py(1 hunks)backend/app/api/main.py(2 hunks)backend/app/api/routes/config/__init__.py(1 hunks)backend/app/api/routes/config/config.py(1 hunks)backend/app/api/routes/config/version.py(1 hunks)backend/app/crud/config/__init__.py(1 hunks)backend/app/crud/config/config.py(1 hunks)backend/app/crud/config/version.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/config/__init__.py(1 hunks)backend/app/models/config/config.py(1 hunks)backend/app/models/config/version.py(1 hunks)backend/app/tests/api/routes/configs/test_config.py(1 hunks)backend/app/tests/api/routes/configs/test_version.py(1 hunks)backend/app/tests/crud/config/test_config.py(1 hunks)backend/app/tests/crud/config/test_version.py(1 hunks)backend/app/tests/utils/test_data.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/api/routes/config/__init__.pybackend/app/api/routes/config/version.pybackend/app/tests/api/routes/configs/test_version.pybackend/app/api/routes/config/config.pybackend/app/tests/crud/config/test_version.pybackend/app/tests/crud/config/test_config.pybackend/app/tests/api/routes/configs/test_config.pybackend/app/alembic/versions/e51b082aa9b3_config_management_tables.pybackend/app/crud/config/version.pybackend/app/models/config/version.pybackend/app/models/config/config.pybackend/app/api/main.pybackend/app/models/config/__init__.pybackend/app/tests/utils/test_data.pybackend/app/crud/config/__init__.pybackend/app/models/__init__.pybackend/app/crud/config/config.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/routes/config/__init__.pybackend/app/api/routes/config/version.pybackend/app/api/routes/config/config.pybackend/app/api/main.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/config/version.pybackend/app/crud/config/__init__.pybackend/app/crud/config/config.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/config/version.pybackend/app/models/config/config.pybackend/app/models/config/__init__.pybackend/app/models/__init__.py
🧠 Learnings (3)
📚 Learning: 2025-10-08T12:05:01.317Z
Learnt from: CR
Repo: ProjectTech4DevAI/ai-platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T12:05:01.317Z
Learning: Applies to backend/app/api/**/*.py : Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Applied to files:
backend/app/api/routes/config/__init__.pybackend/app/api/routes/config/config.py
📚 Learning: 2025-10-08T12:05:01.317Z
Learnt from: CR
Repo: ProjectTech4DevAI/ai-platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T12:05:01.317Z
Learning: Applies to backend/app/models/**/*.py : Define SQLModel entities (database tables and domain objects) in backend/app/models/
Applied to files:
backend/app/models/config/version.pybackend/app/models/config/config.py
📚 Learning: 2025-10-08T12:05:01.317Z
Learnt from: CR
Repo: ProjectTech4DevAI/ai-platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T12:05:01.317Z
Learning: Applies to backend/app/crud/**/*.py : Implement database access operations in backend/app/crud/
Applied to files:
backend/app/crud/config/config.py
🧬 Code graph analysis (14)
backend/app/api/routes/config/version.py (6)
backend/app/crud/config/config.py (5)
ConfigCrud(19-157)create(28-74)read_all(86-99)read_one(76-84)delete(124-130)backend/app/crud/config/version.py (5)
ConfigVersionCrud(15-137)create(25-62)read_all(78-100)read_one(64-76)delete(102-111)backend/app/models/config/version.py (3)
ConfigVersionCreate(58-59)ConfigVersionPublic(62-67)ConfigVersionItems(70-82)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(30-54)success_response(37-40)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/tests/api/routes/configs/test_version.py (4)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/utils/test_data.py (3)
create_test_config(236-272)create_test_project(52-68)create_test_version(275-304)backend/app/tests/conftest.py (2)
db(28-45)user_api_key(102-104)backend/app/tests/crud/config/test_version.py (1)
test_delete_version(280-293)
backend/app/api/routes/config/config.py (5)
backend/app/crud/config/config.py (6)
ConfigCrud(19-157)create(28-74)read_all(86-99)exists(132-140)update(101-122)delete(124-130)backend/app/models/config/config.py (5)
Config(23-47)ConfigCreate(50-65)ConfigUpdate(68-72)ConfigPublic(75-79)ConfigWithVersion(82-83)backend/app/models/config/version.py (1)
ConfigVersion(31-55)backend/app/utils.py (2)
APIResponse(30-54)success_response(37-40)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/tests/crud/config/test_version.py (5)
backend/app/models/config/version.py (1)
ConfigVersionCreate(58-59)backend/app/crud/config/version.py (1)
ConfigVersionCrud(15-137)backend/app/tests/utils/test_data.py (3)
create_test_project(52-68)create_test_config(236-272)create_test_version(275-304)backend/app/tests/conftest.py (1)
db(28-45)backend/app/crud/config/config.py (5)
create(28-74)read_one(76-84)delete(124-130)read_all(86-99)exists(132-140)
backend/app/tests/crud/config/test_config.py (5)
backend/app/models/config/config.py (2)
ConfigCreate(50-65)ConfigUpdate(68-72)backend/app/crud/config/config.py (9)
ConfigCrud(19-157)create(28-74)read_one(76-84)delete(124-130)read_all(86-99)update(101-122)exists(132-140)_check_unique_name(142-147)_read_by_name(149-157)backend/app/tests/utils/test_data.py (2)
create_test_project(52-68)create_test_config(236-272)backend/app/tests/utils/utils.py (1)
random_lower_string(20-21)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/tests/api/routes/configs/test_config.py (3)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/utils/test_data.py (2)
create_test_config(236-272)create_test_project(52-68)backend/app/tests/conftest.py (2)
db(28-45)user_api_key(102-104)
backend/app/crud/config/version.py (4)
backend/app/crud/config/config.py (6)
ConfigCrud(19-157)create(28-74)read_one(76-84)read_all(86-99)delete(124-130)exists(132-140)backend/app/core/util.py (1)
now(13-14)backend/app/models/config/config.py (1)
Config(23-47)backend/app/models/config/version.py (3)
ConfigVersion(31-55)ConfigVersionCreate(58-59)ConfigVersionItems(70-82)
backend/app/models/config/version.py (2)
backend/app/core/util.py (1)
now(13-14)backend/app/models/config/config.py (1)
validate_blob_not_empty(62-65)
backend/app/models/config/config.py (2)
backend/app/core/util.py (1)
now(13-14)backend/app/models/config/version.py (2)
ConfigVersionPublic(62-67)validate_blob_not_empty(25-28)
backend/app/models/config/__init__.py (2)
backend/app/models/config/config.py (6)
Config(23-47)ConfigBase(12-20)ConfigCreate(50-65)ConfigPublic(75-79)ConfigUpdate(68-72)ConfigWithVersion(82-83)backend/app/models/config/version.py (5)
ConfigVersion(31-55)ConfigVersionBase(13-28)ConfigVersionCreate(58-59)ConfigVersionPublic(62-67)ConfigVersionItems(70-82)
backend/app/tests/utils/test_data.py (6)
backend/app/models/config/config.py (1)
ConfigCreate(50-65)backend/app/models/config/version.py (2)
ConfigVersion(31-55)ConfigVersionCreate(58-59)backend/app/crud/config/config.py (1)
ConfigCrud(19-157)backend/app/crud/config/version.py (1)
ConfigVersionCrud(15-137)backend/app/tests/conftest.py (1)
db(28-45)backend/app/tests/utils/utils.py (1)
random_lower_string(20-21)
backend/app/crud/config/__init__.py (2)
backend/app/crud/config/config.py (1)
ConfigCrud(19-157)backend/app/crud/config/version.py (1)
ConfigVersionCrud(15-137)
backend/app/models/__init__.py (2)
backend/app/models/config/config.py (6)
Config(23-47)ConfigBase(12-20)ConfigCreate(50-65)ConfigUpdate(68-72)ConfigPublic(75-79)ConfigWithVersion(82-83)backend/app/models/config/version.py (5)
ConfigVersion(31-55)ConfigVersionBase(13-28)ConfigVersionCreate(58-59)ConfigVersionPublic(62-67)ConfigVersionItems(70-82)
backend/app/crud/config/config.py (4)
backend/app/models/config/config.py (3)
Config(23-47)ConfigCreate(50-65)ConfigUpdate(68-72)backend/app/models/config/version.py (1)
ConfigVersion(31-55)backend/app/core/util.py (1)
now(13-14)backend/app/crud/config/version.py (5)
create(25-62)read_one(64-76)read_all(78-100)exists(113-123)delete(102-111)
🪛 Ruff (0.14.4)
backend/app/tests/api/routes/configs/test_version.py
79-79: Unused function argument: db
(ARG001)
203-203: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
234-234: Unused function argument: db
(ARG001)
419-419: Unused function argument: db
(ARG001)
437-437: Unused function argument: db
(ARG001)
449-449: Unused function argument: db
(ARG001)
461-461: Unused function argument: db
(ARG001)
484-484: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
498-498: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
backend/app/tests/crud/config/test_config.py
197-197: Local variable config2 is assigned to but never used
Remove assignment to unused variable config2
(F841)
198-198: Local variable config3 is assigned to but never used
Remove assignment to unused variable config3
(F841)
backend/app/tests/api/routes/configs/test_config.py
12-12: Unused function argument: db
(ARG001)
47-47: Unused function argument: db
(ARG001)
74-74: Local variable config is assigned to but never used
Remove assignment to unused variable config
(F841)
192-192: Unused function argument: db
(ARG001)
290-290: Local variable config1 is assigned to but never used
Remove assignment to unused variable config1
(F841)
318-318: Unused function argument: db
(ARG001)
366-366: Unused function argument: db
(ARG001)
402-402: Unused function argument: db
(ARG001)
421-421: Unused function argument: db
(ARG001)
backend/app/crud/config/config.py
3-3: typing.Tuple is deprecated, use tuple instead
(UP035)
73-73: f-string without any placeholders
Remove extraneous f prefix
(F541)
backend/app/alembic/versions/e51b082aa9b3_config_management_tables.py
Outdated
Show resolved
Hide resolved
backend/app/alembic/versions/e51b082aa9b3_config_management_tables.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/alembic/versions/4523c8d1253d_config_management_tables.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/alembic/versions/4523c8d1253d_config_management_tables.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/app/crud/config/version.py (1)
125-134: Fix_get_next_versionto return an integer.
Session.exec(stmt).first()yields aRow(orNone), so adding1raises aTypeErrorand the very first version insert crashes. Fetch the scalar value and handle the empty case instead.- latest = self.session.exec(stmt).first() - return latest + 1 + latest = self.session.exec(stmt).scalar_one_or_none() + return (latest or 0) + 1
🧹 Nitpick comments (3)
backend/app/crud/config/config.py (2)
28-28: Use built-intuplein return annotation.
typing.Tupleis deprecated in favor of the nativetuple[...]; switching avoids lint noise and keeps annotations modern.- def create(self, config_create: ConfigCreate) -> Tuple[Config, ConfigVersion]: + def create(self, config_create: ConfigCreate) -> tuple[Config, ConfigVersion]:
71-74: Drop the redundant f-string prefix.There are no placeholders in the string, so the
fprefix is unnecessary and flagged by linters.raise HTTPException( status_code=500, - detail=f"Unexpected error occurred: failed to create config", + detail="Unexpected error occurred: failed to create config", )backend/app/crud/config/version.py (1)
4-4: Drop the unusedfuncimport.
funcisn’t referenced anywhere, and keeping it will trigger lint failures.-from sqlmodel import Session, select, and_, func +from sqlmodel import Session, select, and_
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
backend/app/alembic/versions/f245ccb27ab6_config_management_tables.py(1 hunks)backend/app/crud/config/config.py(1 hunks)backend/app/crud/config/version.py(1 hunks)backend/app/models/config/config.py(1 hunks)backend/app/models/config/version.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/alembic/versions/f245ccb27ab6_config_management_tables.pybackend/app/models/config/config.pybackend/app/crud/config/version.pybackend/app/crud/config/config.pybackend/app/models/config/version.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/config/config.pybackend/app/models/config/version.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/config/version.pybackend/app/crud/config/config.py
🧠 Learnings (1)
📚 Learning: 2025-10-08T12:05:01.317Z
Learnt from: CR
Repo: ProjectTech4DevAI/ai-platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T12:05:01.317Z
Learning: Applies to backend/app/models/**/*.py : Define SQLModel entities (database tables and domain objects) in backend/app/models/
Applied to files:
backend/app/models/config/config.pybackend/app/models/config/version.py
🧬 Code graph analysis (4)
backend/app/models/config/config.py (2)
backend/app/core/util.py (1)
now(13-14)backend/app/models/config/version.py (2)
ConfigVersionPublic(66-71)validate_blob_not_empty(25-28)
backend/app/crud/config/version.py (3)
backend/app/crud/config/config.py (4)
ConfigCrud(19-157)read_one(76-84)delete(124-130)exists(132-140)backend/app/core/util.py (1)
now(13-14)backend/app/models/config/version.py (3)
ConfigVersion(31-59)ConfigVersionCreate(62-63)ConfigVersionItems(74-86)
backend/app/crud/config/config.py (4)
backend/app/models/config/config.py (3)
Config(21-52)ConfigCreate(55-70)ConfigUpdate(73-77)backend/app/models/config/version.py (1)
ConfigVersion(31-59)backend/app/core/util.py (1)
now(13-14)backend/app/crud/config/version.py (5)
create(25-62)read_one(64-76)read_all(78-100)exists(113-123)delete(102-111)
backend/app/models/config/version.py (2)
backend/app/core/util.py (1)
now(13-14)backend/app/models/config/config.py (1)
validate_blob_not_empty(67-70)
🪛 Ruff (0.14.4)
backend/app/crud/config/config.py
3-3: typing.Tuple is deprecated, use tuple instead
(UP035)
73-73: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
backend/app/alembic/versions/f245ccb27ab6_config_management_tables.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/app/alembic/versions/f245ccb27ab6_config_management_tables.py (2)
20-20: Add return type hint.The function signature is missing a return type annotation.
As per coding guidelines, apply this diff to add the type hint:
-def upgrade(): +def upgrade() -> None:
80-80: Add return type hint.The function signature is missing a return type annotation.
As per coding guidelines, apply this diff to add the type hint:
-def downgrade(): +def downgrade() -> None:
🧹 Nitpick comments (1)
backend/app/alembic/versions/f245ccb27ab6_config_management_tables.py (1)
67-77: Clarify intent: redundant uniqueness constraint and index.The
config_versiontable has both aUniqueConstrainton(config_id, version)(lines 67-69) and a partial unique index on the same columns withWHERE deleted_at IS NULL(lines 71-77). The constraint already enforces uniqueness across all rows, making the partial index redundant for uniqueness enforcement (though it may help query performance on active rows).This differs from the
configtable pattern, which uses only a partial unique index on(project_id, name)to allow name reuse after soft-deletion. If version numbers should never be reused (consistent with "immutable versions"), then consider removing the partial unique index or documenting why both are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/alembic/versions/f245ccb27ab6_config_management_tables.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/alembic/versions/f245ccb27ab6_config_management_tables.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/ai-platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T12:05:01.317Z
Learning: Applies to backend/app/models/**/*.py : Define SQLModel entities (database tables and domain objects) in backend/app/models/
|
|
||
|
|
||
| class ConfigVersionBase(SQLModel): | ||
| config_blob: dict[str, Any] = Field( |
There was a problem hiding this comment.
if config table already has the blob why do nwe need it in the version table
There was a problem hiding this comment.
No config table does not have a blob,
Version table has it
| """ | ||
| Update a specific configuration. | ||
| """ | ||
| config_crud = ConfigCrud(session=session, project_id=current_user.project.id) |
There was a problem hiding this comment.
Assuming this a DB call, may be could be done in a service instead of exposing to the router.
There was a problem hiding this comment.
Since this route only calls the CRUD directly and has no extra logic, adding a service layer would be unnecessary overhead. We can introduce one later if the logic grows.
| ) | ||
|
|
||
|
|
||
| @router.delete( |
There was a problem hiding this comment.
could explore soft deleting instead.
There was a problem hiding this comment.
@Prajna1999 Did not understand this one
There was a problem hiding this comment.
As in not hard deleting from the database but toggling the flag that makes it invisible for GET-ing APIs but the entry remains in the db
| @router.delete( | ||
| "/{config_id}", | ||
| response_model=APIResponse[Message], | ||
| status_code=200, |
There was a problem hiding this comment.
nitpick: status code would be 204 I guess.
There was a problem hiding this comment.
204 is used when we return no content.
Here we return a message so 200 is acceptable.
| version = version_crud.create(version_create=version_create) | ||
|
|
||
| return APIResponse.success_response( | ||
| data=ConfigVersionPublic(**version.model_dump()), |
There was a problem hiding this comment.
nitpick: expand the keys of the dict object for better readability. Its better not to spread dicts without santizing the keys first.
There was a problem hiding this comment.
I get the point, but in this case the model is already validated and sanitized by Pydantic, so spreading the dict is safe. Expanding every field manually doesn’t add value and would just increase noise. I'd prefer to keep it as is unless we hit a specific issue.
| session=session, project_id=current_user.project.id, config_id=config_id | ||
| ) | ||
| version = version_crud.read_one(version_number=version_number) | ||
| if version is None: |
There was a problem hiding this comment.
enhancement: maybe explore extracting error handling at service level, although no hard and fast rule exists.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class ConfigVersionCrud: |
There was a problem hiding this comment.
General Comment: Usually we prefer class based structures for either 1) intializing a third party client 2) or where strict encapsulation is required like common adapters. May be for generic CRUD DB calls, simpler function based approach be useful where we needn't perform the overhead of class invocation; initializing a giant object every time.
There was a problem hiding this comment.
Mostly true, I also prefer functions when there's no shared state. In this case though, every CRUD method requires project_id, session, and for versions also config_id. Using a class avoids repeating these parameters across all functions and keeps the CRUD logic grouped and cleaner.
| .order_by(ConfigVersion.version.desc()) | ||
| .limit(1) | ||
| ) | ||
| latest = self.session.exec(stmt).first() |
There was a problem hiding this comment.
Not possible, but for safety returning 1 if latest is none
There was a problem hiding this comment.
how will return latest + 1 return 1 if latest is None? should be something like return (latest + 1) if latest else 1
i understand that the manner in which _get_next_version has been used, there will always be at least 1 version; but the method name is generic enough such that it can be used elsewhere, so we should handle this edge-case; what if someone manually deletes all versions of the config from the db (should NOT happen!), but while coding, we should still be defensive
| if self._read_by_name(name): | ||
| raise HTTPException( | ||
| status_code=409, | ||
| detail=f"Config with name '{name}' already exists in this project", |
There was a problem hiding this comment.
nitpicking - maintain either lowercase or uppercase
in some places config is used, others Config for the exceptions
| Index( | ||
| "idx_config_project_id_updated_at_active", | ||
| "project_id", | ||
| "updated_at", | ||
| postgresql_where=text("deleted_at IS NULL"), | ||
| ), |
There was a problem hiding this comment.
the read_all crud method uses .order_by(Config.updated_at.desc())
shoud we also include the DESC in the index? maybe something like this:
from sqlalchemy import desc
Index(
"idx_config_project_id_updated_at_active",
"project_id",
desc("updated_at"),
postgresql_where=text("deleted_at IS NULL"),
),
| Ordered by updated_at in descending order. | ||
| """ | ||
|
|
||
| # Decide how to handle pagination effectively |
There was a problem hiding this comment.
create a github issue for cursor based pagination enhancement - prefix the commment with TODO for easy filtering later on
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/alembic/versions/ecda6b144627_config_management_tables.py (1)
20-21: Add return type hints toupgrade/downgrade(per project typing guidelines)Both Alembic entrypoints are missing return annotations; align them with the rest of the codebase by marking them as returning
None:-def upgrade(): +def upgrade() -> None: @@ -def downgrade(): +def downgrade() -> None:Also applies to: 82-82
🧹 Nitpick comments (1)
backend/app/models/config/__init__.py (1)
1-29: Re-exports and__all__look consistent with config/version modelsImports and
__all__cleanly mirror the models inconfig.pyandversion.py, giving a clear public API for the package; no changes needed. If you want stricter typing, you could optionally annotate__all__aslist[str], but that’s not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/alembic/versions/ecda6b144627_config_management_tables.py(1 hunks)backend/app/api/routes/config/config.py(1 hunks)backend/app/api/routes/config/version.py(1 hunks)backend/app/crud/config/version.py(1 hunks)backend/app/models/config/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/api/routes/config/config.py
- backend/app/api/routes/config/version.py
- backend/app/crud/config/version.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/alembic/versions/ecda6b144627_config_management_tables.pybackend/app/models/config/__init__.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/config/__init__.py
🧬 Code graph analysis (1)
backend/app/models/config/__init__.py (2)
backend/app/models/config/config.py (6)
Config(21-52)ConfigBase(12-18)ConfigCreate(55-70)ConfigPublic(80-84)ConfigUpdate(73-77)ConfigWithVersion(87-88)backend/app/models/config/version.py (5)
ConfigVersion(31-59)ConfigVersionBase(13-28)ConfigVersionCreate(62-63)ConfigVersionPublic(66-71)ConfigVersionItems(74-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/alembic/versions/ecda6b144627_config_management_tables.py (1)
1-101: Migration schema matches the newConfig/ConfigVersionmodelsThe tables, constraints, and partial indexes line up with the SQLModel definitions (PKs, FKs with
ON DELETE CASCADE, unique(project_id, name)and(config_id, version), anddeleted_at IS NULLfilters), so this migration correctly supports the config/version + soft-delete semantics. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
backend/app/tests/crud/config/test_config.py (1)
197-199: Fix unused config variables in ordering test.Lines 197-198 create
config2andconfig3to populate the dataset but never reference them, triggering Ruff F841. Remove the variable assignments.Apply this diff:
- config2 = create_test_config(db, project_id=project.id, name="config-2") - config3 = create_test_config(db, project_id=project.id, name="config-3") + create_test_config(db, project_id=project.id, name="config-2") + create_test_config(db, project_id=project.id, name="config-3")
🧹 Nitpick comments (2)
backend/app/api/routes/config/config.py (1)
6-14: Remove unused imports.
Config(line 7) andConfigVersion(line 12) are imported but not used in this module.Apply this diff:
from app.models import ( - Config, ConfigCreate, ConfigUpdate, ConfigPublic, ConfigWithVersion, - ConfigVersion, Message, )backend/app/crud/config/version.py (1)
125-137: Simplify return type annotation.The return type on line 125 is annotated as
int | None, but the function always returns anint(either 1 orlatest + 1). Remove| Nonefor accuracy.Apply this diff:
- def _get_next_version(self, config_id: UUID) -> int | None: + def _get_next_version(self, config_id: UUID) -> int: """Get the next version number for a config."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/api/routes/config/config.py(1 hunks)backend/app/api/routes/config/version.py(1 hunks)backend/app/crud/config/config.py(1 hunks)backend/app/crud/config/version.py(1 hunks)backend/app/tests/crud/config/test_config.py(1 hunks)backend/app/tests/crud/config/test_version.py(1 hunks)backend/app/tests/utils/test_data.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/routes/config/version.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/tests/crud/config/test_version.pybackend/app/api/routes/config/config.pybackend/app/tests/crud/config/test_config.pybackend/app/crud/config/config.pybackend/app/tests/utils/test_data.pybackend/app/crud/config/version.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/routes/config/config.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/config/config.pybackend/app/crud/config/version.py
🧠 Learnings (1)
📚 Learning: 2025-10-08T12:05:01.317Z
Learnt from: CR
Repo: ProjectTech4DevAI/ai-platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T12:05:01.317Z
Learning: Applies to backend/app/api/**/*.py : Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Applied to files:
backend/app/api/routes/config/config.py
🧬 Code graph analysis (6)
backend/app/tests/crud/config/test_version.py (5)
backend/app/models/config/version.py (1)
ConfigVersionCreate(62-63)backend/app/crud/config/version.py (6)
ConfigVersionCrud(15-142)create_or_raise(25-62)read_one(64-76)delete_or_raise(102-111)read_all(78-100)exists_or_raise(113-123)backend/app/tests/utils/test_data.py (3)
create_test_project(52-68)create_test_config(236-272)create_test_version(275-304)backend/app/tests/conftest.py (1)
db(28-45)backend/app/crud/config/config.py (5)
create_or_raise(28-76)read_one(78-86)delete_or_raise(126-132)read_all(88-101)exists_or_raise(134-142)
backend/app/api/routes/config/config.py (6)
backend/app/crud/config/config.py (6)
ConfigCrud(19-159)create_or_raise(28-76)read_all(88-101)exists_or_raise(134-142)update_or_raise(103-124)delete_or_raise(126-132)backend/app/models/config/config.py (5)
Config(21-52)ConfigCreate(55-70)ConfigUpdate(73-77)ConfigPublic(80-84)ConfigWithVersion(87-88)backend/app/models/config/version.py (1)
ConfigVersion(31-59)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(30-54)success_response(37-40)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/tests/crud/config/test_config.py (5)
backend/app/models/config/config.py (2)
ConfigCreate(55-70)ConfigUpdate(73-77)backend/app/crud/config/config.py (8)
ConfigCrud(19-159)create_or_raise(28-76)read_one(78-86)delete_or_raise(126-132)read_all(88-101)update_or_raise(103-124)exists_or_raise(134-142)_read_by_name(151-159)backend/app/tests/utils/test_data.py (2)
create_test_project(52-68)create_test_config(236-272)backend/app/tests/utils/utils.py (1)
random_lower_string(20-21)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/crud/config/config.py (4)
backend/app/models/config/config.py (3)
Config(21-52)ConfigCreate(55-70)ConfigUpdate(73-77)backend/app/models/config/version.py (1)
ConfigVersion(31-59)backend/app/core/util.py (1)
now(13-14)backend/app/crud/config/version.py (5)
create_or_raise(25-62)read_one(64-76)read_all(78-100)exists_or_raise(113-123)delete_or_raise(102-111)
backend/app/tests/utils/test_data.py (6)
backend/app/models/config/config.py (1)
ConfigCreate(55-70)backend/app/models/config/version.py (2)
ConfigVersion(31-59)ConfigVersionCreate(62-63)backend/app/crud/config/config.py (2)
ConfigCrud(19-159)create_or_raise(28-76)backend/app/crud/config/version.py (2)
ConfigVersionCrud(15-142)create_or_raise(25-62)backend/app/tests/conftest.py (1)
db(28-45)backend/app/tests/utils/utils.py (1)
random_lower_string(20-21)
backend/app/crud/config/version.py (4)
backend/app/crud/config/config.py (6)
ConfigCrud(19-159)create_or_raise(28-76)read_one(78-86)read_all(88-101)delete_or_raise(126-132)exists_or_raise(134-142)backend/app/core/util.py (1)
now(13-14)backend/app/models/config/config.py (1)
Config(21-52)backend/app/models/config/version.py (3)
ConfigVersion(31-59)ConfigVersionCreate(62-63)ConfigVersionItems(74-86)
🪛 Ruff (0.14.5)
backend/app/tests/crud/config/test_config.py
197-197: Local variable config2 is assigned to but never used
Remove assignment to unused variable config2
(F841)
198-198: Local variable config3 is assigned to but never used
Remove assignment to unused variable config3
(F841)
backend/app/crud/config/config.py
3-3: typing.Tuple is deprecated, use tuple instead
(UP035)
75-75: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/app/tests/crud/config/test_config.py (1)
197-198: Fix unused config variables in ordering test.
config2andconfig3are only created to populate the dataset and end up unused, which Ruff flags asF841. Drop the assignments so the lint check passes.Apply this diff:
- config2 = create_test_config(db, project_id=project.id, name="config-2") - config3 = create_test_config(db, project_id=project.id, name="config-3") + create_test_config(db, project_id=project.id, name="config-2") + create_test_config(db, project_id=project.id, name="config-3")backend/app/crud/config/config.py (2)
3-3: Replace deprecatedTuplewithtuple.Python 3.9+ supports
tupledirectly without importing fromtyping. As per coding guidelines, this is a Python 3.11+ project.Apply this diff:
-from typing import TupleUpdate line 30:
- ) -> Tuple[Config, ConfigVersion]: + ) -> tuple[Config, ConfigVersion]:
75-75: Remove unnecessary f-string prefix.The string on line 75 has no placeholders; remove the
fprefix.Apply this diff:
- detail=f"Unexpected error occurred: failed to create config", + detail="Unexpected error occurred: failed to create config",
🧹 Nitpick comments (1)
backend/app/crud/config/config.py (1)
139-139: Standardize error message casing.Error messages use inconsistent casing: lowercase "config" at line 139 and uppercase "Config" at line 148. Consider standardizing to lowercase for consistency across error messages.
Based on learnings
Apply this diff:
- detail=f"Config with name '{name}' already exists in this project", + detail=f"config with name '{name}' already exists in this project",Also applies to: 148-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/config/config.py(1 hunks)backend/app/tests/crud/config/test_config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/crud/config/config.pybackend/app/tests/crud/config/test_config.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/config/config.py
🧬 Code graph analysis (2)
backend/app/crud/config/config.py (4)
backend/app/models/config/config.py (3)
Config(21-52)ConfigCreate(55-70)ConfigUpdate(73-77)backend/app/models/config/version.py (1)
ConfigVersion(31-59)backend/app/core/util.py (1)
now(13-14)backend/app/crud/config/version.py (5)
create_or_raise(25-62)read_one(64-76)read_all(78-100)exists_or_raise(113-123)delete_or_raise(102-111)
backend/app/tests/crud/config/test_config.py (5)
backend/app/models/config/config.py (2)
ConfigCreate(55-70)ConfigUpdate(73-77)backend/app/crud/config/config.py (9)
ConfigCrud(19-159)create_or_raise(28-76)read_one(78-86)delete_or_raise(126-132)read_all(88-101)update_or_raise(103-124)exists_or_raise(134-142)_check_unique_name_or_raise(144-149)_read_by_name(151-159)backend/app/tests/utils/test_data.py (2)
create_test_project(52-68)create_test_config(236-272)backend/app/tests/utils/utils.py (1)
random_lower_string(20-21)backend/app/tests/conftest.py (1)
db(28-45)
🪛 Ruff (0.14.5)
backend/app/crud/config/config.py
3-3: typing.Tuple is deprecated, use tuple instead
(UP035)
75-75: f-string without any placeholders
Remove extraneous f prefix
(F541)
backend/app/tests/crud/config/test_config.py
197-197: Local variable config2 is assigned to but never used
Remove assignment to unused variable config2
(F841)
198-198: Local variable config3 is assigned to but never used
Remove assignment to unused variable config3
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (5)
backend/app/crud/config/config.py (5)
28-77: LGTM: Config creation with initial version.The implementation correctly creates a config with an initial version in a single transaction. The use of
flush()to obtain the config ID before creating the version is appropriate, and exception handling with rollback is properly implemented.
78-102: LGTM: Read operations with proper scoping and soft-delete filtering.Both
read_oneandread_allcorrectly enforce project-level isolation and exclude soft-deleted records. The ordering byupdated_at.desc()and pagination support inread_allare implemented properly.
103-125: LGTM: Update logic with proper name uniqueness check.The update implementation correctly handles partial updates, validates name uniqueness only when the name is actually changing, and properly updates the
updated_attimestamp.
126-143: LGTM: Soft delete and existence check.The soft delete implementation correctly sets
deleted_atwithout removing the record. Theexists_or_raisehelper properly delegates toread_oneand provides appropriate 404 error handling.
144-159: LGTM: Name uniqueness validation helpers.The private helper methods correctly implement name-based lookups with proper project scoping and soft-delete filtering. The uniqueness check appropriately raises a 409 Conflict status when a duplicate name is detected.
| ) | ||
|
|
||
|
|
||
| @router.delete( |
There was a problem hiding this comment.
As in not hard deleting from the database but toggling the flag that makes it invisible for GET-ing APIs but the entry remains in the db
| version = version_crud.create(version_create=version_create) | ||
|
|
||
| return APIResponse.success_response( | ||
| data=ConfigVersionPublic(**version.model_dump()), |
Summary
Target issue is #430
This pull request implements the Config Management system.
The feature introduces project-scoped LLM configuration objects with immutable, versioned JSON definitions to ensure reproducible and stable LLM behavior across the platform.
Key Features
Config Entities
Versioned Configurations
Usage Model
Endpoints / Functionality Added
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
Database
Models
Tests
✏️ Tip: You can customize this high-level summary in your review settings.