[Django] #28703: Endpoint regex pattern construction is not stable
Django
-----------------------------------------+------------------------
Reporter: erikbryant | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 1.11
Severity: Normal | Keywords: stable
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/28703>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: erikbryant | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Keywords: stable | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
Old description:
> 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
New description:
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
--
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Keywords: stable | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by erikbryant):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Keywords: stable | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Erik Bryant | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: stable | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Erik Bryant):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Erik Bryant | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: stable | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I'm not sure if there's a compelling reason to make the proposed change
but I guess the overhead isn't too large.
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Erik Bryant | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: stable | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Erik Bryant):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:6>
Django
Reporter: Erik Bryant | Owner: nobody
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* keywords: stable =>
* stage: Unreviewed => Accepted
Comment:
I think that change could be made separately without a ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: Erik Bryant | Owner: nobody
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Erik Bryant):
* status: new => closed
* resolution: => wontfix
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28703#comment:8>