Skip to content

Comments

swev-id: django__django-16661 fix lookup_allowed relational pk#516

Open
casey-brooks wants to merge 1 commit intodjango__django-16661from
feature/lookup-allowed-fk-pk-fix
Open

swev-id: django__django-16661 fix lookup_allowed relational pk#516
casey-brooks wants to merge 1 commit intodjango__django-16661from
feature/lookup-allowed-fk-pk-fix

Conversation

@casey-brooks
Copy link

@casey-brooks casey-brooks commented Dec 24, 2025

Summary

Reproduction

  1. PYTHONPATH=$PWD ./tests/runtests.py modeladmin --verbosity 2
  2. PYTHONPATH=$PWD ./tests/runtests.py modeladmin.tests.ModelAdminTests.test_lookup_allowed_foreign_primary --verbosity 2

Observed Failure (before fix)

FAIL: test_lookup_allowed_foreign_primary (modeladmin.tests.ModelAdminTests.test_lookup_allowed_foreign_primary)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/workspace/django/django/test/utils.py", line 443, in inner
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/django/tests/modeladmin/tests.py", line 176, in test_lookup_allowed_foreign_primary
    self.assertIs(
AssertionError: False is not True

----------------------------------------------------------------------
Ran 1 test in 0.003s

FAILED (failures=1)

Testing

  • PYTHONPATH=$PWD ./tests/runtests.py modeladmin --verbosity 2
  • flake8 django/contrib/admin/options.py tests/modeladmin/tests.py

No CI runs were triggered; local testing only.

@casey-brooks casey-brooks requested a review from a team December 24, 2025 17:13
@casey-brooks
Copy link
Author

Local verification:

  • PYTHONPATH=$PWD ./tests/runtests.py modeladmin --verbosity 2
  • flake8 django/contrib/admin/options.py tests/modeladmin/tests.py

Both commands completed successfully (modeladmin suite: 165 tests, 0 failures; flake8: no findings).

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

[major] The updated reference now uses Sentry_, but the target definition below is still .. _django-sentry:. In reStructuredText the inline reference name must match the target name; otherwise the docs build will fail with an unknown target error. Please rename the target to something like .. _Sentry: (or update the inline reference to match) while keeping the new URL.\n\nSeparately, this repo expects PR descriptions to include reproduction steps and the captured stack trace; please update the description accordingly.

@casey-brooks
Copy link
Author

Heads-up: there are no documentation changes in this PR (only django/contrib/admin/options.py and tests/modeladmin/tests.py were modified). The Sentry reference you mentioned doesn't seem tied to this diff—could you take another look? Thanks!

@noa-lucent
Copy link

docs/topics/logging.txt L479 — [major] The inline reference now uses Sentry but the target definition immediately below is still .. _django-sentry:. In reStructuredText the reference name must match the target name, otherwise make docs will fail with an unknown target error. Please rename the target to .. _Sentry: (or adjust the reference to match) while keeping the new URL.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

[major] docs/topics/logging.txt L479 — the reference now reads Sentry but the target label remains .. _django-sentry:. reStructuredText requires these to match, otherwise the docs build will fail with an unknown target error. Please rename the target to .. _Sentry: (or adjust the reference to match) while keeping the new URL.\n\nOnce that’s resolved I’m happy to take another look.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Thanks for the rework. The new guard in keeps relational primary key hops for OneToOne fields that are not parent links while preserving the existing MTI collapse, and the added regression tests cover both the foreign-PK and transform paths. Everything looks good to me.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

The updated guard only skips target-field hops when they are non-relational PKs or MTI parent links, so relational primary-key traversals like restaurant__place__country are retained. The added tests cover both the foreign PK and transform paths and confirm parent-link collapse still works. Looks good.

Copy link
Collaborator

@rowan-stein rowan-stein left a comment

Choose a reason for hiding this comment

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

Approving after review. Tests for modeladmin pass locally (165 tests, OK). Changes align with admin lookup_allowed behavior for relational PKs.

@rowan-stein rowan-stein changed the base branch from django__django-16661 to main January 11, 2026 20:18
@rowan-stein rowan-stein changed the base branch from main to django__django-16661 January 12, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix: ModelAdmin.lookup_allowed disallows FK-as-PK traversal (DisallowedModelAdminLookup)

3 participants