-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#2209 Placeholders matching must work if RouteIsCaseSensitive = false #2210
Conversation
@raman-m ready for review |
f2e659e
to
aaa8dbd
Compare
// Here we are not interested in the path itself, only the placeholders. | ||
// Path validation is not part of this class, therefore allowing case-insensitive | ||
// matching. | ||
return $"^(?i){escaped}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matching must work if RouteIsCaseSensitive = false
But the option is not applied here, right?
I am trying to understand how we've lost old functionality of insensitive placeholder names...
After research I found only this code which confirms that in old version names were case insensitive:
Ocelot/src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs
Lines 149 to 152 in d76fc95
private static bool CharactersDontMatch(char characterOne, char characterTwo) | |
{ | |
return char.ToLower(characterOne) != char.ToLower(characterTwo); | |
} |
That means placeholder names are always case insensitive.
Not an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raman-m What we have lost: /blabla/bliBLI/{PlaceHolder}
wil not match /blabla/blibli/{PlaceHolder}
because the elements after the host name could be case sensitive, and stricly taken /blabla/bliBLI
can't match /blabla/blibli
. But, yes, the behavior with ^(?i)
is the same as the code snippet you provided.
{ | ||
var port = PortFinder.GetRandomPort(); | ||
var route = GivenRoute(port, upstream, downstream); | ||
route.RouteIsCaseSensitive = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... Again, I'm trying to understand entire logic.
We have only 2 places where the property is applied:
1
Ocelot/src/Ocelot/Configuration/Creator/UpstreamHeaderTemplatePatternCreator.cs
Lines 42 to 44 in 7a5046f
var template = route.RouteIsCaseSensitive | |
? $"^{headerTemplateValue}$" | |
: $"^(?i){headerTemplateValue}$"; // ignore case |
2
var template = route.RouteIsCaseSensitive | |
? $"^{upstreamTemplate}{RegExMatchEndString}" | |
: $"^{RegExIgnoreCase}{upstreamTemplate}{RegExMatchEndString}"; |
This 2nd code snippet seems the right place where we apply case sensitivity.
Not an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raman-m And we don't pass the pattern to the placeholder finder, but the original one, without the regex chars.
@ggnaegi Regarding the merged commit fe029eb, there has been an override of the name/title of the PR. The significant issue is the lack of reference to the fixed bug. In the future, please maintain the original commit name while prefixing it with the reference to the issue: - Fixing issue 2209, adding extensive set of unit and acceptance tests (#2210)
+ #2209 Placeholders matching must work if RouteIsCaseSensitive = false (#2210) As an Administrator, I will now proceed to rectify the commit name... |
New commit in develop is d41f8fa ❕ |
Fixes #2209
Proposed Changes