Fixed #26431 -- Prevented django.urls.resolve() from returning missing optional parameters.

[MisterRios]

…amed groups are missing in the URL pattern

Commit Message too long?

I had patched with the original patch provided by the reporter, but then dug a bit deeper. I then patched at
_reverse_with_prefix which is where most of my previous work was situated. (Delete kwargs with None values that are valid parameters- using set magic). But the tests for named_optional and named_optional_terminated with just one parameter still went through, so I dug deeper.

Usually these None-value parameters never make it to _reverse_with_prefix. The culprit, in the end, was django.urls.resolvers.RegexPattern.match. It user groupdict() which returns a None as a default value instead of nixing the parameter: https://docs.python.org/3.6/library/re.html#re.match.groupdict

https://code.djangoproject.com/ticket/26431

[felixxm]

@MisterRios Thanks for this patch 👍 Tests in i18n also pass if I remove empty kwargs in _reverse_with_prefix(), e.g.

kwargs = {key: value for key, value in kwargs.items() if value is not None}

Can you add extra tests to show that fix should be made in RegexPattern?

Moreover tests added to the urlpatterns_reverse pass without this fix that's why I moved them to a separate commit.

[MisterRios]

@MisterRios Thanks for this patch +1 Tests in i18n also pass if I remove empty kwargs in _reverse_with_prefix(), e.g.

kwargs = {key: value for key, value in kwargs.items() if value is not None}

@felixxm Yes! I came up against this, but urlpatterns_reverse.tests.ReverseShortcutTests.test_redirect_view_object fails because the kwarg wrong_argument=None doesn't get stripped out, and doesn't raise a NoReverseMatch Error.

My first approach (which might be preferable) was to strip out kwargs set as None which are valid url kwargs:

# Remove optional kwargs if a value is not declared
none_kwargs = {key for key, value in kwargs.items() if value is None}
available_kwargs = {kwarg for result, params in possibility for kwarg in params}
to_delete_kwargs = available_kwargs.intersection(none_kwargs)
kwargs = {key: value for key, value in kwargs.items() if key not in to_delete_kwargs}

But I noticed that tests for non-provided optional arguments already passed without this patch, Those arguments never come to this stage.

Can you add extra tests to show that fix should be made in RegexPattern?

Will do.

Moreover tests added to the urlpatterns_reverse pass without this fix that's why I moved them to a separate commit.
Thanks.

[felixxm]

@MisterRios Thanks for explanation 👍 I think that current approach is good and I'm waiting for extra tests.

[MisterRios]

@felixxm I wasn't sure exactly where to add them, but ended up with two tests that check the matches. The one test_re_path_with_optional_parameter_not_provided fails without the patch.

[felixxm]

@MisterRios Thanks! Welcome aboard 🚀

[MisterRios]

@felixxm Thanks! Hopefully the first of many!

[nikolaysm]

Will an update be provided for django 1.11 to fix the mentioned error?

[felixxm]

@nikolaysm No, Django 1.11 receives only security fixes.

[nikolaysm]

@nikolaysm No, Django 1.11 receives only security fixes.

@felixxm Ok, thanks for your reply.