Request.QueryString can contain null keys and t4mvc does not like this.

Developer
Sep 26, 2012 at 12:48 AM

If you have a request like

 

localhost:/mycontroller/myaction?test=3

 

everything is fine.  Request.QueryString has a key of 'test' and a value of '3'.

If you do MVC.MyController.MyAction().AddRouteValues(Request.QueryString) everything works.

If you have a request like

localhost:/mycontroller/myaction?test

 then Request.QueryString contains an item that has a key of null!

 

If you do MVC.MyController.MyAction().AddRouteValues(Request.QueryString) that calls Request.QueryString.CopyTo() and that fails.

Really, I think Request.QueryString should not have this null key...but not much I can do about that.  In addition, the System.Web.Mvc.NameValueCollectionExtensions CopyTo code could simply check for null.....

Unfortunately the QueryString collection is read only, so this is the kind of fix we would be looking at:

 

        public static ActionResult AddRouteValues(this ActionResult result, System.Collections.Specialized.NameValueCollection nameValueCollection) {
            // Copy all the values from the NameValueCollection into the route dictionary
            System.Collections.Specialized.NameValueCollection filtered = new System.Collections.Specialized.NameValueCollection(nameValueCollection);
            filtered.Remove(null);
            filtered.CopyTo(result.GetRouteValueDictionary());
            return result;
        }

 

Or doing your own CopyToFixed extension:

 

    /// <summary>
    /// Copies the specified collection to the specified destination.
    /// </summary>
    /// <param name="collection">The collection.</param><param name="destination">The destination.</param>
    public static void CopyTo(this NameValueCollection collection, IDictionary<string, object> destination)
    {
      NameValueCollectionExtensions.CopyTo(collection, destination, false);
    }

    /// <summary>
    /// Copies the specified collection to the specified destination, and optionally replaces previous entries.
    /// </summary>
    /// <param name="collection">The collection.</param><param name="destination">The destination.</param><param name="replaceEntries">true to replace previous entries; otherwise, false.</param>
    public static void CopyTo(this NameValueCollection collection, IDictionary<string, object> destination, bool replaceEntries)
    {
      if (collection == null)
        throw new ArgumentNullException("collection");
      if (destination == null)
        throw new ArgumentNullException("destination");
      foreach (string key in collection.Keys)
      {
        //either fix like this and dont allow null keys at all...
        if (key!=null){
           if (replaceEntries || !destination.ContainsKey(key))
             destination[key] = (object) collection[key];
        }
        //or fix like this...
        if (key!=null){
           if (replaceEntries || (key!=null && !destination.ContainsKey(key)))
             destination[key] = (object) collection[key];
        }
} }

Thoughts?  BTW, here is the error generated...

[ArgumentNullException: Value cannot be null.
Parameter name: key]
   System.ThrowHelper.ThrowArgumentNullException(ExceptionArgument argument) +74
   System.Collections.Generic.Dictionary`2.FindEntry(TKey key) +71
   System.Collections.Generic.Dictionary`2.ContainsKey(TKey key) +36
   System.Web.Routing.RouteValueDictionary.ContainsKey(String key) +41
   System.Web.Mvc.NameValueCollectionExtensions.CopyTo(NameValueCollection collection, IDictionary`2 destination, Boolean replaceEntries) +134
   System.Web.Mvc.NameValueCollectionExtensions.CopyTo(NameValueCollection collection, IDictionary`2 destination) +7
   System.Web.Mvc.T4Extensions.AddRouteValues(ActionResult result, NameValueCollection nameValueCollection)

 

 

 

 

 

 

 

 

Coordinator
Sep 26, 2012 at 2:33 AM

Yeah, pretty funky how that works with the null key. It's probably some kind of legacy behavior from the really early ASP.NET days. So what happens if you have many of those, e.g.

localhost:/mycontroller/myaction?test&test2

I'm guessing they all end up under the same null key? I agree it's worth working around.

 

Developer
Sep 27, 2012 at 1:54 AM

Not sure, but I think the problem is the QueryString property is not created properly - and that is ASPNET issue.

Anyway, if you want to throw a fix in there (my fixes from above or one of your own), I would get one less error reported from the website!

Or did you need me to fix?

Coordinator
Sep 27, 2012 at 1:18 PM

If you don't mind sending a pull request with a unit test to cover this null case, that'd be ideal. I have so little time to work on T4MVC lately, that all the help is appreciated!

Your first solution looks simpler, even if it's not quite as efficient (extra collection copy).

Developer
Oct 1, 2012 at 12:23 AM

Done.

Coordinator
Oct 1, 2012 at 1:12 AM

It's in 2.10.3. Thanks for fixing this bug Wayne!