Config: Fix legacy configs missing completion type field#614
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds an Alembic revision to backfill missing config completion Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (2)
backend/app/tests/crud/config/test_version.py (1)
422-424: Remove unusedexample_config_blobfixture parameter.Both
test_validate_immutable_fields_legacy_config_allows_text_updateandtest_validate_immutable_fields_legacy_config_rejects_non_text_updateacceptexample_config_blobbut never use it. This was also flagged by Ruff (ARG001).Proposed fix
def test_validate_immutable_fields_legacy_config_allows_text_update( - db: Session, example_config_blob: ConfigBlob + db: Session, ) -> None:def test_validate_immutable_fields_legacy_config_rejects_non_text_update( - db: Session, example_config_blob: ConfigBlob + db: Session, ) -> None:Also applies to: 458-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/crud/config/test_version.py` around lines 422 - 424, Both tests include an unused fixture parameter example_config_blob causing ARG001; remove the unused fixture from the function signatures of test_validate_immutable_fields_legacy_config_allows_text_update and test_validate_immutable_fields_legacy_config_rejects_non_text_update (leave db: Session if used) so the tests no longer accept example_config_blob, and run tests to ensure no references to example_config_blob remain inside those functions.backend/app/crud/config/version.py (1)
281-293: Inconsistent log formatting forold_type/new_type.On line 288,
old_typeandnew_typeare logged without quotes ({old_type}), while on line 298 they are quoted ('{old_type}'). This produces inconsistent structured log output.Proposed fix
- f"{{'config_id': '{self.config_id}', 'old_type': {old_type}, 'new_type': {new_type}}}" + f"{{'config_id': '{self.config_id}', 'old_type': '{old_type}', 'new_type': '{new_type}'}}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/config/version.py` around lines 281 - 293, The logger.error call in ConfigVersionCrud._validate_config_type_unchanged logs old_type and new_type inconsistently (unquoted vs quoted); update the logger.error invocation to format old_type and new_type consistently (e.g., wrap both in quotes like '{old_type}' and '{new_type}' or use repr/str consistently) so the structured message f"...{{'config_id': '{self.config_id}', 'old_type': '{old_type}', 'new_type': '{new_type}'}}" produces uniform output; change only the logger.error string formatting around old_type/new_type in version.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/alembic/versions/047_backfill_legacy_config_completion_type.py`:
- Around line 27-34: The current downgrade() function removes completion.type
from any row where type='text', which will strip this required field from
legitimately created configs; change downgrade to be a safe no-op (i.e., remove
the destructive UPDATE and replace it with a comment explaining why rollback is
unsafe) or implement a tracking-based reversal (add/consult a backfilled marker
such as a backfilled_at timestamp set by the upgrade and only remove type for
rows with that marker). Update the downgrade() implementation (the downgrade
function) to perform the safe no-op or to conditionally target only backfilled
rows using the chosen marker, and include a clear comment in the function
explaining why deletion of a mandatory field is destructive.
In `@backend/app/tests/crud/config/test_version.py`:
- Around line 494-525: Tests call _validate_config_type_unchanged directly but
production path doesn't, so either remove the dead method or integrate it into
the public creation flow; update ConfigVersionCrud.create_or_raise to call
self._validate_config_type_unchanged(...) (alongside the existing
self._validate_immutable_fields(...) call) before persisting new versions, or
delete _validate_config_type_unchanged and move its logic into
_validate_immutable_fields if you want a single validation path, ensuring
ConfigVersionCrud and tests are updated accordingly.
---
Nitpick comments:
In `@backend/app/crud/config/version.py`:
- Around line 281-293: The logger.error call in
ConfigVersionCrud._validate_config_type_unchanged logs old_type and new_type
inconsistently (unquoted vs quoted); update the logger.error invocation to
format old_type and new_type consistently (e.g., wrap both in quotes like
'{old_type}' and '{new_type}' or use repr/str consistently) so the structured
message f"...{{'config_id': '{self.config_id}', 'old_type': '{old_type}',
'new_type': '{new_type}'}}" produces uniform output; change only the
logger.error string formatting around old_type/new_type in version.py.
In `@backend/app/tests/crud/config/test_version.py`:
- Around line 422-424: Both tests include an unused fixture parameter
example_config_blob causing ARG001; remove the unused fixture from the function
signatures of test_validate_immutable_fields_legacy_config_allows_text_update
and test_validate_immutable_fields_legacy_config_rejects_non_text_update (leave
db: Session if used) so the tests no longer accept example_config_blob, and run
tests to ensure no references to example_config_blob remain inside those
functions.
backend/app/alembic/versions/047_backfill_legacy_config_completion_type.py
Outdated
Show resolved
Hide resolved
| def test_validate_config_type_unchanged_legacy_config_allows_text( | ||
| db: Session, | ||
| ) -> None: | ||
| """Test that _validate_config_type_unchanged defaults legacy (null type) to 'text'.""" | ||
| config = create_test_config(db) | ||
| version_crud = ConfigVersionCrud( | ||
| session=db, project_id=config.project_id, config_id=config.id | ||
| ) | ||
|
|
||
| # Directly patch the latest version's config_blob to remove 'type' (simulate legacy) | ||
| latest_version = version_crud._get_latest_version() | ||
| blob = dict(latest_version.config_blob) | ||
| blob["completion"] = {k: v for k, v in blob["completion"].items() if k != "type"} | ||
| latest_version.config_blob = blob | ||
| db.add(latest_version) | ||
| db.commit() | ||
|
|
||
| # Creating a new version with type="text" should succeed | ||
| new_blob = ConfigBlob( | ||
| completion=NativeCompletionConfig( | ||
| provider="openai-native", | ||
| type="text", | ||
| params={"model": "gpt-4", "temperature": 0.8, "max_tokens": 1500}, | ||
| ) | ||
| ) | ||
| version_create = ConfigVersionCreate( | ||
| config_blob=new_blob, | ||
| commit_message="Update after legacy", | ||
| ) | ||
|
|
||
| # Should NOT raise | ||
| version_crud._validate_config_type_unchanged(version_create) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '_validate_config_type_unchanged' --type=py -C3Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 4332
🏁 Script executed:
rg -n 'def create_or_raise' --type=py -A30 backend/app/crud/config/version.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 1374
🏁 Script executed:
rg -n 'def create_or_raise' --type=py -A60 backend/app/crud/config/version.py | head -80Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2585
🏁 Script executed:
rg '_validate_config_type_unchanged' --type=py backend/app/crud/ backend/app/routers/ backend/app/schemas/ backend/app/db/ -nRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 608
🏁 Script executed:
rg '_validate_config_type_unchanged' --type=pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 1135
Remove or integrate _validate_config_type_unchanged into the actual code path.
The method is never called from create_or_raise (the public entry point) or any other production code—only directly from tests. The tests exercise this method in isolation, giving false confidence that it works. Either the method is dead code and should be removed, or it should be integrated into the actual validation flow (likely in create_or_raise at line 61 where _validate_immutable_fields is called).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tests/crud/config/test_version.py` around lines 494 - 525, Tests
call _validate_config_type_unchanged directly but production path doesn't, so
either remove the dead method or integrate it into the public creation flow;
update ConfigVersionCrud.create_or_raise to call
self._validate_config_type_unchanged(...) (alongside the existing
self._validate_immutable_fields(...) call) before persisting new versions, or
delete _validate_config_type_unchanged and move its logic into
_validate_immutable_fields if you want a single validation path, ensuring
ConfigVersionCrud and tests are updated accordingly.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/alembic/versions/047_backfill_legacy_config_completion_type.py (1)
16-24: Upgrade logic looks correct for the intended backfill.The SQL properly targets only rows where
completionexists buttypeis missing, and sets it to"text", which aligns with the learned constraint that valid values are"text","stt", or"tts".Minor edge case: if any row has
config_blob->'completion'set to JSONnull(as opposed to the key being absent), it would pass theIS NOT NULLcheck butjsonb_setthrough a JSON null could behave unexpectedly. Consider tightening the WHERE clause if this is possible in your data:WHERE config_blob->'completion' IS NOT NULL + AND jsonb_typeof(config_blob->'completion') = 'object' AND config_blob->'completion'->>'type' IS NULL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/alembic/versions/047_backfill_legacy_config_completion_type.py` around lines 16 - 24, The upgrade() migration currently updates rows where config_blob->'completion' IS NOT NULL but that will include JSON null values; modify the WHERE clause in the op.execute SQL so it only targets real objects (e.g. add a check like jsonb_typeof(config_blob->'completion') = 'object' or config_blob->'completion' <> 'null'::jsonb) in addition to the existing config_blob->'completion'->>'type' IS NULL predicate so jsonb_set is only called on object completions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/alembic/versions/047_backfill_legacy_config_completion_type.py`:
- Around line 16-24: The upgrade() migration currently updates rows where
config_blob->'completion' IS NOT NULL but that will include JSON null values;
modify the WHERE clause in the op.execute SQL so it only targets real objects
(e.g. add a check like jsonb_typeof(config_blob->'completion') = 'object' or
config_blob->'completion' <> 'null'::jsonb) in addition to the existing
config_blob->'completion'->>'type' IS NULL predicate so jsonb_set is only called
on object completions.
3 participants
Summary
Target issue is #605
Explain the motivation for making this change. What existing problem does the pull request solve?
To make the create new version for existing legacy config to support completion.type where it is null
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
Bug Fixes
Chores
Tests