Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#31069 closed Bug (fixed)

Change in behavior for optional re matches.

Reported by: Terence Honles Owned by: Hasan Ramezani
Component: Documentation Version: 3.0
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 Terence Honles)

I am mainly pointing this out as it probably should have been called out as a breaking change from Django 2.X to 3.X (And I think the release notes should probably be adjusted if the old functionality is not to be restored). You can probably argue either way about if this is a bug since it's coming from re's match.groupdict(). It's related to https://code.djangoproject.com/ticket/31061 and caused by https://code.djangoproject.com/ticket/26431 / https://github.com/django/django/pull/11477

The test is the following:

from django.urls import re_path

def view(request, optional1, optional2): ...

urlpatterns = [
  re_path(r'^(?P<optional1>\w+/)(?:(?P<optional2>\w+)/)?', view)
]

For Django 2.X this would work as expected because named captures are returned even if they are None, however in Django 3.0 this would raise an error because positional parameter optional2 is no longer provided. (i.e. view() missing 1 required positional argument: 'optional2')

You can argue that optional2 should actually have had a default all along, but it didn't before and code may unexpectedly break because it is now required.

I'm not very familiar with https://code.djangoproject.com/ticket/26431, but I think if possible it probably should have been fixed at a different level.

Change History (10)

comment:1 by Mariusz Felisiak, 4 years ago

Component: Core (URLs)Documentation
Severity: NormalRelease blocker
Summary: Change in behavior for optional re matchesChange in behavior for optional re matches.
Triage Stage: UnreviewedAccepted

I think that the current behavior is correct, sorry for inconvenience. We should clarified this in documentation, add release notes and a versionchanged annotation.

comment:2 by Baptiste Mispelon, 4 years ago

I can't reproduce the view() missing 1 required positional argument: 'optional2' error with the URLconf you've provided. There aren't any optional capture groups in your regexp so I don't see how groupdict() would return None values.

Do you have an example URL that triggers the error?

Thanks.

comment:3 by jishansingh, 4 years ago

error is caused by missing ? at end of re_path

from django.urls import re_path

def view(request, optional1, optional2): ...

urlpatterns = [
  re_path(r'^(?P<optional1>\w+/)(?:(?P<optional2>\w+)/)?', view)
]

works

Last edited 4 years ago by jishansingh (previous) (diff)

comment:4 by Mariusz Felisiak, 4 years ago

Severity: Release blockerNormal

Issue that Terence described is that we no longer pass None for named groups that are not provided. The following example

def modules(request, format):
    return render(request, 'home.html')

urlpatterns = [
    path('admin/', admin.site.urls),
    re_path(r'^module1/(?P<format>(html|json|xml))?/?$', modules, name='modules1'),
]

works in Django 2.2 when we call http://localhost:8000/module1/ but throws an error in Django 3.0:

    response = wrapped_callback(request, *callback_args, **callback_kwargs)
TypeError: modules() missing 1 required positional argument: 'format'

IMO this behavior is correct, see comment.

I'm decreasing a severity because it's a documentation issue and can be backported at anytime.

comment:5 by Terence Honles, 4 years ago

Description: modified (diff)

comment:6 by Terence Honles, 4 years ago

Thanks @jishansingh, yes, I forgot the last ? when typing the example into the report (edited).

@felixxm it would be best if this was fixed (documented) sooner rather than later so people migrating would know they need to fix it. We didn't have appropriate test coverage, but one of my coworkers noticed it by chance (not a frequently used route), and checked the rest of our application to make sure there weren't any offenders.

I don't know where to put any documentation, but I'm willing to help call it out if I'm pointed in the right direction (I may not have time next week so if someone else can get to it that would be great, otherwise I'll see what I can do).

Edit: I'm guessing it's here: https://github.com/django/django/blob/6410d38ca7255961c08f1d657fdf920555d113f3/docs/topics/http/urls.txt#L58-L61

Last edited 4 years ago by Terence Honles (previous) (diff)

comment:7 by Mariusz Felisiak, 4 years ago

Yes, that's the right place to document this change.

comment:8 by Hasan Ramezani, 4 years ago

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: newassigned
Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 6cb30414:

[3.0.x] Fixed #31069, Refs #26431 -- Doc'd RegexPattern behavior change in passing optional named groups in Django 3.0.

Backport of 9736137cdc4b7528a0aca17415dc9798660eed81 from master

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 9736137:

Fixed #31069, Refs #26431 -- Doc'd RegexPattern behavior change in passing optional named groups in Django 3.0.

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