Skip to content

fix: suggest-indexes no longer proposes indexes for pk/id, db_index, unique, and UniqueConstraint #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
RinZ27 wants to merge 1 commit into from
Closed

fix: suggest-indexes no longer proposes indexes for pk/id, db_index, unique, and UniqueConstraint #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,22 @@ All notable changes to Django ORM Lens will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Fixed

- **`suggest-indexes` no longer proposes redundant indexes** for fields that
already have a DB index by other means: implicit primary-key columns (`pk`
and `id`), fields with `db_index=True`, fields with `unique=True`,
`ForeignKey` fields (which Django indexes by default), and composite indexes
declared via `Meta.unique_together` or `Meta.constraints`
(`UniqueConstraint`). Previously all of these were treated as uncovered,
producing noisy proposals for columns the DB had already indexed.
The parser's `class Meta` reader was also extended to capture multi-line
bracket values (`constraints = [...]`, `unique_together = [...]`) that were
previously truncated to a bare `[`.
Closes [#60](https://github.com/FROWNINGdev/django-orm-lens/issues/60).

## [py-1.8.1] - 2026-07-29

### Added
Expand Down
44 changes: 43 additions & 1 deletion cli/django_orm_lens/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,36 @@ def _read_balanced_args(lines: Sequence[str], start: int):
return "".join(parts).lstrip("("), len(lines) - 1


def _read_balanced_meta_value(lines: Sequence[str], start: int, col: int) -> tuple[str, int]:
"""Read a possibly-multi-line Meta attribute value starting at ``col`` on
line ``start``.

If the value begins with ``[`` or ``(``, collect lines until the bracket
is balanced and return the full text. Otherwise return the single-line
value as-is.
"""
first = lines[start][col:].rstrip()
opener = first[0] if first else ""
if opener not in ("[", "("):
return first, start
closer = "]" if opener == "[" else ")"
depth = 0
parts: list[str] = []
for i in range(start, len(lines)):
segment = lines[i][col:] if i == start else lines[i]
for ch in segment:
if ch == opener:
depth += 1
elif ch == closer:
depth -= 1
if depth == 0:
parts.append(ch)
return "".join(parts), i
parts.append(ch)
parts.append(" ") # join lines with a space for readable re-matching
return "".join(parts), len(lines) - 1


def _looks_like_model(base_classes: list[str]) -> bool:
for b in base_classes:
tail = b.split(".")[-1]
Expand Down Expand Up @@ -474,7 +504,19 @@ def _collect_defs(file_path: str, content: str) -> list[ParsedModel]:
break
m2 = rx["META_ITEM_RE"].match(ml)
if m2:
model.meta[m2.group(1)] = m2.group(2).strip()
key = m2.group(1)
raw_val = m2.group(2).strip()
# If the value opens a bracket that hasn't closed on
# this line, read ahead until the bracket balances.
if raw_val and raw_val[-1] in ("[", "(") or (
raw_val.count("[") > raw_val.count("]")
or raw_val.count("(") > raw_val.count(")")
):
val_col = ml.index(raw_val[0])
full_val, k = _read_balanced_meta_value(lines, k, val_col)
Comment on lines +515 to +516

@coderabbitai coderabbitai Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

model.meta[key] = full_val
else:
model.meta[key] = raw_val
k += 1
j = k
continue
Expand Down
80 changes: 73 additions & 7 deletions cli/django_orm_lens/query_analyzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
from typing import Any

from .models import (
ParsedModel,
WorkspaceIndex,
find_user_model,
find_user_model_from_dict,
Expand Down Expand Up @@ -159,6 +160,69 @@ def _existing_meta_indexes(meta: dict[str, str]) -> list[list[str]]:
return out


def _covered_index_sets(model: ParsedModel) -> set[tuple[str, ...]]:
"""Return every field combination that already has a DB index.

This covers:
* ``Meta.indexes`` entries
* ``Meta.unique_together`` tuples (which imply a composite index)
* ``Meta.constraints`` with ``UniqueConstraint(fields=[...])``
* Per-field ``primary_key=True``, ``db_index=True``, or ``unique=True``
* The implicit PK columns ``pk`` and ``id`` on models that rely on the
default auto-primary-key (no custom ``primary_key=True`` field declared)
"""
covered: set[tuple[str, ...]] = set()

# --- Meta.indexes ---
for fields in _existing_meta_indexes(model.meta):
covered.add(tuple(fields))

# --- Meta.unique_together ---
raw_ut = model.meta.get("unique_together")
if raw_ut:
for m in re.finditer(r"\(([^)]+)\)", raw_ut):
fields = re.findall(r"['\"]([^'\"]+)['\"]", m.group(1))
if fields:
covered.add(tuple(fields))
covered.add(tuple(sorted(fields)))

# --- Meta.constraints (UniqueConstraint) ---
raw_con = model.meta.get("constraints")
if raw_con:
for m in re.finditer(r"fields\s*=\s*\[([^\]]*)\]", raw_con):
fields = re.findall(r"['\"]([^'\"]+)['\"]", m.group(1))
if fields:
covered.add(tuple(fields))
covered.add(tuple(sorted(fields)))

Comment thread
coderabbitai[bot] marked this conversation as resolved.
# --- Field-level flags ---
has_custom_pk = any(
re.search(r"primary_key\s*=\s*True", f.args or "")
for f in model.fields
)
for f in model.fields:
args = f.args or ""
if re.search(r"primary_key\s*=\s*True", args):
covered.add((f.name,))
# Django maps `pk` as an alias to whatever the PK column is
covered.add(("pk",))
if re.search(r"db_index\s*=\s*True", args):
covered.add((f.name,))
if re.search(r"(?<![a-z_])unique\s*=\s*True", args):
covered.add((f.name,))
# ForeignKey implicitly creates a DB index unless db_index=False
if f.is_relation and f.relation_kind == "ForeignKey" and not re.search(r"db_index\s*=\s*False", args):
covered.add((f.name,))
covered.add((f.name + "_id",))

# --- Implicit auto-PK (id / pk) ---
if not has_custom_pk:
covered.add(("id",))
covered.add(("pk",))

return covered


class _Collector(ast.NodeVisitor):
def __init__(self, target_name: str, file_path: Path, root: Path) -> None:
self.target = target_name
Expand Down Expand Up @@ -302,7 +366,7 @@ def _propose_indexes(
filter_singles: list[dict[str, Any]],
filter_composites: list[dict[str, Any]],
order_by_summary: list[dict[str, Any]],
existing: list[list[str]],
covered: set[tuple[str, ...]],
) -> list[dict[str, Any]]:
"""Turn the frequency tables into concrete ``Meta.indexes`` suggestions.

Expand All @@ -314,9 +378,10 @@ def _propose_indexes(
* every order_by field with >=2 sites gets a proposal (unless already
covered by an existing index whose leading field matches).

Existing ``Meta.indexes`` are dedup'd by exact field-list match.
``covered`` is the full set of already-indexed field tuples produced by
:func:`_covered_index_sets` — includes Meta.indexes, field-level flags,
implicit PK columns, and unique constraints.
"""
existing_set = {tuple(x) for x in existing}
proposed: list[dict[str, Any]] = []
covered_leaders: set[str] = set()

Expand All @@ -325,7 +390,7 @@ def _propose_indexes(
continue
fields = combo["field"]
key = tuple(fields)
if key in existing_set:
if key in covered:
continue
proposed.append(
{
Expand All @@ -340,7 +405,7 @@ def _propose_indexes(
continue
field = single["field"]
key = (field,)
if key in existing_set:
if key in covered:
continue
if field in covered_leaders:
continue
Expand All @@ -356,7 +421,7 @@ def _propose_indexes(
continue
field = row["field"]
key = (field,)
if key in existing_set:
if key in covered:
continue
# Don't duplicate an existing filter proposal on the same bare field
if any(p["fields"] == [field] for p in proposed):
Expand Down Expand Up @@ -443,8 +508,9 @@ def suggest_indexes(
aggregate_summary = _summarise_order_by(all_aggregate) # same shape

existing = _existing_meta_indexes(target_model.meta)
covered = _covered_index_sets(target_model)
proposed = _propose_indexes(
filter_singles, filter_composites, order_by_summary, existing
filter_singles, filter_composites, order_by_summary, covered
)

filter_usages: list[dict[str, Any]] = list(filter_singles)
Expand Down
4 changes: 2 additions & 2 deletions cli/tests/fixtures/golden/django-cms.snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"lineNumber": 1108,
"meta": {
"app_label": "\"cms\"",
"constraints": "[",
"constraints": "[ UniqueConstraint(fields=[\"page\", \"language\"], name=\"unique_together_page_language\"), ]",
"default_permissions": "[]"
},
"name": "PageUrl"
Expand Down Expand Up @@ -171,7 +171,7 @@
"lineNumber": 153,
"meta": {
"app_label": "'cms'",
"indexes": "[",
"indexes": "[ models.Index(fields=['placeholder', 'language', 'position']), ]",
"ordering": "('position',)",
"unique_together": "('placeholder', 'language', 'position')",
"verbose_name": "_(\"plugin\")",
Expand Down
26 changes: 13 additions & 13 deletions cli/tests/fixtures/golden/saleor.snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@
"filePath": "saleor/discount/models.py",
"lineNumber": 269,
"meta": {
"indexes": "[",
"indexes": "[ BTreeIndex(fields=[\"voucher_code\"], name=\"vouchercustomer_voucher_code_idx\") ]",
"ordering": "(\"voucher_code\", \"customer_email\", \"pk\")",
"unique_together": "((\"voucher_code\", \"customer_email\"),)"
},
Expand Down Expand Up @@ -339,7 +339,7 @@
"filePath": "saleor/discount/models.py",
"lineNumber": 520,
"meta": {
"indexes": "[",
"indexes": "[ BTreeIndex( fields=[\"promotion_rule\"], name=\"orderdiscount_promotion_rule_idx\" ), # Orders searching index GinIndex(fields=[\"name\", \"translated_name\"]), GinIndex(fields=[\"voucher_code\"], name=\"orderdiscount_voucher_code_idx\"), ]",
"ordering": "(\"created_at\", \"id\")"
},
"name": "OrderDiscount"
Expand Down Expand Up @@ -372,8 +372,8 @@
"filePath": "saleor/discount/models.py",
"lineNumber": 542,
"meta": {
"constraints": "[",
"indexes": "[",
"constraints": "[ models.UniqueConstraint( fields=[\"line_id\", \"unique_type\"], name=\"unique_orderline_discount_type\", ), ]",
"indexes": "[ BTreeIndex( fields=[\"promotion_rule\"], name=\"orderlinedisc_promotion_rule_idx\" ), GinIndex(fields=[\"voucher_code\"], name=\"orderlinedisc_voucher_code_idx\"), ]",
"ordering": "(\"created_at\", \"id\")"
},
"name": "OrderLineDiscount"
Expand All @@ -399,7 +399,7 @@
"filePath": "saleor/discount/models.py",
"lineNumber": 574,
"meta": {
"indexes": "[",
"indexes": "[ BTreeIndex(fields=[\"promotion_rule\"], name=\"checkoutdiscount_rule_idx\"), # Orders searching index GinIndex(fields=[\"name\", \"translated_name\"]), GinIndex(fields=[\"voucher_code\"], name=\"checkoutdiscount_voucher_idx\"), ]",
"ordering": "(\"created_at\", \"id\")",
"unique_together": "(\"checkout_id\", \"promotion_rule_id\")"
},
Expand Down Expand Up @@ -433,8 +433,8 @@
"filePath": "saleor/discount/models.py",
"lineNumber": 594,
"meta": {
"constraints": "[",
"indexes": "[",
"constraints": "[ models.UniqueConstraint( fields=[\"line_id\", \"unique_type\"], name=\"unique_checkoutline_discount_type\", ), ]",
"indexes": "[ BTreeIndex( fields=[\"promotion_rule\"], name=\"checklinedisc_promotion_rule_idx\" ), GinIndex(fields=[\"voucher_code\"], name=\"checklinedisc_voucher_code_idx\"), ]",
"ordering": "(\"created_at\", \"id\")"
},
"name": "CheckoutLineDiscount"
Expand Down Expand Up @@ -588,7 +588,7 @@
"filePath": "saleor/order/models.py",
"lineNumber": 829,
"meta": {
"indexes": "["
"indexes": "[ BTreeIndex( fields=[\"reason_reference\"], name=\"fulfillmentline_reason_ref_idx\", condition=Q(reason_reference__isnull=False), ), ]"
},
"name": "FulfillmentLine"
},
Expand Down Expand Up @@ -667,7 +667,7 @@
"filePath": "saleor/order/models.py",
"lineNumber": 866,
"meta": {
"indexes": "[",
"indexes": "[ BTreeIndex(fields=[\"related\"], name=\"order_orderevent_related_id_idx\"), models.Index(fields=[\"type\"]), BTreeIndex(fields=[\"date\"], name=\"order_orderevent_date_idx\"), ]",
"ordering": "(\"date\",)"
},
"name": "OrderEvent"
Expand Down Expand Up @@ -847,7 +847,7 @@
"filePath": "saleor/order/models.py",
"lineNumber": 965,
"meta": {
"indexes": "["
"indexes": "[ BTreeIndex( fields=[\"reason_reference\"], name=\"grantedrefundline_reason_ref_idx\", condition=Q(reason_reference__isnull=False), ), ]"
},
"name": "OrderGrantedRefundLine"
}
Expand Down Expand Up @@ -941,7 +941,7 @@
"filePath": "saleor/product/models.py",
"lineNumber": 481,
"meta": {
"indexes": "[",
"indexes": "[ GinIndex(fields=[\"price_amount\", \"channel_id\"]), ]",
"ordering": "(\"pk\",)",
"unique_together": "[[\"variant\", \"channel\"]]"
},
Expand Down Expand Up @@ -1223,7 +1223,7 @@
"filePath": "saleor/warehouse/models.py",
"lineNumber": 635,
"meta": {
"indexes": "[",
"indexes": "[ models.Index(fields=[\"checkout_line\", \"reserved_until\"]), ]",
"ordering": "(\"pk\",)",
"unique_together": "[[\"checkout_line\", \"product_variant_channel_listing\"]]"
},
Expand Down Expand Up @@ -1275,7 +1275,7 @@
"filePath": "saleor/warehouse/models.py",
"lineNumber": 663,
"meta": {
"indexes": "[",
"indexes": "[ models.Index(fields=[\"checkout_line\", \"reserved_until\"]), ]",
"ordering": "(\"pk\",)",
"unique_together": "[[\"checkout_line\", \"stock\"]]"
},
Expand Down
6 changes: 3 additions & 3 deletions cli/tests/fixtures/golden/wagtail.snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
"filePath": "wagtail/models/pages.py",
"lineNumber": 297,
"meta": {
"permissions": "[",
"permissions": "[ (codename, name) for codename, _, name in PAGE_PERMISSION_TYPES if codename not in {\"add_page\", \"change_page\", \"delete_page\", \"view_page\"} ]",
"unique_together": "[(\"translation_key\", \"locale\")]",
"verbose_name": "_(\"page\")",
"verbose_name_plural": "_(\"pages\")"
Expand Down Expand Up @@ -153,7 +153,7 @@
"filePath": "wagtail/models/pages.py",
"lineNumber": 2155,
"meta": {
"constraints": "[",
"constraints": "[ models.UniqueConstraint( fields=(\"group\", \"page\", \"permission\"), name=\"unique_permission\", ), ]",
"verbose_name": "_(\"group page permission\")",
"verbose_name_plural": "_(\"group page permissions\")"
},
Expand Down Expand Up @@ -281,7 +281,7 @@
"filePath": "wagtail/models/pages.py",
"lineNumber": 2851,
"meta": {
"unique_together": "["
"unique_together": "[ (\"page\", \"user\"), ]"
},
"name": "PageSubscription"
},
Expand Down
Loading
Loading