Opened 6 years ago

Closed 3 months ago

Last modified 3 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 13 months ago by Zach

Cc: zborboa@… added

comment:9 Changed 4 months ago by Simon Meers

Owner: changed from nobody to Simon Meers
Status: newassigned

comment:10 Changed 3 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 3 months ago by Simon Meers

Has patch: set
Needs documentation: unset

comment:12 Changed 3 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 3 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 3 months ago by Simon Meers

Patch needs improvement: unset

comment:15 Changed 3 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 3 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 3 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 60443e84:

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

comment:18 Changed 3 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