diff --git a/CHANGELOG.md b/CHANGELOG.md index 740b36a..8d74eb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/cli/django_orm_lens/parser.py b/cli/django_orm_lens/parser.py index fe21251..dd86dac 100644 --- a/cli/django_orm_lens/parser.py +++ b/cli/django_orm_lens/parser.py @@ -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] @@ -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) + model.meta[key] = full_val + else: + model.meta[key] = raw_val k += 1 j = k continue diff --git a/cli/django_orm_lens/query_analyzer.py b/cli/django_orm_lens/query_analyzer.py index 8bb0e61..ed0a6ea 100644 --- a/cli/django_orm_lens/query_analyzer.py +++ b/cli/django_orm_lens/query_analyzer.py @@ -32,6 +32,7 @@ from typing import Any from .models import ( + ParsedModel, WorkspaceIndex, find_user_model, find_user_model_from_dict, @@ -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))) + + # --- 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"(? None: self.target = target_name @@ -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. @@ -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() @@ -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( { @@ -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 @@ -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): @@ -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) diff --git a/cli/tests/fixtures/golden/django-cms.snapshot.json b/cli/tests/fixtures/golden/django-cms.snapshot.json index 52a3091..aaba12a 100644 --- a/cli/tests/fixtures/golden/django-cms.snapshot.json +++ b/cli/tests/fixtures/golden/django-cms.snapshot.json @@ -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" @@ -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\")", diff --git a/cli/tests/fixtures/golden/saleor.snapshot.json b/cli/tests/fixtures/golden/saleor.snapshot.json index ef2517b..4ac25fc 100644 --- a/cli/tests/fixtures/golden/saleor.snapshot.json +++ b/cli/tests/fixtures/golden/saleor.snapshot.json @@ -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\"),)" }, @@ -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" @@ -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" @@ -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\")" }, @@ -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" @@ -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" }, @@ -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" @@ -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" } @@ -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\"]]" }, @@ -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\"]]" }, @@ -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\"]]" }, diff --git a/cli/tests/fixtures/golden/wagtail.snapshot.json b/cli/tests/fixtures/golden/wagtail.snapshot.json index 6412e44..e10f679 100644 --- a/cli/tests/fixtures/golden/wagtail.snapshot.json +++ b/cli/tests/fixtures/golden/wagtail.snapshot.json @@ -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\")" @@ -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\")" }, @@ -281,7 +281,7 @@ "filePath": "wagtail/models/pages.py", "lineNumber": 2851, "meta": { - "unique_together": "[" + "unique_together": "[ (\"page\", \"user\"), ]" }, "name": "PageSubscription" }, diff --git a/cli/tests/fixtures/golden/zulip.snapshot.json b/cli/tests/fixtures/golden/zulip.snapshot.json index ebc25bc..dddaa4a 100644 --- a/cli/tests/fixtures/golden/zulip.snapshot.json +++ b/cli/tests/fixtures/golden/zulip.snapshot.json @@ -90,7 +90,7 @@ "filePath": "zerver/models/messages.py", "lineNumber": 166, "meta": { - "indexes": "[" + "indexes": "[ GinIndex(\"search_tsvector\", fastupdate=False, name=\"zerver_message_search_tsvector\"), models.Index( # For moving messages between streams or marking # streams as read. The \"id\" at the end makes it easy # to scan the resulting messages in order, and perform # batching. \"realm_id\", \"recipient_id\", \"id\", name=\"zerver_message_realm_recipient_id\", ), models.Index( # For generating digest emails and message archiving, # which both group by stream. \"realm_id\", \"recipient_id\", \"date_sent\", name=\"zerver_message_realm_recipient_date_sent\", ), models.Index( # For exports, which want to limit both sender and # receiver. The prefix of this index (realm_id, # sender_id) can be used for scrubbing users and/or # deleting users' messages. # Also used in send_welcome_bot_response \"realm_id\", \"sender_id\", \"recipient_id\", name=\"zerver_message_realm_sender_recipient\", ), models.Index( # For analytics and retention queries \"realm_id\", \"date_sent\", name=\"zerver_message_realm_date_sent\", ), models.Index( # For users searching by topic (but not stream), which # is done case-insensitively \"realm_id\", Upper(\"subject\"), F(\"id\").desc(nulls_last=True), name=\"zerver_message_realm_upper_subject\", condition=Q(is_channel_message=True), ), models.Index( # Most stream/topic searches are case-insensitive by # topic name (e.g. messages_for_topic). The \"id\" at # the end makes it easy to scan the resulting messages # in order, and perform batching. \"realm_id\", \"recipient_id\", Upper(\"subject\"), F(\"id\").desc(nulls_last=True), name=\"zerver_message_realm_recipient_upper_subject\", condition=Q(is_channel_message=True), ), models.Index( # Used when determining recent topics (we post-process # to merge and show the most recent case) \"realm_id\", \"recipient_id\", \"subject\", F(\"id\").desc(nulls_last=True), name=\"zerver_message_realm_recipient_subject\", condition=Q(is_channel_message=True), ), models.Index( # Only used by update_first_visible_message_id \"realm_id\", F(\"id\").desc(nulls_last=True), name=\"zerver_message_realm_id\", ), models.Index( # Potentially useful for migrations that rewrite # message edit history. Originally added for # 0680_rename_general_chat_to_empty_string_topic, # though that migration was adjusted in a way that no # longer uses this. fields=[\"id\"], condition=Q(edit_history__isnull=False), name=\"zerver_message_edit_history_id\", ), ]" }, "name": "Message" }, @@ -202,7 +202,7 @@ "filePath": "zerver/models/messages.py", "lineNumber": 582, "meta": { - "indexes": "[" + "indexes": "[ models.Index( \"user_profile\", \"message\", condition=Q(flags__andnz=AbstractUserMessage.flags.starred.mask), name=\"zerver_usermessage_starred_message_id\", ), models.Index( \"user_profile\", \"message\", condition=Q(flags__andnz=AbstractUserMessage.flags.mentioned.mask), name=\"zerver_usermessage_mentioned_message_id\", ), models.Index( \"user_profile\", \"message\", condition=Q(flags__andz=AbstractUserMessage.flags.read.mask), name=\"zerver_usermessage_unread_message_id\", ), models.Index( \"user_profile\", \"message\", condition=Q(flags__andnz=AbstractUserMessage.flags.has_alert_word.mask), name=\"zerver_usermessage_has_alert_word_message_id\", ), models.Index( \"user_profile\", \"message\", condition=Q(flags__andnz=AbstractUserMessage.flags.mentioned.mask) | Q(flags__andnz=AbstractUserMessage.flags.stream_wildcard_mentioned.mask), name=\"zerver_usermessage_wildcard_mentioned_message_id\", ), models.Index( \"user_profile\", \"message\", condition=Q( flags__andnz=AbstractUserMessage.flags.mentioned.mask | AbstractUserMessage.flags.stream_wildcard_mentioned.mask | AbstractUserMessage.flags.topic_wildcard_mentioned.mask | AbstractUserMessage.flags.group_mentioned.mask ), name=\"zerver_usermessage_any_mentioned_message_id\", ), models.Index( \"user_profile\", \"message\", condition=Q(flags__andnz=AbstractUserMessage.flags.is_private.mask), name=\"zerver_usermessage_is_private_message_id\", ), models.Index( \"user_profile\", \"message\", condition=( Q(flags__andnz=AbstractUserMessage.flags.is_private.mask) & Q(flags__andz=AbstractUserMessage.flags.read.mask) ), name=\"zerver_usermessage_is_private_unread_message_id\", ), models.Index( \"user_profile\", \"message\", condition=Q( flags__andnz=AbstractUserMessage.flags.active_mobile_push_notification.mask ), name=\"zerver_usermessage_active_mobile_push_notification_id\", ), models.Index( \"message\", condition=Q( flags__andnz=AbstractUserMessage.flags.active_mobile_push_notification.mask ), name=\"zerver_usermessage_message_active_mobile_push_notification_idx\", ), ]" }, "name": "UserMessage" }, @@ -342,7 +342,7 @@ "filePath": "zerver/models/messages.py", "lineNumber": 802, "meta": { - "indexes": "[" + "indexes": "[ models.Index( \"realm\", \"create_time\", name=\"zerver_attachment_realm_create_time\", ), ]" }, "name": "Attachment" }, @@ -464,7 +464,7 @@ "filePath": "zerver/models/realm_audit_logs.py", "lineNumber": 215, "meta": { - "indexes": "[", + "indexes": "[ models.Index( name=\"zerver_realmauditlog_realm__event_type__event_time\", fields=[\"realm\", \"event_type\", \"event_time\"], ), models.Index( name=\"zerver_realmauditlog_user_subscriptions_idx\", fields=[\"modified_user\", \"modified_stream\"], condition=Q( event_type__in=[ AuditLogEventType.SUBSCRIPTION_CREATED, AuditLogEventType.SUBSCRIPTION_ACTIVATED, AuditLogEventType.SUBSCRIPTION_DEACTIVATED, ] ), ), models.Index( # Used in analytics/lib/counts.py for computing active users for realm_active_humans name=\"zerver_realmauditlog_user_activations_idx\", fields=[\"modified_user\", \"event_time\"], condition=Q( event_type__in=[ AuditLogEventType.USER_CREATED, AuditLogEventType.USER_ACTIVATED, AuditLogEventType.USER_DEACTIVATED, AuditLogEventType.USER_REACTIVATED, ] ), ), ]", "ordering": "[\"id\"]" }, "name": "RealmAuditLog" @@ -1701,7 +1701,7 @@ "filePath": "zerver/models/streams.py", "lineNumber": 32, "meta": { - "indexes": "[" + "indexes": "[ models.Index(Upper(\"name\"), name=\"upper_stream_name_idx\"), ]" }, "name": "Stream" }, @@ -1805,7 +1805,7 @@ "filePath": "zerver/models/streams.py", "lineNumber": 359, "meta": { - "indexes": "[", + "indexes": "[ models.Index( fields=(\"recipient\", \"user_profile\"), name=\"zerver_subscription_recipient_id_user_profile_id_idx\", condition=Q(active=True, is_user_active=True), ), ]", "unique_together": "(\"user_profile\", \"recipient\")" }, "name": "Subscription" @@ -1964,7 +1964,7 @@ "filePath": "zerver/models/streams.py", "lineNumber": 476, "meta": { - "indexes": "[", + "indexes": "[ models.Index( fields=(\"realm\", \"channel\"), name=\"zerver_channelemailaddress_realm_id_channel_id_idx\", ), ]", "unique_together": "(\"channel\", \"creator\", \"sender\")" }, "name": "ChannelEmailAddress" @@ -2248,8 +2248,8 @@ "filePath": "zerver/models/users.py", "lineNumber": 445, "meta": { - "constraints": "[", - "indexes": "[" + "constraints": "[ models.UniqueConstraint( \"realm\", Upper(F(\"email\")), name=\"zerver_userprofile_realm_id_email_uniq\", ), models.UniqueConstraint( \"realm\", Upper(F(\"delivery_email\")), name=\"zerver_userprofile_realm_id_delivery_email_uniq\", ), ]", + "indexes": "[ models.Index(Upper(\"email\"), name=\"upper_userprofile_email_idx\"), ]" }, "name": "UserProfile" }, @@ -2304,7 +2304,7 @@ "filePath": "zerver/models/users.py", "lineNumber": 1260, "meta": { - "constraints": "[" + "constraints": "[ models.UniqueConstraint( fields=[ \"realm\", \"external_auth_method_name\", \"external_auth_id\", ], name=\"zerver_externalauthid_uniq\", ), # Each user should only have at most a single ExternalAuthID # for any authentication method. models.UniqueConstraint( fields=[ \"user\", \"external_auth_method_name\", ], name=\"zerver_user_externalauth_uniq\", ), ]" }, "name": "ExternalAuthID" } diff --git a/cli/tests/test_query_analyzer.py b/cli/tests/test_query_analyzer.py index c7df762..c29570a 100644 --- a/cli/tests/test_query_analyzer.py +++ b/cli/tests/test_query_analyzer.py @@ -171,6 +171,102 @@ def test_get_call_is_captured(self) -> None: fields = {u["field"]: u["sites"] for u in result["filter_usages"] if not u.get("composite")} self.assertEqual(fields.get("pk"), 2) + def test_pk_and_id_not_proposed(self) -> None: + """Filtering on pk/id should never generate a proposal — it's the PK.""" + with TemporaryDirectory() as tmp: + root = Path(tmp) + _copy_miniapp(root) + _write( + root / "api.py", + "from orders.models import Order\n" + "def a(): return Order.objects.filter(pk=1)\n" + "def b(): return Order.objects.filter(pk=2)\n" + "def c(): return Order.objects.filter(id=3)\n" + "def d(): return Order.objects.filter(id=4)\n", + ) + + result = suggest_indexes("orders", "Order", str(root)) + proposed = [p["fields"] for p in result["proposed_indexes"]] + self.assertNotIn(["pk"], proposed) + self.assertNotIn(["id"], proposed) + + def test_db_index_true_field_not_proposed(self) -> None: + """Fields with db_index=True already have a DB index — no proposal.""" + with TemporaryDirectory() as tmp: + root = Path(tmp) + _write( + root / "myapp" / "__init__.py", "" + ) + _write( + root / "myapp" / "models.py", + "from django.db import models\n\n" + "class Widget(models.Model):\n" + " code = models.CharField(max_length=32, db_index=True)\n" + " name = models.CharField(max_length=64)\n", + ) + _write( + root / "views.py", + "from myapp.models import Widget\n" + "def a(): return Widget.objects.filter(code='x')\n" + "def b(): return Widget.objects.filter(code='y')\n", + ) + + result = suggest_indexes("myapp", "Widget", str(root)) + proposed = [p["fields"] for p in result["proposed_indexes"]] + self.assertNotIn(["code"], proposed) + + def test_unique_field_not_proposed(self) -> None: + """Fields declared unique=True carry an implicit index.""" + with TemporaryDirectory() as tmp: + root = Path(tmp) + _write(root / "shop" / "__init__.py", "") + _write( + root / "shop" / "models.py", + "from django.db import models\n\n" + "class Product(models.Model):\n" + " sku = models.CharField(max_length=32, unique=True)\n", + ) + _write( + root / "views.py", + "from shop.models import Product\n" + "def a(): return Product.objects.filter(sku='A1')\n" + "def b(): return Product.objects.filter(sku='A2')\n", + ) + + result = suggest_indexes("shop", "Product", str(root)) + proposed = [p["fields"] for p in result["proposed_indexes"]] + self.assertNotIn(["sku"], proposed) + + def test_unique_constraint_not_proposed(self) -> None: + """UniqueConstraint in Meta.constraints implies a DB index — skip it.""" + with TemporaryDirectory() as tmp: + root = Path(tmp) + _write(root / "inventory" / "__init__.py", "") + _write( + root / "inventory" / "models.py", + "from django.db import models\n\n" + "class Stock(models.Model):\n" + " warehouse = models.CharField(max_length=32)\n" + " product = models.CharField(max_length=32)\n\n" + " class Meta:\n" + " constraints = [\n" + " models.UniqueConstraint(\n" + " fields=['warehouse', 'product'], name='uc_stock'\n" + " )\n" + " ]\n", + ) + _write( + root / "views.py", + "from inventory.models import Stock\n" + "def a(): return Stock.objects.filter(warehouse='A', product='X')\n" + "def b(): return Stock.objects.filter(warehouse='B', product='Y')\n", + ) + + result = suggest_indexes("inventory", "Stock", str(root)) + proposed = [p["fields"] for p in result["proposed_indexes"]] + self.assertNotIn(["product", "warehouse"], proposed) + self.assertNotIn(["warehouse", "product"], proposed) + if __name__ == "__main__": unittest.main()