Optimize calling of `get_app_list` with AdminSites index/app_index

[Daniel Hahler]

AdminSite.index gets the list of available apps twice: for app_list and available_apps in the context.

Code ref: ​https://github.com/django/django/blob/bee09df39fe0734eb2388a2b3439436796592820/django/contrib/admin/sites.py#L547-L555

I think this can be optimized to only call it once.

With app_index it could also be filtered in app_index itself.

The benefit of this is handling the use case of moving apps around in get_app_list (from custom_auth into auth, after calling the default method), which fails when the list is filtered by app_label before.

I think changing index makes sense, and is the far more often used code path, but changing app_index might break some projects, where by now it might be expected that get_app_list gets called with an app_label - and optimizing it there removed the internal use of calling get_app_list with app_label completely again. (This was changed in Django 4.1: https://code.djangoproject.com/ticket/7497)

[Carlton Gibson]

Hey Daniel.

I think at least for the index case it does make sense. My only reservation there is whether the micro-optimisation justifies the change in readability: get_app_list() is clear as day, whereas I might need to dig down to see why we're doing "app_list": context["available_apps"]. A comment there in the two cases would be enough I guess, Aliasing for the template context, (or not).

The app_index case is more complex. It looks like maybe the app_label usage could be teased out/simplified, but it's clearly old/delicate. I wonder how much effort you want to put in here? Do you want to deprecate the (4.1 added) app_label argument, as unused?

What about (then) simplifying _build_app_dict() — the label parameter would be unused, and there's a block of code using that that could be removed? If we're going to do this, I think we should clean that up too?

If you're happy to take that on, then happy to progress to see what others think, given that _build_app_dict() is somewhat complex (to read at least). Otherwise we can wontfix as it's likely marginal.

The benefit of this is handling the use case of moving apps around in get_app_list (from custom_auth into auth, after calling the default method), which fails when the list is filtered by app_label before.

I'd imagine a bit of defensive programming would be enough here no? 🤔 But maybe there's a slightly separate thought/improvement there?