Fixed #37240 -- Fixed simplify_regex() with multiple unnamed groups. - #21698
Fixed #37240 -- Fixed simplify_regex() with multiple unnamed groups.#21698SEPURI-SAI-KRISHNA wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
|
Thank you for your contribution to Django! This pull request has one or more items that need attention before it can be accepted for review. ❗ Error: Trac Ticket Not Ready for a Pull RequestThe referenced ticket ticket-37240 is not ready for a pull request. A ticket must be in the Accepted or Ready for checkin stage, assigned status, and have no resolution. Current state: stage='Unreviewed' What to do:
For more information on the Django triage process see https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/. If you have questions about these requirements, please review the contributing guidelines or ask for help on the Django Forum. |
|
Superseded by #21700. This PR was auto-closed before review and could not be reopened after the commit was amended. |
sarahboyce
left a comment
There was a problem hiding this comment.
This looks really good - thank you for the ticket and PR!
Can you remove the release note and make sure your commit message ends with a full stop and then reopen please? ⭐
Trac ticket number
ticket-37240
Branch description
simplify_regex()re-emits the whole pattern prefix once per capture group, so anyre_path()containing two or more unnamed capture groups was rendered with a duplicated, malformed path.The corruption compounds with each additional group — three unnamed groups produce
/a/<var>/b/a/(\w)/b/<var>/c/a/(\w)/b/(\d)/c/<var>.Both callers are affected: the admindocs "Views" page (
ViewIndexView.get_context_data()passessimplify_regex(regex)straight into theurlcontext value) and thelisturlsmanagement command added in 6.2.Cause. In
replace_unnamed_groups(), the first iteration correctly emitspattern[:start]becauseprev_endisNone. Every later iteration appends the text between the previous group and this one (correct) and then appendspattern[:start]again, duplicating the entire prefix from index 0:The sibling
remove_non_capturing_groups()in the same module already has the correct shape, relying onpattern[None:start] == pattern[:start]for the first iteration. This change collapses the three lines into the same idiom:Why it went unnoticed.
SimplifyRegexTests.test_simplify_regexhas over 100 cases, but every one of them contains at most one unnamed group, so the second-iteration path was never executed. This patch adds six cases covering two and three unnamed groups, unnamed groups mixed with named ones, nested alternations, and escaped parentheses. All six fail before the fix.Long-standing, not a regression. Verified by running the reproduction against released wheels:
^articles/(\w+)/comments/(\d+)/$^articles/<var>/comments/^articles/(\w+)/comments/<var>/$/articles/<var>/comments/articles/(\w)/comments/<var>//articles/<var>/comments/articles/(\w)/comments/<var>//articles/<var>/comments/articles/(\w)/comments/<var>//articles/<var>/comments/articles/(\w)/comments/<var>/The 2.2 result confirms the defect predates both the move of these helpers to
django/urls/utils.pyin 6.2 and the changes made for #30731. A release note is included indocs/releases/6.0.8.txtsince the fix qualifies for backport.AI Assistance Disclosure (REQUIRED)
Claude Code (Claude Opus 5) was used to locate the bug, write the fix and the regression tests, and run the verification against released versions. I have reviewed and verified all of it myself.
Checklist
This PR follows the contribution guidelines.
This PR does not disclose a security vulnerability (see vulnerability reporting).
This PR targets the
mainbranch.The commit message is written in past tense, mentions the ticket number (if applicable), and ends with a period (see guidelines).
I have not requested, and will not request, an automated AI review for this PR.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.