#2212 Incorrect calculation of placeholders, only the last placeholder should be allowed to match several segments #2213
Conversation
…ssue and adding unit and acceptance tests verifying the behavior.
|
@raman-m ready for review |
raman-m
added
hotfix
|
@raman-m could you please review this, so we can close it? |
| { | ||
| var groupName = match.Groups[1].Value; | ||
| index++; | ||
| return index == placeHoldersCount ? $"(?<{groupName}>[^&]*)" : $"(?<{groupName}>[^/|&]*)"; |
There was a problem hiding this comment.
So, this [^/|&] negative matching group filters out all slashes / after closing brace }, before catch all placeholder, right?
And, the last RX-match, for the last Catch-All, just accepts uncounted number of slashes before query string, right?
Query string processing is quite unclear... What's the final generated pattern?
| @@ -501,6 +501,30 @@ public void ShouldNotMatchComplexQueriesCaseSensitive(string downstream, string | |||
| .BDDfy(); | |||
| } | |||
|
|
|||
| [Theory] | |||
| [Trait("Bug", "2212")] | |||
| [InlineData("/data-registers/{version}/it/{everything}", "/dati-registri/{version}/{everything}", "/dati-registri/v1.0/operatore/R80QQ5J9600/valida", "/data-registers/v1.0/it/operatore/R80QQ5J9600/valida")] | |||
There was a problem hiding this comment.
Is this test data from #2212, right?
Convert anonimous delegate to local function.
| @@ -67,10 +67,12 @@ public Response<List<PlaceholderNameAndValue>> Find(string path, string query, s | |||
| private static List<Group> FindGroups(string path, string query, string template) | |||
| { | |||
| template = EscapeExceptBraces(template); | |||
| var regexPattern = $"^{RegexPlaceholders().Replace(template, match => $"(?<{match.Groups[1].Value}>[^&]*)")}"; | |||
| var regexPattern = GenerateRegexPattern(template); | |||
There was a problem hiding this comment.
Seems this is dynamic Regex, so the pattern is dynamically generated, but the previous version aka RegexPlaceholders() returns static object.
Well... is it possible to convert somehow to static object to improve the performance?
| return index == placeHoldersCount ? $"(?<{groupName}>[^&]*)" : $"(?<{groupName}>[^/|&]*)"; | ||
| } | ||
|
|
||
| return $@"^{RegexPlaceholders().Replace(escapedTemplate, MatchEvaluator)}"; |
There was a problem hiding this comment.
Is it possible to move this replacement to the static object? It appears not, due to the varying escapedTemplate that represents the current path template. This is unfortunate 😢 as it seems there are no options available.
|
I will release a patch after your pressing magic buttons here, and I needn't your approval 😝 |
|
It is that one may wish to press the magic buttons on incorrect PRs 😝 however, being offline means you don't wanna merge your own PRs 🤣 Happy Friday, Gui! 🥳 |
Yes, I think a few benchmarks are needed... |
Fixes #2212
Proposed Changes