fix: ensure database model indexes

[buua436]

What problem does this PR solve?

Ensure database indexes declared in Peewee models are checked and created during database initialization.

This also adds the missing evaluation_runs.dialog_id column migration before index creation, preventing index creation failures on existing databases.

Type of change

• Bug Fix (non-breaking change which fixes an issue)

[coderabbitai]

📝 Walkthrough

Walkthrough

Database migration logic now derives expected Peewee indexes, creates missing database indexes, and invokes reconciliation during migrate_db(). Obsolete evaluation model declarations and an unused import were removed from api/db/db_models.py.

Changes

Database Index Reconciliation

Layer / File(s)	Summary
Model index reconciliation api/db/db_models.py	Adds ensure_model_indexes(migrator) to compare Peewee-declared indexes with database indexes and create missing entries.
Migration flow cleanup api/db/db_models.py	Runs index reconciliation within migrate_db() and removes obsolete evaluation model declarations and an unused import.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

I’m a bunny with indexes to sow,
Missing ones now neatly grow.
Peewee checks each table’s face,
Migrations put them in place.
Hop, hop—clean code joins the race!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description covers the problem and change type, but it misses the template's required Summary section.	Add a ### Summary section that briefly explains the PR's purpose and context, matching the repository template.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: creating missing database model indexes.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[wangq8]

Review: PR #16860 — fix: ensure database model indexes

Author: @buua436
Files: api/db/db_models.py (+39, −1)
Labels: bug, ci, size:M

Summary

This PR introduces ensure_model_indexes(migrator) — a generic function that introspects Peewee model declarations and creates any missing database indexes. It also adds the missing evaluation_runs.dialog_id column before calling the reconciliation, and removes the unused PrimaryKeyField import.

What looks good ✅

1. Comprehensive approach — ensure_model_indexes scans all DataBaseModel subclasses, collects field-level (field.index, field.unique) and model-level (_meta.indexes) indexes, compares them against DB.get_indexes(...), and creates what's missing. This is a solid pattern that will catch any future model index additions automatically.

2. Error handling is reasonable — Each index creation is wrapped in try/except with logging, so a single failure won't crash the entire migration.

3. Correct ordering — dialog_id column is added before the index reconciliation runs, which is the right sequence.

4. Idempotent — Existing indexes are skipped; duplicate column errors (MySQL 1060) are already handled by alter_db_add_column; index creation errors are caught.

5. Nice cleanup — Removing the unused PrimaryKeyField import.

Concerns & suggestions ⚠️

1. Potential data integrity issue: dialog_id = "" for existing rows

The dialog_id column is added with default="" and null=False. For any existing rows in the evaluation_runs table, this means they'll get an empty string "" as their dialog_id.

Looking at the model definition:

dialog_id = CharField(max_length=32, null=False, index=True, help_text="dialog configuration being evaluated")

Since dialog_id references a dialog configuration being evaluated, empty string rows may indicate an inconsistent state. Consider whether a backfill step is needed — e.g., for each existing row, relate it to the evaluation run's dataset agent, or allow NULL and handle it at the application layer.

Suggestion: Either:

• Set null=True on the column migration (but keep the model-level null=False as a forward-facing constraint), OR
• Add a data migration step to backfill dialog_id from the associated evaluation_cases or evaluation_datasets before the index creation.

2. Silent failure on unique constraint violations

When ensure_model_indexes tries to create a UNIQUE index on a column that already has duplicate values, the add_index call will fail and the error is logged but the migration continues silently. This could leave the database in a state where the model declares a unique constraint but the database doesn't enforce it.

Suggestion: For unique index failures, consider raising a critical error or at least logging with a louder level (logging.critical or logging.error with more context), so operators know they need to deduplicate data manually.

3. dialog_id index will be created redundantly by ensure_model_indexes

The dialog_id column is defined with index=True in the model and added via alter_db_add_column, but the migration only adds the column — not the index. The index is then picked up by ensure_model_indexes. This is intentional and works, but it's worth noting that the dialog_id index appears in the "missing" list during the reconciliation, resulting in a two-step column-then-index migration that could have been a single DDL statement.

Not a bug, just an observation — the current approach is fine for correctness.

4. Minor: scope of introspection

inspect.getmembers(sys.modules[__name__], inspect.isclass) will also pick up any test or utility classes defined in the same module that happen to inherit from DataBaseModel. In the current codebase this is unlikely to be an issue, but could be surprising if new model-like classes are added.

5. Performance on large databases

ensure_model_indexes calls DB.get_indexes(table_name) for every model subclass, then issues individual migrator.add_index calls for each missing index. On a busy production database with many tables, this one-time migration cost is acceptable, but it's worth being aware of.

Verdict

This is a well-structured PR that addresses a real gap — model-declared indexes not being created. The ensure_model_indexes function is a smart, maintainable solution. The main concern is the data integrity of existing evaluation_runs rows getting an empty dialog_id string, which should be reviewed.

Recommendation: Approve after considering the backfill question for existing evaluation_runs.dialog_id rows.

[coderabbitai]

🧹 Nitpick comments (2)

api/db/db_models.py (2)

1462-1462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename unused loop variable name to _name.

Ruff B007 flagged the unused loop variable.

♻️ Proposed fix

-    for name, model in members:
+    for _name, model in members:

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/db/db_models.py` at line 1462, In the loop over members, rename the
unused iteration variable name to _name while leaving model and the loop
behavior unchanged.

Source: Linters/SAST tools

---

1486-1493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Handle the unique-index-upgrade edge case more gracefully.

When a unique index is expected but only a non-unique one exists, the code attempts to create a unique index, which will fail because an index on those columns already exists. The error is caught and logged, but the log message reads as a creation failure rather than an upgrade limitation. Consider detecting this case and logging a more informative warning.

♻️ Proposed improvement

         for columns, unique in expected.items():
-            if columns in existing and (not unique or existing[columns]):
+            if columns in existing:
+                if not unique or existing[columns]:
                     continue
+                if unique and not existing[columns]:
+                    logging.warning(
+                        f"Index on {table_name} ({', '.join(columns)}) exists but is not unique; "
+                        f"manual upgrade required"
+                    )
+                    continue
             try:
                 migrate(migrator.add_index(table_name, columns, unique=unique))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/db/db_models.py` around lines 1486 - 1493, Update the index
reconciliation loop over expected and existing indexes to detect when a unique
index is expected but an existing index for the same columns is non-unique. Log
this as an upgrade limitation with a warning instead of attempting
migrate(migrator.add_index(...)); retain the current creation path for missing
indexes and already-correct unique indexes.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@api/db/db_models.py`:
- Line 1462: In the loop over members, rename the unused iteration variable name
to _name while leaving model and the loop behavior unchanged.
- Around line 1486-1493: Update the index reconciliation loop over expected and
existing indexes to detect when a unique index is expected but an existing index
for the same columns is non-unique. Log this as an upgrade limitation with a
warning instead of attempting migrate(migrator.add_index(...)); retain the
current creation path for missing indexes and already-correct unique indexes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0cb578b7-8121-4c30-a22c-7c6a601ac727

📥 Commits

Reviewing files that changed from the base of the PR and between 9e68100 and 4180376.

📒 Files selected for processing (1)

• api/db/db_models.py

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.16%. Comparing base (891261e) to head (4180376).
⚠️ Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16860      +/-   ##
==========================================
- Coverage   94.56%   93.16%   -1.40%     
==========================================
  Files          10       10              
  Lines         717      717              
  Branches      118      118              
==========================================
- Hits          678      668      -10     
- Misses         25       29       +4     
- Partials       14       20       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.