#55 add IIS rewrite map support with tests #168

david-peden-q2 · 2016-11-04T19:43:23Z

No description provided.

dnfclas · 2016-11-04T19:43:27Z

Hi @david-peden-q2, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, DNFBOT;

jkotalik

Overall looks pretty solid, but there are some general issues we need to verify before we get this in.

jkotalik · 2016-11-04T23:34:49Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/IISRewriteMap.cs

+		{
+			if (string.IsNullOrEmpty(key))
+			{
+				throw new ArgumentException($"{nameof(key)} cannot be empty or null", nameof(key));


Sufficient to just throw new ArgException(nameof(key))

jkotalik · 2016-11-04T23:34:58Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/IISRewriteMap.cs

+		{
+			if (string.IsNullOrEmpty(name))
+			{
+				throw new ArgumentException("name cannot be empty or null", nameof(name));


Sufficient to just throw new ArgException(nameof(key))

jkotalik · 2016-11-04T23:35:12Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/IISRewriteMap.cs

+			}
+			if (string.IsNullOrEmpty(value))
+			{
+				throw new ArgumentException($"{nameof(value)} cannot be empty or null", nameof(value));


jkotalik · 2016-11-04T23:39:21Z

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

        private const char CloseBrace = '}';
+        private readonly IDictionary<string, IISRewriteMap> _rewriteMaps;
+
+        public InputParser(IDictionary<string, IISRewriteMap> rewriteMaps = null)


Prefer this not to be a default parameter.

what do you prefer? two constructors? if so, why?

I think it was more of a style thing ASP.NET uses in general

so replace with an explicit default constructor and non-null one?

jkotalik · 2016-11-04T23:40:27Z

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

+                            IISRewriteMap rewriteMap;
+                            if (_rewriteMaps != null && _rewriteMaps.TryGetValue(parameter, out rewriteMap))
+                            {
+                                Pattern pattern = ParseString(context);


We use var for all local parameters if possible :)

not great for readability but ok

jkotalik · 2016-11-04T23:44:57Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/RewriteMapParser.cs

+            var rewriteMaps = new Dictionary<string, IISRewriteMap>();
+            foreach (XElement mapElement in mapsElement.Elements(RewriteTags.RewriteMap))
+            {
+                var map = new IISRewriteMap(mapElement.Attribute(RewriteTags.Name)?.Value);


Don't think this should be nullable; if the map doesn't have a name, we should probably throw.

jkotalik · 2016-11-04T23:45:44Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/RewriteMapParser.cs

+                {
+                    map.AddOrUpdateEntry(addElement.Attribute(RewriteTags.Key)?.Value, addElement.Attribute(RewriteTags.Value)?.Value);
+                }
+                rewriteMaps.Add(map.Name, map);


What if we have two rewrite maps of the same name? We should decide whether we throw or overwrite an entry (IMO throwing is better).

Dictionary.Add does throw. are you suggesting we throw an explicit exception?

jkotalik · 2016-11-04T23:48:17Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/UrlRewriteFileParser.cs

            if (xmlRoot != null)
            {
+                var rewriteMapParser = new RewriteMapParser();
+                _inputParser = new InputParser(rewriteMapParser.Parse(xmlRoot));


jkotalik · 2016-11-04T23:49:49Z

src/Microsoft.AspNetCore.Rewrite/Internal/PatternSegments/RewriteMapSegment.cs

+            {
+                return value;
+            }
+            var exception = new Exception($"Rewrite map entry not found: '{key}'");


This style seems a bit odd. I'd refer to how we normally do exception handling with logging errors

jkotalik · 2016-11-04T23:55:39Z

src/Microsoft.AspNetCore.Rewrite/Internal/PatternSegments/RewriteMapSegment.cs

+        {
+            string key = _pattern.Evaluate(context, ruleMatch, condMatch);
+            string value;
+            if (_rewriteMap.TryGetEntry(key, out value))


I think this is wrong. If we don't find an entry in our map, we shouldn't be throwing, it just means it wasn't found in our server variables. Ex I have a url: /foo/bar which maps to foo?bar=1. If our url doesn't equal /foo/bar, it's not an invalid url, it just means it wasn't found in our map.

jkotalik · 2016-12-05T21:07:26Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/RewriteMapParser.cs

+    {
+        public IDictionary<string, IISRewriteMap> Parse(XElement xmlRoot)
+        {
+            var mapsElement = xmlRoot.Descendants(RewriteTags.RewriteMaps).SingleOrDefault();


Check here if xmlRoot is null (or nullable)

there is already a null check at the call site. should the check be in both places? i'll go ahead and add it but just wanted to point that out.

jkotalik · 2016-12-05T21:08:27Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/RewriteMapParser.cs

+
+namespace Microsoft.AspNetCore.Rewrite.Internal.IISUrlRewrite
+{
+    public class RewriteMapParser


You probably want this to be a static helper rather than an object?

jkotalik · 2016-12-05T21:14:13Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/UrlRewriteFileParser.cs


            if (xmlRoot != null)
            {
+                var rewriteMapParser = new RewriteMapParser();


Again can probably make this static.

Tratcher · 2016-12-12T19:44:32Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/IISRewriteMap.cs

+            Name = name;
+        }
+
+        public void AddOrUpdateEntry(string key, string value)


Tratcher · 2016-12-12T19:45:02Z

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

        private const char CloseBrace = '}';
+        private readonly IDictionary<string, IISRewriteMap> _rewriteMaps;
+
+        public InputParser(IDictionary<string, IISRewriteMap> rewriteMaps = null)


Tratcher · 2016-12-12T19:57:23Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/IISRewriteMap.cs

+
+        public bool TryGetEntry(string key, out string value)
+        {
+            return _map.TryGetValue(key, out value);


It looks like the only caller of this method does _rewriteMap.TryGetEntry(key, out value) ? value : null, so why not build that in and just do a simple GetEntry or an indexer?

Tratcher · 2017-01-20T21:13:04Z

src/Microsoft.AspNetCore.Rewrite/IISUrlRewriteOptionsExtensions.cs

+		/// <param name="reader">The text reader stream.</param>
+		/// <param name="rewriteMaps">An optional set of rewrite maps to uztilize when parsing rules.
+		/// Rewrite maps in this collection will overwrite (supercede) duplicate named entries in the XML.</param>
+		public static RewriteOptions AddIISUrlRewrite(this RewriteOptions options, TextReader reader, IEnumerable<IISRewriteMap> rewriteMaps = null)


make a second method rather than using a default parameter

If RewriteMaps are part of the xml file, why pass them in here?

For scenarios where there are other sources of rewrite maps. E.g., we have static rules (configured in xml) but have dynamically constructed rewrite maps (from an api/database call). This allows for composing rewrite maps outside of those statically defined in the file.

Is that a supported IIS scenario? We're not trying to add functionality beyond IIS at this point.

i don't know but i doubt it. it's a minor addition and something i can account for by purely using the programmatic api. i won't lose sleep over cutting it but it is useful.

cut it for now

mikaelm12

This will also need to be rebased on dev

mikaelm12 · 2017-01-20T22:58:11Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/RewriteMapParser.cs

@@ -0,0 +1,36 @@
+using System;


Copyright and license header needed here

mikaelm12 · 2017-01-20T22:59:05Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/RewriteMapParser.cs

+        {
+            if (xmlRoot == null)
+            {
+                throw new ArgumentException(nameof(xmlRoot));


Should be an argument null exception

mikaelm12 · 2017-01-20T23:01:21Z

src/Microsoft.AspNetCore.Rewrite/Internal/PatternSegments/RewriteMapSegment.cs

@@ -0,0 +1,22 @@
+using Microsoft.AspNetCore.Rewrite.Internal.IISUrlRewrite;


Copyright and license header needed here

mikaelm12 · 2017-01-20T23:02:21Z

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

 using Microsoft.AspNetCore.Rewrite.Internal.IISUrlRewrite;
+using Microsoft.AspNetCore.Rewrite.Internal.PatternSegments;
+using Microsoft.Extensions.Logging.Testing;
+


Remove this new line

mikaelm12 · 2017-01-20T23:03:05Z

test/Microsoft.AspNetCore.Rewrite.Tests/IISUrlRewrite/RewriteMapParserTests.cs

+using System.IO;
+using System.Linq;
+using System.Xml.Linq;
+


Remove new line

mikaelm12 · 2017-01-20T23:03:23Z

test/Microsoft.AspNetCore.Rewrite.Tests/IISUrlRewrite/RewriteMapParserTests.cs

@@ -0,0 +1,43 @@
+using System.Collections.Generic;


Copyright and license header

mikaelm12 · 2017-01-20T23:05:38Z

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

+            var maps = new IISRewriteMapCollection { map };
+
+            string inputString = $"{{{expectedMapName}:{{R:1}}}}";
+            Pattern pattern = new InputParser(maps).ParseInputString(inputString);


mikaelm12 · 2017-01-20T23:05:48Z

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

+            Pattern pattern = new InputParser(maps).ParseInputString(inputString);
+            Assert.Equal(1, pattern.PatternSegments.Count);

+            PatternSegment segment = pattern.PatternSegments.Single();


mikaelm12 · 2017-01-20T23:18:05Z

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

                                return;
                            }
                        default:
+                            IISRewriteMap rewriteMap = _rewriteMaps?[parameter];


mikaelm12 · 2017-01-20T23:20:16Z

src/Microsoft.AspNetCore.Rewrite/Internal/IISUrlRewrite/RewriteMapParser.cs

+            var mapsElement = xmlRoot.Descendants(RewriteTags.RewriteMaps).SingleOrDefault();
+            if (mapsElement == null)
+            {
+                return null;


Hmm. Should we throw here?

I don't think so. It's valid not to have a rewrite map. Am I missing your point?

That's what I was talking about. It just seems like and odd scenario

i could move the test for the element outside of the parser and never end up in that scenario but this seems to be the preferred style throughout the project. i definitely don't think it should throw, though. is the structure what's objectionable?

No, it should stay here. If its a valid scenario it can stay

…w feedback

mikaelm12 · 2017-02-01T22:47:58Z

src/Microsoft.AspNetCore.Rewrite/IISUrlRewriteOptionsExtensions.cs

nit: Remove this extra new line

ondrejsevcik · 2017-03-14T03:55:44Z

Hi guys, do you have any idea when it get's released?

Thank you 👍

Tratcher · 2017-03-14T05:28:14Z

https://github.com/aspnet/Home/wiki/Roadmap Q2 is our next release.

david-peden-q2 added 2 commits November 4, 2016 11:03

#55 add rewrite map support to IIS

30b4b75

add tests for rewrite maps

2adedfc

dnfclas added the cla-signed label Nov 4, 2016

david-peden-q2 changed the title ~~#55 add rewrite map support to IIS with tests~~ #55 add IIS rewrite map support with tests Nov 4, 2016

jkotalik suggested changes Nov 4, 2016

View reviewed changes

code review feedback

7d0ad0f

davidpeden3 mentioned this pull request Dec 5, 2016

Url Rewrite Checklist #67

Closed

27 tasks

ensure lookups in rewrite maps are case-insensitive

d428eb9

jkotalik reviewed Dec 5, 2016

View reviewed changes

code review feedback

5faa822

Tratcher reviewed Dec 12, 2016

View reviewed changes

david-peden-q2 added 4 commits December 12, 2016 16:55

code review feedback

224665a

code review feedback

24f2ad7

code review feedback

87c900a

code review feedback

c2b536d

guardrex mentioned this pull request Jan 5, 2017

URL Rewriting doc dotnet/AspNetCore.Docs#2446

Merged

david-peden-q2 added 7 commits January 9, 2017 10:17

Merge branch 'dev' into rewritemaps

162114e

fix breakages after merging aspnet/basicmiddleware

bfae468

Merge branch 'dev' into rewritemaps

1ed4bc1

revert version number change per code review feedback

883a1b2

add extension method and initial failing tests

a88996d

add support for adding externally generated rewrite maps

2ccb462

add IISRewriteMapCollection

7a8ac6b

Tratcher requested a review from mikaelm12 January 20, 2017 21:09

Tratcher reviewed Jan 20, 2017

View reviewed changes

mikaelm12 suggested changes Jan 20, 2017

View reviewed changes

code review feedback

9eb1b74

david-peden-q2 added 2 commits January 30, 2017 12:13

add overloads to eliminate optional parameters per ms code standards

43fbf3b

remove extra ability to layer in external rewrite maps per code revie…

7d1cc11

…w feedback

mikaelm12 reviewed Feb 1, 2017

View reviewed changes

src/Microsoft.AspNetCore.Rewrite/IISUrlRewriteOptionsExtensions.cs Outdated

}

}

Copy link

Contributor

mikaelm12 Feb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Remove this extra new line

fix whitespace

461d587

Tratcher approved these changes Feb 7, 2017

View reviewed changes

Tratcher assigned mikaelm12 Feb 7, 2017

david-peden-q2 added 2 commits February 7, 2017 13:51

merge dev in

ad5b72c

fix breakages after merge

435fed7

mikaelm12 approved these changes Feb 7, 2017

View reviewed changes

mikaelm12 merged commit 3faa3de into aspnet:dev Feb 7, 2017

mikaelm12 mentioned this pull request Feb 7, 2017

add custom response support #200

Merged

ondrejsevcik mentioned this pull request Mar 16, 2017

Add full support for IISUrlRewrite.xml file format kontent-ai/boilerplate-net#4

Closed

		@@ -0,0 +1,22 @@
		using Microsoft.AspNetCore.Rewrite.Internal.IISUrlRewrite;

#55 add IIS rewrite map support with tests #168

#55 add IIS rewrite map support with tests #168

Uh oh!

Conversation

david-peden-q2 commented Nov 4, 2016

Uh oh!

dnfclas commented Nov 4, 2016

Uh oh!

jkotalik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelm12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!