Opened 6 years ago

Closed 10 months ago

Last modified 10 months ago

#17985 closed New feature (fixed)

Document ModelAdmin.lookup_allowed()

Reported by: 3point2 Owned by: Simon Meers
Component: Documentation Version: 1.4
Severity: Normal Keywords:
Cc: lemaire.adrien@…, zborboa@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Right now, as a result of the security fix introduced in r15031, the only way to allow querystring lookups across relationships in the admin is to whitelist them by including them in list_filter.

However, in my application the lookup that needs to be whitelisted generates a huge filter widget as it contains thousands of instances.

It would be helpful if I could whitelist the exact lookup I need to link to without having to generate the filter widget itself.

Something like

class MyModelAdmin(ModelAdmin):
allow_lookup = ["fieldname__id__exact"]

would do. If the developers agree this is useful functionality, I could write a patch.

Change History (18)

comment:1 Changed 6 years ago by Julien Phalip

Resolution: wontfix
Status: newclosed

Thanks for the suggestion, but you can already easily achieve this by overriding the ModelAdmin.lookup_allowed() method. So there is no need for introducing a new class attribute.

comment:2 Changed 6 years ago by 3point2

Resolution: wontfix
Status: closedreopened

Sorry to re-open. I'm fine with overriding lookup_allowed, but I opened this ticket because I feel like this is a feature that is generally useful, and lookup_allowed is undocumented. I feel like this functionality should be officially supported, and overriding an undocumented method is more of a work-around. Also see http://www.hoboes.com/Mimsy/hacks/fixing-django-124s-suspiciousoperation-filtering/lookup_allowed-gets-new-parameter-value/

At the very least, documenting lookup_allowed would be helpful.

If on the other hand you feel that this functionality is not a common use case, I'm fine with closing the ticket and sticking with the work around.

comment:3 Changed 6 years ago by Adrien Lemaire

Cc: lemaire.adrien@… added
Easy pickings: set
Needs documentation: set
Summary: Add additional lookup_allowed whitelist functionality to ModelAdminAdd documentation for the lookup_allowed method
Triage Stage: UnreviewedAccepted

Renamed the ticket: Improving the documentation is a good idea.

comment:4 Changed 6 years ago by Julien Phalip

Triage Stage: AcceptedDesign decision needed

I'm not sure we want to document this method yet. It has been introduced recently (in 1.2.4) to fix a security issue, and has been modified quite a bit since then, so it's quite unstable. At the very least, this needs more thought before we make it part of the official API.

comment:5 Changed 5 years ago by Tim Graham

Component: contrib.adminDocumentation

comment:6 Changed 5 years ago by Aymeric Augustin

Status: reopenednew

comment:7 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset
Triage Stage: Design decision neededAccepted

Julien, do you think lookup_allowed can be considered stable now?

comment:8 Changed 20 months ago by Zach

Cc: zborboa@… added

comment:9 Changed 10 months ago by Simon Meers

Owner: changed from nobody to Simon Meers
Status: newassigned

comment:10 Changed 10 months ago by Simon Meers

See https://github.com/django/django/pull/8904

Though I suspect I didn't actually need to create three separate pull requests for the three (dependent but distinct) commits?

comment:11 Changed 10 months ago by Simon Meers

Has patch: set
Needs documentation: unset

comment:12 Changed 10 months ago by Tim Graham

Patch needs improvement: set
Summary: Add documentation for the lookup_allowed methodDocument ModelAdmin.lookup_allowed()

I suppose your thinking was that since ModelAdmin.lookup_allowed() isn't documented, it's okay to make a backwards-incompatible change in adding the request argument without a deprecation? Lately, Django has been more conservative about changing undocumented APIs without a deprecation if there's evidence that the API is at least somewhat widely used by projects. A GitHub code search reveals that's probably the case here.

If I were writing a patch, I would first document ModelAdmin.lookup_allowed() as it is now and backport that documentation to 1.11. (A separate pull request would ease merging that incrementally). Then I'd implement a deprecation and corresponding documentation changes for adding the request argument as part of #22569.

comment:13 Changed 10 months ago by Simon Meers

Thanks Tim, sounds like a good approach. Here is a documentation patch for the current implementation to start with: https://github.com/django/django/pull/8906

comment:14 Changed 10 months ago by Simon Meers

Patch needs improvement: unset

comment:15 Changed 10 months ago by Simon Meers

So the deprecation would involve:

  • Adding a heads-up in internals/deprecation.txt that request will be required in 2.0 (or do we have to go further ahead -- 2.1?)
  • Adding code to lookup_allowed in the current implementation that accepts both parameter formats (with/without request) and provides a deprecation warning if request is absent -- can this be in 1.11, or does that need to be in 2.0?
  • Then performing the change in https://github.com/django/django/pull/8904 (and removing the deprecation warning)
  • Hopefully https://github.com/django/django/pull/8903 can be merged in the meanwhile to close #28496 and lay the foundation for the simpler patch for #22569?

comment:16 Changed 10 months ago by Tim Graham

The deprecation should start in Django 2.0 and request would be required in Django 3.0 (use RemovedInDjango30Warning). You may find the checklist for Deprecating a feature helpful.

I plan to look at the patch for #28496 tomorrow.

comment:17 Changed 10 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 60443e84:

Fixed #17985 -- Documented ModelAdmin.lookup_allowed().

comment:18 Changed 10 months ago by Tim Graham <timograham@…>

In 07f73da:

[1.11.x] Fixed #17985 -- Documented ModelAdmin.lookup_allowed().

Backport of 60443e84b38ea3a143b0ef9c05b1e1f39d91ddb5 from master

Note: See TracTickets for help on using tickets.
Back to Top