fix: suggest-indexes no longer proposes indexes for pk/id, db_index, unique, and UniqueConstraint - #61
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesMeta parsing and index analysis
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Source as Django model source
participant Parser as _collect_defs
participant Coverage as _covered_index_sets
participant Proposals as _propose_indexes
participant Result as suggest_indexes
Source->>Parser: Parse Meta metadata
Parser-->>Coverage: Provide model metadata
Coverage->>Coverage: Combine declared and implicit coverage
Coverage-->>Proposals: Return covered field tuples
Proposals-->>Result: Return deduplicated proposals
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
6da45a9 to
856cb6b
Compare
![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
🤖 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.
Inline comments:
In `@cli/django_orm_lens/parser.py`:
- Around line 515-516: Update the Meta value parsing near
_read_balanced_meta_value to use the regex capture position m2.start(2) as
val_col instead of indexing raw_val[0]. Preserve the existing balanced-value
reading flow so bracketed type annotations are skipped and parsing begins at the
RHS value.
In `@cli/django_orm_lens/query_analyzer.py`:
- Around line 177-198: Update _existing_meta_indexes and the
unique_together/UniqueConstraint parsing in the covered-index analysis to
recognize positional arguments plus fields supplied as either lists or tuples.
Normalize every parsed declaration consistently, including sorting composite
field tuples where existing coverage matching expects it, and add regression
cases covering positional and list/tuple forms for indexes and UniqueConstraint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cb3f3db-0a20-4144-9e3c-44f379b73a07
📒 Files selected for processing (7)
cli/django_orm_lens/parser.pycli/django_orm_lens/query_analyzer.pycli/tests/fixtures/golden/django-cms.snapshot.jsoncli/tests/fixtures/golden/saleor.snapshot.jsoncli/tests/fixtures/golden/wagtail.snapshot.jsoncli/tests/fixtures/golden/zulip.snapshot.jsoncli/tests/test_query_analyzer.py
| val_col = ml.index(raw_val[0]) | ||
| full_val, k = _read_balanced_meta_value(lines, k, val_col) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the regex capture position for the Meta value.
Line 515 can select a bracket in a type annotation instead of the RHS: indexes: list[models.Index] = [ starts parsing at list[...] and truncates the actual value. Use m2.start(2).
Proposed fix
- val_col = ml.index(raw_val[0])
+ val_col = m2.start(2)📝 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.
| val_col = ml.index(raw_val[0]) | |
| full_val, k = _read_balanced_meta_value(lines, k, val_col) | |
| val_col = m2.start(2) | |
| full_val, k = _read_balanced_meta_value(lines, k, val_col) |
🤖 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 `@cli/django_orm_lens/parser.py` around lines 515 - 516, Update the Meta value
parsing near _read_balanced_meta_value to use the regex capture position
m2.start(2) as val_col instead of indexing raw_val[0]. Preserve the existing
balanced-value reading flow so bracketed type annotations are skipped and
parsing begins at the RHS value.
…db_index, unique, and UniqueConstraint fields Closes FROWNINGdev#60. The suggest-indexes command was ignoring several existing index forms when deciding what to propose, leading to noise on fields that already have a DB index by design: - Implicit PK columns (id / pk) — Django auto-creates an index for the primary key; filtering on pk= or id= should never generate a proposal. - Fields with db_index=True — explicit single-column index already present. - Fields with unique=True — a UNIQUE constraint carries an implicit index. - ForeignKey fields — Django adds a DB index by default unless db_index=False. - Meta.unique_together and Meta.constraints (UniqueConstraint) — both produce a composite index in the DB. Two changes to make this work: 1. parser.py: add _read_balanced_meta_value to read multi-line bracket values ([...] / (...)) inside class Meta. The old regex-per-line approach captured only the opening bracket for constraints = [...] and unique_together = [...] defined across several lines. Snapshots updated to reflect the now-complete capture. 2. query_analyzer.py: replace the narrow _existing_meta_indexes() dedup with _covered_index_sets(model), which collects all of the above into a single set<tuple> and is used directly by _propose_indexes(). Four regression tests added, one per previously-missed index form.
856cb6b to
250ce92
Compare
drift compared migrations against a model's own class body, so anything a model inherits from an abstract base read as migrated-but-no-longer-declared - four such fields per model on django-guardian. The parser now resolves abstract inheritance the way Django does, including across files; a concrete base still donates nothing, because multi-table inheritance leaves those columns on the parent's table. suggest-index read Meta.indexes and nothing else, so it recommended indexes Django had already created: the primary key, db_index, unique, foreign keys, unique_together and UniqueConstraint. pk and id were counted as two fields when they are one lookup. Underneath both: the Meta reader truncated any multi-line value to a bare opening bracket, which is why constraints were invisible. Both the list and the tuple spelling of fields= are read. Reported by @sevdog in #58 and #60. @RinZ27 independently found the same Meta-truncation cause for #60 in #61. Golden snapshots regenerated - the diffs are the previously truncated Meta values on django-cms, saleor, wagtail and zulip.
|
Thank you for this — and I owe you a clear account of what happened, because closing a correct PR needs one. Your diagnosis was right, including the part that was easy to miss: the parser truncating a multi-line What shipped in py-1.9.0 covers your seven points and two more that were still open:
The PR is now You are credited by name in the CHANGELOG entry and in the README roadmap as having found the same cause independently. That credit stands regardless of whose diff landed. Your Vietnamese translation in #54 is in the shipped docs, and this second look at |
2 participants
Summary
suggest-indexeswas treatingMeta.indexesas the only existing-index source, so it kept proposing indexes for columns the DB had already covered. Fixed for: implicit pk/id,db_index=True,unique=True,ForeignKey(implicitly indexed by Django),Meta.unique_together, andMeta.constraintswithUniqueConstraint. The parser was also silently truncating multi-lineclass Metabracket values —constraints = [...]defined across several lines was captured as[— so that is fixed too.Type of change
Test plan
Four regression tests added to
tests/test_query_analyzer.py, one per previously-missed index form, each building a synthetic workspace intmp_path:Golden snapshots regenerated with
UPDATE_GOLDEN_SNAPSHOTS=1 pytest tests/test_golden_snapshots.py— the diffs show the now-completeindexesvalues for django-cms, saleor, wagtail, and zulip that were previously truncated.Checklist
cd cli && pytest -qfor Python) and it is green## [Unreleased]mcp_server.pyand the relevant tests intest_mcp_server.py## Summaryabove and suggested a migration pathRelated issues / discussions
Closes #60