Fix incorrect regex pattern in regex_replace_posix_groups#19827
Fix incorrect regex pattern in regex_replace_posix_groups#19827martin-g merged 10 commits intoapache:mainfrom
Conversation
The regex pattern was using (\d*) which matches zero or more digits,
causing a lone backslash to be incorrectly converted to ${}. Changed
to (\d+) which requires at least one digit, ensuring only actual POSIX
capture group references like \1, \2, etc. are converted to , .
Added unit tests to verify the correct behavior.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the regex_replace_posix_groups function where the regex pattern (\d*) incorrectly matched zero or more digits, causing a lone backslash \ to be transformed into ${}. The fix changes the pattern to (\d+) to require at least one digit.
Changes:
- Modified the regex pattern in
regex_replace_posix_groupsfrom(\d*)to(\d+)to match one or more digits - Added comprehensive unit tests to validate the fix and prevent regression
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
xanderbailey
left a comment
xanderbailey
left a comment
There was a problem hiding this comment.
Nice fix. Thanks for the PR. This looks correct to me but I don't have approval permissions!
The regex pattern was only matching single backslash (\1) but SQL string
literals with escaped backslashes pass through as double backslash (\\1).
Changed the pattern from (\\)(\d+) to \\{1,2}(\d+) to match 1 or 2
backslashes followed by digits, ensuring proper handling of SQL escaped
backreferences like 'X\\1Y'.
Added unit tests for the double backslash case.
|
Hi @GaneshPatil7517 Thank you for contribution. Please try to use the PR templates instead of the manual descriptions, it will allow the reviewers to easily understand the context. |
ok sir ill follow from you instruction.... |
|
FYI @shivbhatia10 -- is there any chance you would like to help review this PR too? |
shivbhatia10
left a comment
shivbhatia10
left a comment
There was a problem hiding this comment.
Looks good to me, thank you @GaneshPatil7517 !
|
Thank you so much to all for feedback, all checks are passed successfully passed Excited to see it merged... |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks @GaneshPatil7517 for contributing.
|
I think so, yes! |
The `regex_replace_posix_groups` method was using the pattern `(\d*)` to
match
POSIX capture group references like `\1`. However, `*` matches zero or
more
digits, which caused a lone backslash `\` to incorrectly become `${}`.
Changed to `(\d+)` which requires at least one digit, fixing the issue.
Added unit tests to validate correct behavior.
- Fixes apache#19766
---------
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
The `regex_replace_posix_groups` method was using the pattern `(\d*)` to
match
POSIX capture group references like `\1`. However, `*` matches zero or
more
digits, which caused a lone backslash `\` to incorrectly become `${}`.
Changed to `(\d+)` which requires at least one digit, fixing the issue.
Added unit tests to validate correct behavior.
- Fixes apache#19766
---------
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
The
regex_replace_posix_groupsmethod was using the pattern(\d*)to matchPOSIX capture group references like
\1. However,*matches zero or moredigits, which caused a lone backslash
\to incorrectly become${}.Changed to
(\d+)which requires at least one digit, fixing the issue.Added unit tests to validate correct behavior.