Cleaning up a bunch of todos

src/Microsoft.AspNetCore.Rewrite/CodeRewriteExtensions.cs

@@ -73,7 +73,6 @@ namespace Microsoft.AspNetCore.Rewrite
             return RedirectToHttps(options, statusCode, null);
         }
 
-        // TODO Don't do this, it doesn't work in all cases. Will refactor tonight/ tomorrow.
         public static RewriteOptions RedirectToHttps(this RewriteOptions options, int statusCode, int? sslPort)
         {
             options.Rules.Add(new RedirectToHttpsRule { StatusCode = statusCode, SSLPort = sslPort });

src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RewriteToHttpsRule.cs

@@ -7,7 +7,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules
 {
     public class RewriteToHttpsRule : Rule
     {
-
         public bool stopProcessing { get; set; }
         public int? SSLPort { get; set; }
 

src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/ModRewriteRedirectAction.cs

@@ -33,7 +33,7 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
             var pattern = Url.Evaluate(context, ruleMatch, condMatch);
             if (EscapeBackReferences)
             {
-                // TODO right way to escape backreferences?
+                // because escapebackreferences will be encapsulated by the pattern, just escape the pattern
                 pattern = Uri.EscapeDataString(pattern);
             }
             context.HttpContext.Response.StatusCode = StatusCode;
@@ -47,11 +47,13 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
                 QueryString query;
                 if (QueryStringAppend)
                 {
-                    query = context.HttpContext.Request.QueryString.Add(new QueryString(pattern.Substring(split)));
+                    query = context.HttpContext.Request.QueryString.Add(
+                        QueryString.FromUriComponent(
+                            pattern.Substring(split)));
                 }
                 else
                 {
-                    query = new QueryString(pattern.Substring(split));
+                    query = QueryString.FromUriComponent(pattern.Substring(split));
                 }
 
                 // not using the response.redirect here because status codes may be 301, 302, 307, 308 

src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/ModRewriteRewriteAction.cs

@@ -64,7 +64,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
             }
             else
             {
-                // TODO fix with redirect action logic
                 var split = pattern.IndexOf('?');
                 if (split >= 0)
                 {
@@ -77,7 +76,18 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
                     {
                         context.HttpContext.Request.Path = PathString.FromUriComponent(ForwardSlash + path);
                     }
-                    context.HttpContext.Request.QueryString = context.HttpContext.Request.QueryString.Add(new QueryString(pattern.Substring(split)));
+
+                    if (QueryStringAppend)
+                    {
+                        context.HttpContext.Request.QueryString = context.HttpContext.Request.QueryString.Add(
+                            QueryString.FromUriComponent(
+                                pattern.Substring(split)));
+                    }
+                    else
+                    {
+                        context.HttpContext.Request.QueryString = QueryString.FromUriComponent(
+                            pattern.Substring(split));
+                    }
                 }
                 else
                 {
@@ -89,6 +99,11 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
                     {
                         context.HttpContext.Request.Path = PathString.FromUriComponent(ForwardSlash + pattern);
                     }
+
+                    if (QueryStringDelete)
+                    {
+                        context.HttpContext.Request.QueryString = QueryString.Empty;
+                    }
                 }
             }
             return Result;

src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/RuleBuilder.cs

@@ -23,14 +23,13 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
         {
             if (_action == null || _match == null)
             {
-                // TODO throw an exception here, find apporpriate exception
+                throw new InvalidOperationException("Cannot create ModRewriteRule without action and match");
             }
             return new ModRewriteRule(_match, _conditions, _action, _preActions);
         }
 
         public void AddRule(string rule)
         {
-            // TODO 
             var tokens = new Tokenizer().Tokenize(rule);
             var regex = new RuleRegexParser().ParseRuleRegex(tokens[1]);
             var pattern = new TestStringParser().Parse(tokens[2]);
@@ -65,7 +64,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
 
             if (input.ConditionType == ConditionType.Regex)
             {
-                // TODO make nullable?
                 if (flags.HasFlag(FlagType.NoCase))
                 {
                     condition.Match = new RegexMatch(new Regex(input.Operand, RegexOptions.Compiled | RegexOptions.IgnoreCase, RegexTimeout), input.Invert);
@@ -147,7 +145,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
                     case OperationType.Executable:
                         throw new NotImplementedException("Executable Property search is not implemented");
                     default:
-                        // TODO change exception
                         throw new ArgumentException("Invalid operation for property comparison.");
                 }
             }

src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/TestStringParser.cs

@@ -64,12 +64,8 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
                         context.Next();
                         var ruleVariable = context.Capture();
                         context.Back();
-                        int parsedIndex;
+                        var parsedIndex = int.Parse(ruleVariable);
-                        if (!int.TryParse(ruleVariable, out parsedIndex))
+                        
-                        {
-                            // TODO this should always pass, remove try parse?
-                            throw new FormatException(Resources.FormatError_InputParserInvalidInteger(ruleVariable, context.Index));
-                        }
                         results.Add(new RuleMatchSegment(parsedIndex));
                     }
                     else
@@ -117,7 +113,7 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
                     else if (context.Current == Colon)
                     {
                         // Have a segmented look up Ex: HTTP:xxxx 
-                        // TODO 
+                        // Most of these we can't handle
                         throw new NotImplementedException("Segmented Lookups no implemented");
                     }
                 }
@@ -133,14 +129,11 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite
                 context.Mark();
                 context.Next();
                 var rawConditionParameter = context.Capture();
+
                 // Once we leave this method, the while loop will call next again. Because
                 // capture is exclusive, we need to go one past the end index, capture, and then go back.
                 context.Back();
-                int parsedIndex;
+                var parsedIndex = int.Parse(rawConditionParameter);
-                if (!int.TryParse(rawConditionParameter, out parsedIndex))
-                {
-                    throw new FormatException(Resources.FormatError_InputParserInvalidInteger(rawConditionParameter, context.Index));
-                }
                 results.Add(new ConditionMatchSegment(parsedIndex));
             }
             else

src/Microsoft.AspNetCore.Rewrite/Internal/ParserContext.cs

@@ -62,6 +62,7 @@ namespace Microsoft.AspNetCore.Rewrite.Internal
                 return null;
             }
         }
+
         public string Error()
         {
             return string.Format("Syntax Error at index: ", Index, " with character: ", Current); 

src/Microsoft.AspNetCore.Rewrite/Internal/PreActions/ChangeEnvironmentPreAction.cs

@@ -17,6 +17,7 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.PreActions
         public override void ApplyAction(HttpContext context, MatchResults ruleMatch, MatchResults condMatch)
         {
             // Do stuff to modify the env
+            throw new NotImplementedException();
         }
     }
 }

src/Microsoft.AspNetCore.Rewrite/Internal/UrlAction.cs

@@ -1,9 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using Microsoft.AspNetCore.Http;
-using Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite;
-
 namespace Microsoft.AspNetCore.Rewrite.Internal
 {
     public abstract class UrlAction

src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RedirectAction.cs

@@ -27,7 +27,9 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions
             var split = pattern.IndexOf('?');
             if (split >= 0)
             {
-                var query =  context.HttpContext.Request.QueryString.Add(new QueryString(pattern.Substring(split)));
+                var query =  context.HttpContext.Request.QueryString.Add(
+                    QueryString.FromUriComponent(
+                        pattern.Substring(split)));
                 // not using the HttpContext.Response.redirect here because status codes may be 301, 302, 307, 308 
                 context.HttpContext.Response.Headers[HeaderNames.Location] = pattern.Substring(0, split) + query;
             }

src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RedirectClearQueryAction.cs

@@ -1,7 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using Microsoft.AspNetCore.Http;
 using Microsoft.Net.Http.Headers;
 
 namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions

src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RewriteAction.cs

@@ -25,7 +25,7 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions
 
             if (ClearQuery)
             {
-                context.HttpContext.Request.QueryString = new QueryString();
+                context.HttpContext.Request.QueryString = QueryString.Empty;
             }
             // TODO PERF, substrings, object creation, etc.
             if (pattern.IndexOf("://") >= 0)
@@ -56,7 +56,9 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions
                     {
                         context.HttpContext.Request.Path = PathString.FromUriComponent(ForwardSlash + path);
                     }
-                    context.HttpContext.Request.QueryString = context.HttpContext.Request.QueryString.Add(new QueryString(pattern.Substring(split)));
+                    context.HttpContext.Request.QueryString = context.HttpContext.Request.QueryString.Add(
+                        QueryString.FromUriComponent(
+                            pattern.Substring(split)));
                 }
                 else
                 {

src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/VoidAction.cs

@@ -1,8 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using Microsoft.AspNetCore.Http;
-
 namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions
 {
     public class VoidAction : UrlAction

src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/InputParser.cs

@@ -48,9 +48,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite
                 }
                 else if (context.Current == CloseBrace)
                 {
-                    // TODO we should be throwing a syntax error if we have uneven close braces
-                    // Can fix by keeping track of the number of '{' and '}' with an int, where {
-                    // increments and } decrements. Throw if < 0.
                     return new Pattern(results);
                 }
                 else
@@ -71,7 +68,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite
             // 2. {R:1}  - Rule parameter
             // 3. {C:1}  - Condition Parameter
             // 4. {function:xxx} - String function 
-            // TODO consider perf here. This is on startup and will only happen one time
             // (unless we support Reload)
             string parameter;
             while (context.Next())

src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteRuleBuilder.cs

@@ -24,7 +24,10 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite
 
         public UrlRewriteRule Build()
         {
-            // TODO some of these are required fields, throw if null?
+            if (_initialMatch == null || _action == null)
+            {
+                throw new InvalidOperationException("Cannot create UrlRewriteRule without action and match");
+            }
             var rule = new UrlRewriteRule();
             rule.Action = _action;
             rule.Conditions = _conditions;
@@ -61,10 +64,10 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite
                     }
                     break;
                 case ActionType.AbortRequest:
-                    throw new FormatException("Abort Requests are not supported");
+                    throw new NotImplementedException("Abort Requests are not supported");
                 case ActionType.CustomResponse:
                     // TODO
-                    throw new FormatException("Custom Responses are not supported");
+                    throw new NotImplementedException("Custom Responses are not supported");
             }
         }
 
@@ -94,7 +97,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite
             }
         }
 
-        // TODO make this take two overloads and handle regex vs non regex case.
         public void AddUrlCondition(Pattern input, string pattern, PatternSyntax patternSyntax, MatchType matchType, bool ignoreCase, bool negate)
         {
             // If there are no conditions specified, 
@@ -140,7 +142,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite
                                     break;
                                 }
                             default:
-                                // TODO new exception handling
                                 throw new FormatException("Unrecognized matchType");
                         }
                         break;

src/Microsoft.AspNetCore.Rewrite/RewriteMiddleware.cs

@@ -15,6 +15,8 @@ namespace Microsoft.AspNetCore.Rewrite
     /// </summary>
     public class RewriteMiddleware
     {
+        private static readonly Task CompletedTask = Task.FromResult(0);
+
         private readonly RequestDelegate _next;
         private readonly RewriteOptions _options;
         private readonly IFileProvider _fileProvider;
@@ -64,8 +66,7 @@ namespace Microsoft.AspNetCore.Rewrite
                         // Explicitly show that we continue executing rules
                         break;
                     case RuleTerminiation.ResponseComplete:
-                        // TODO cache task for perf
+                        return CompletedTask;
-                        return Task.FromResult(0);
                     case RuleTerminiation.StopRules:
                         return _next(context);
                 }

test/Microsoft.AspNetCore.Rewrite.Tests/ModRewrite/RewriteTokenizerTest.cs

@@ -26,7 +26,6 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.ModRewrite
         [Fact]
         public void Tokenize_CheckEscapedSpaceIgnored()
         {
-            // TODO need consultation on escape characters.
             var testString = @"RewriteCond %{HTTPS}\ what !-f";
             var tokens = new Tokenizer().Tokenize(testString);
 

test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/FileParserTests.cs

@@ -171,7 +171,6 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.UrlRewrite
                 Assert.Equal(r1.Name, r2.Name);
                 Assert.Equal(r1.Enabled, r2.Enabled);
 
-                // TODO conditions, url pattern, initial match regex
                 if (r1.Conditions == null)
                 {
                     Assert.Equal(r2.Conditions.ConditionList.Count, 0);

test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/FormatExceptionHandlingTests.cs

@@ -30,26 +30,6 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.UrlRewrite
 </rewrite>", 
             "Could not parse the UrlRewrite file. Message: 'Missing close brace for parameter at string index: '1''. Line number '5': '14'.")]
         [InlineData(
-@"<rewrite>
-    <rules>
-        <rule name=""Rewrite to article.aspx"">
-            <match url = ""(.*)"" />
-            <action type=""AbortRequest"" url ="""" />
-        </rule>
-    </rules>
-</rewrite>", 
-            "Could not parse the UrlRewrite file. Message: 'Abort Requests are not supported'. Line number '5': '14'.")]
-        [InlineData(
-@"<rewrite>
-    <rules>
-        <rule name=""Rewrite to article.aspx"">
-            <match url = ""(.*)"" />
-            <action type=""CustomResponse"" url ="""" />
-        </rule>
-    </rules>
-</rewrite>", 
-            "Could not parse the UrlRewrite file. Message: 'Custom Responses are not supported'. Line number '5': '14'.")]
-        [InlineData(
 @"<rewrite>
     <rules>
         <rule name=""Rewrite to article.aspx"">

test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/InputParserTests.cs

@@ -19,8 +19,7 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.UrlRewrite
             var result = new InputParser().ParseInputString(testString);
             Assert.Equal(result.PatternSegments.Count, 1);
         }
-        
+
-        // TODO update tests to check type
         [Theory]
         [InlineData("foo/bar/{R:1}/what", 3)]
         [InlineData("foo/{R:1}", 2)]
@@ -93,7 +92,6 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.UrlRewrite
         {
 
             var context = new DefaultHttpContext();
-            // TODO add fields if necessary
             return new RewriteContext { HttpContext = context, FileProvider = null };
         }
 

test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/ServerVariableTests.cs

@@ -37,8 +37,8 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.UrlRewrite
         {
             var context = new DefaultHttpContext();
             context.Request.Host = new HostString("example.com");
-            context.Request.Path = new PathString("/foo");
+            context.Request.Path = PathString.FromUriComponent("/foo");
-            context.Request.QueryString = new QueryString("?bar=1");
+            context.Request.QueryString = QueryString.FromUriComponent("?bar=1");
             context.Request.ContentLength = 10;
             context.Request.ContentType = "json";
             context.Request.Headers[HeaderNames.Accept] = "accept";