aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

IISUrlRewriteBuilder and RegexOptions.ECMAScript #147

Closed natemcmaster closed 8 years ago

natemcmaster commented 8 years ago

URL Rewrite uses the ECMAScript spec to parse regular expressions (see https://www.iis.net/learn/extensions/url-rewrite-module/url-rewrite-module-20-configuration-reference). Currently the IISUrlRewriteBuilder does not use RegexOptions.ECMAScript which means the regex pattern matching uses canonical instead of ECMAScript matching.

The difference may cause subtle differences in how IIS would have matched a pattern vs how the rewrite middleware does (such as what matches the \w character group) or self-referencing capture groups.

muratg commented 8 years ago

@mikaelm12 Could we check what Apache modrewrite uses for regex syntax? We should make sure we use the right regex syntax for that as well.

mikaelm12 commented 8 years ago

From the mod_rewrite doc

mod_rewrite uses the Perl Compatible Regular Expression vocabulary.

From the url rewrite doc

Perl compatible (ECMAScript standard compliant) regular expression syntax

@natemcmaster Also something to note is that the RegexOptions.ECMAScript flag cannot be used with the RegexOptions.CultureInvariant flag which is currently being set as well.

mikaelm12 commented 8 years ago

I was having trouble with making this change but it turns out that there is an inconsistency in the docs It turns out that you can only use the RegexOptions.ECMAScript flag with the IgnoreCase and Multiline flags

These RegexOption docs say about RegexOptions.ECMAScript

This value can be used only in conjunction with the IgnoreCase, Multiline, and Compiled values.

But these docs say

The RegexOptions.ECMAScript option can be combined only with the RegexOptions.IgnoreCase and RegexOptions.Multiline options

which is correct. So we also have to removed the compiled flag as well.

muratg commented 8 years ago

@mikaelm12 Would removing the compiled flag be a perf concern?

mikaelm12 commented 8 years ago

@muratg Yes it is a perf concern. If we wan't to use the ECMAScript option we have to remove the compiled flag though.

natemcmaster commented 8 years ago

IIRC you can still use .Compiled on desktop .NET. We should measure perf impact to see if it actually matters.

mikaelm12 commented 8 years ago

So I did some basic perf testing and with large numbers of iterations ( > 10 Million) the absence of the compiled flag caused an about 13% hit to performance. Less than 1 Million iterations had less than a 5% effect.

Thoughts @muratg @Tratcher

Tratcher commented 8 years ago

Correctness trumps perf. How well do you understand the functional differences of ECMAScript?

mikaelm12 commented 8 years ago

Not well tbh.

muratg commented 8 years ago

https://msdn.microsoft.com/en-us/library/yd1hzczs(v=vs.110).aspx#ECMAScript

mikaelm12 commented 8 years ago

Damian says don't use the ECMAScript flag.

muratg commented 8 years ago

Closing per @DamianEdwards's decision.