Correct NullReferenceException on ServerVariables collection

[ashleyglee]

Related to this post: http://stackoverflow.com/q/38854251/4023606

When authenticationManager.User = new ClaimsPrincipal(); is executed, the following 3 values (AUTH_TYPE, AUTH_USER, REMOTE_USER) are set to null inside the System.Web.HttpContext.Current.Request.ServerVariables object, even though the keys are still there inside the collection. This causes any exception thrown by the application after that line to not get logged because Exceptional doesn't handle the NullReferenceException.

[CptRobby]

@ashleyglee Instead of simply adding an extra catch block to handle the NullReferenceException that is causing your specific problem, I would suggest changing the existing catch block that is handling HttpRequestValidationException to just handle all Exceptions. It doesn't really make sense to only gracefully handle certain exceptions but allow other exceptions to cause Exceptional to fail.

And for even more resilience, I would change the function to copy the values iteratively instead of using the copy constructor. So it would look something like this (starting from line 122):

Func<Func<HttpRequest, NameValueCollection>, NameValueCollection> tryGetCollection = getter =>
    {
        try
        {
            var original = getter(request);
            var copy = new NameValueCollection();
            foreach (var key in original.AllKeys)
            {
                try
                {
                    foreach (var value in original.GetValues(key))
                    {
                        copy.Add(key, value);
                    }
                }
                catch (Exception e)
                {
                    Trace.WriteLine($"Error getting collection values [{key}]: {e.Message}");
                    copy.Add(key, $"[Error getting values: {e.Message}]");
                }
            }
            return copy;
        }
        catch (Exception e)
        {
            Trace.WriteLine("Error parsing collection: " + e.Message);
            return new NameValueCollection {{CollectionErrorKey, e.Message}};
        }
    };

As you can see, in addition to handling any exception encountered while trying to fetch the collection itself (which I think would only happen when used outside of a web environment) it handles any exception encountered while fetching the values for a specific key. This would mean that in your error condition, the Error.ServerVariables collection would have error messages for the 3 keys that were throwing exceptions, but would still have the correct values for the other 53 keys. I'd say that would be a definite improvement in reliability and would help to identify problems that are encountered in the error reporting process. :+1:

PS: Please feel free to copy/edit the above code and use it in your pull request however you like. :wink:

[NickCraver]

Looks good - thanks for the PR!