Opened 7 years ago

Closed 7 years ago

#28703 closed Cleanup/optimization (wontfix)

URL regex for admin's app_index view isn't constructed deterministically

Reported by: Erik Bryant Owned by: nobody
Component: contrib.admin Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

https://github.com/django/django/blob/master/django/contrib/admin/sites.py#L277

The regex pattern that is constructed for an endpoint is not stable. For instance, in my project I have seen it construct each of the following:

^admin/^(?P<app_label>social_django|authtoken|queues|jobs|files|users|clusters|appps|auth|vdcs)/$
^admin/^(?P<app_label>social_django|auth|users|vdcs|appps|jobs|queues|authtoken|clusters|files)/$
^admin/^(?P<app_label>authtoken|appps|social_django|clusters|users|queues|jobs|vdcs|auth|files)/$
^admin/^(?P<app_label>authtoken|appps|social_django|clusters|users|queues|vdcs|jobs|auth|files)/$

This precludes the use of patterns in unit tests.

https://github.com/django/django/pull/9230

Change History (8)

comment:1 by Tim Graham, 7 years ago

Description: modified (diff)
Summary: Endpoint regex pattern construction is not stableURL regex for admin's app_index view isn't constructed deterministically
Type: UncategorizedCleanup/optimization

It seems to me that such a URL introspection test in your project is testing an internal implementation in Django that could be subject to change in the future. Can you give an idea of what regressions you're looking to test for? I wonder if a test that inspects _registry (or perhaps better yet, uses the site.is_registered() method) would be more appropriate.

in reply to:  1 comment:2 by Erik Bryant, 7 years ago

I was asked to create a mechanism that would tell us if any new endpoints were added or if any existing endpoints were deleted. In either case we would need to update the web UI that uses our endpoints.

My less-than-ideal solution was to create a snapshot of our existing endpoints in a file. I wrote a test that compares the current set of endpoints with the snapshot. If an endpoint is added/deleted the test will fail. The expectation is that the developer who is making the change would see the failure and create a ticket to have the web UI updated. I'd be happy to know how other teams deal with this. I'm sure there is a better way.

comment:3 by Tim Graham, 7 years ago

I don't entirely understand your setup but I wonder if you're concerned with changes to admin URLs and this URL in particular? Admin URLs may change when upgrading Django when new features are added or removed. For example, Django 2.0 adds an "autocomplete view". I wonder if your mechanism could ignore admin/ URLs entirely?

comment:4 by Erik Bryant, 7 years ago

That's a good point. We don't plan to test the admin/ URLs. I'll remove that one from the list we scrape.

As for this ticket, should we go ahead with the fix?

comment:5 by Tim Graham, 7 years ago

I'm not sure if there's a compelling reason to make the proposed change but I guess the overhead isn't too large.

comment:6 by Erik Bryant, 7 years ago

I've been thinking of changing this from O(N-squared) to O(N) by using a set instead of a list. It would make the code just a little simpler. How about I do that to help offset the extra sorted() call?

https://github.com/erikbryant/django/blob/master/django/contrib/admin/sites.py#L271

comment:7 by Tim Graham, 7 years ago

Keywords: stable removed
Triage Stage: UnreviewedAccepted

I think that change could be made separately without a ticket.

comment:8 by Erik Bryant, 7 years ago

Resolution: wontfix
Status: newclosed

We have an alternate approach, so we don't require this change. Since there is no clear other need for it, closing it as won't fix.

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