This project is read-only.
4

Closed

Request Context Parameters Populating Optional Parameter

description

This issue seems to of crept back up.

T4MVC OptionalParameter values implied from current context

The divisionteamid is being populated from the calling page even though I don't explicitly state it. If I change the parameter to teamid, it works and doesnt generate a URL with divisionteamid or teamid in this matter.

Controller Action
public virtual ActionResult Registration(int? id, string slug, int? divisionTeamId = null, int? divisionId = null)
{
       return GetRegistration(id, divisionId, divisionTeamId, new EventRegistrationViewModel());
}
Hyperlink
<a href="@Url.Action(MVC.Event.Registration(Model.Event.Id, Model.Event.Slug, null, null))">Registration</a>
Actual Output
/8206/10--tournament/registration?divisionteamid=5771
Expected Output
/8206/10--tournament/registration
Closed Jan 28, 2015 at 4:11 PM by KevinKuszyk
As part of our move to GitHub, we are closing all the outstanding issues on CodePlex.

If your issue is still valid, please re-open it on GitHub:
https://github.com/T4MVC/T4MVC/issues

Thanks for contributing!

comments

davidebbo wrote Aug 27, 2013 at 2:19 AM

Based on the code, I'm surprised that this doesn't work correct. Do you get the behavior you want if you use standard MVC syntax to explicitly pass null for those two params? In the end, all T4MVC does is come up with a dictionary of route values that get passed to MVC. In this case, I would expect it to have those two values, both set to null (the earlier bug was that those values were missing). What happens afterwards is MVC routing behavior.

One thing you can also do is debug the call to MVC.Event.Registration(), and look at the RouteValueDictionary in the returned object (which implements IT4MVCActionResult).

cblaze22 wrote Aug 30, 2013 at 7:16 PM

This was an issue on my end with a route being messed up. Sorry for the confusion.

cblaze22 wrote Aug 30, 2013 at 7:48 PM

I take that back it's still doing it.

davidebbo wrote Aug 30, 2013 at 7:50 PM

No problem, thanks for following up!

** Closed by davidebbo 08/30/2013 11:50AM

davidebbo wrote Aug 30, 2013 at 7:52 PM

Oh wait, I only saw your first comment and not the second. :)

So questions above still stand.

cblaze22 wrote Aug 30, 2013 at 7:58 PM

This works just fine.
<a href="@Url.Action("Registration", "Event", new{ slug=Model.Event.Slug, id = Model.Event.Id } )">Registration</a>

cblaze22 wrote Aug 30, 2013 at 8:51 PM

I also debugged the the "divisionteamid" is null.
public override System.Web.Mvc.ActionResult Registration(int? id, string slug, int? teamId, int? divisionId)
        {
            var callInfo = new T4MVC_System_Web_Mvc_ActionResult(Area, Name, ActionNames.Registration, "https");
            ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, "id", id);
            ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, "slug", slug);
            ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, "teamId", teamId);
            ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, "divisionId", divisionId);
            RegistrationOverride(callInfo, id, slug, teamId, divisionId);
            return callInfo;
        }

cblaze22 wrote Aug 30, 2013 at 8:51 PM

Ignore the "teamid", I reverted to that to make it work, "divisionteamid" was the original parameter.

cblaze22 wrote Aug 30, 2013 at 9:05 PM

This is my global.asax, dont see anything wrong with it.

            routes.MapRoute(
                 "EventTeam",
                 "{Id}/{eventslug}/{divisionteamid}/{slug}",
                 new { controller = "Event", action = "Team", eventSlug = "event", slug ="team" },
                 new { Id = @"\d+", divisionteamid = @"\d+" }
            );

            routes.MapRoute(
             "NamedHomeEvent",
             "{Id}/{slug}",
             new { controller = "Event", action = "Index", slug = "event" },
             new { Id = @"\d+" }
            );

            routes.MapRoute(
              "NamedEvent",
              "{Id}/{slug}/{action}",
              new { controller = "Event", action = "Index", slug = "event" },
              new { Id = @"\d+" }
            );

cblaze22 wrote Aug 30, 2013 at 9:09 PM

Here is another example:

davidebbo wrote Aug 30, 2013 at 10:19 PM

I'm pretty confused between teamid, divisionteamid and divisionId. In the standard MVC case, what behavior do you get if you explicitly pass teamid=null and divisionId=null? That should match the T4MVC behavior.

cblaze22 wrote Aug 30, 2013 at 11:01 PM

I just said ignore "teamid" its really called "divisionteamid" in the version that doesnt work. I pasted the version that does work by mistake.

davidebbo wrote Aug 30, 2013 at 11:26 PM

I see. So is the behavior the same with plain MVC if you pass divisionteamid=null and divisionId=null?

The goal here is to tell apart normal MVC behavior from T4MVC behavior.

cblaze22 wrote Aug 30, 2013 at 11:44 PM

If you mean plain MVC behavior in the code below, then yes it works correctly.
<a href="@Url.Action("Registration", "Event", new{ slug=Model.Event.Slug, id = Model.Event.Id } )">Registration</a>

davidebbo wrote Aug 31, 2013 at 12:27 AM

No, I mean plain MVC if you pass divisionteamid=null and divisionId=null. Do you then get the same as T4MVC behavior?

cblaze22 wrote Aug 31, 2013 at 1:02 AM

Not following what you mean then.

davidebbo wrote Aug 31, 2013 at 2:57 AM

I mean try this:
@Url.Action("Registration", "Event", new{ slug=Model.Event.Slug, id = Model.Event.Id, divisionteamid=null, divisionId=null } )
and compare behavior to the T4MVC call.

cblaze22 wrote Sep 1, 2013 at 12:30 AM

I get the error when assigning nulls like that.
Compiler Error Message: CS0828: Cannot assign <null> to anonymous type property

davidebbo wrote Sep 1, 2013 at 1:42 AM

Just use (string)null instead of null.

cblaze22 wrote Sep 1, 2013 at 9:16 PM

So that generates:
/8206/10--tournament/registration?divisionteamid=5771
What does that mean?

davidebbo wrote Sep 1, 2013 at 10:01 PM

I means that it matches the MVC behavior given the same set of params.

To fix your issue, I suggest doing this, relying on the param-less overload:
@Url.Action(MVC.Event.Registration().AddRouteValues(new { id=Model.Event.Id, slug=Model.Event.Slug }))
I really don't think there is a bug at the T4MVC level. If the method has default params, the behavior is going to be the same as if you were passing those same values explicitly (it's identical at the IL level).

cblaze22 wrote Sep 1, 2013 at 10:22 PM

Ok thanks

mimse wrote Dec 4, 2013 at 12:10 PM

I have the same problem

I have a controller action with two optional parameters
 public virtual ActionResult Index(int? id, int? pageNumber = null, bool? outletItems = null)
@Url.Action(MVC.Product.Index(Model.ProductGroupId))
Gives the following result
/product/index/4?outletItems=true
@Url.Action(MVC.Product.ActionNames.Index, MVC.Product.Name, new { ProductGroupId = Model.ProductGroupId })
Gives the following result (the expected result)
/product?ProductGroupId=4;
@Url.Action(MVC.Product.ActionNames.Index, MVC.Product.Name, new { ProductGroupId = Model.ProductGroupId,  outletItems = (string)null })
Gives the following result (more or less the same as with T4)
/product?ProductGroupId=1&outletItems=true

It seems to me like T4 is adding null value parameters to the RouteValueDictionary. And if that parameter has a value in the Request Context it uses this value.

Solution, dont add null value parameters to RouteValueDictionary

mimse wrote Dec 4, 2013 at 12:34 PM

As a solution I have added

A null check to line 234 and 342
if(<#=p.Name #> != null) ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, <#=p.RouteNameExpression #>, <#=p.Name #>);
And it now generates
 public override System.Web.Mvc.ActionResult Index(int? id, int? pageNumber, bool? outletItems)
        {
            var callInfo = new T4MVC_System_Web_Mvc_ActionResult(Area, Name, ActionNames.Index);
            if(id != null) ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, "id", id);
            if(pageNumber != null) ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, "pageNumber", pageNumber);
            if(outletItems != null) ModelUnbinderHelpers.AddRouteValues(callInfo.RouteValueDictionary, "outletItems", outletItems);
            IndexOverride(callInfo, id, pageNumber, outletItems);
            return callInfo;
        }

mimse wrote Dec 4, 2013 at 12:36 PM

Version 3.7.4

davidebbo wrote Dec 4, 2013 at 11:29 PM

My concern is that this could break some scenarios. Suppose that your pageNumber param was not optional, and that you wanted to generate a link using T4MVC, deliberately passing null in there. With your change, that null would end up getting ignored.

mimse wrote Dec 9, 2013 at 10:19 AM

But if you pass null to pageNumber, you get ?pageNumber=null? And the MVC model binder wont see that as null but as string "null"

davidebbo wrote Dec 10, 2013 at 2:44 AM

I see. So I think what you're saying is that this issue is technically unrelated to optional parameters, but that anytime someone passes 'null' to a T4MVC method (whether explicitly or via optional param), then it will do something that doesn't make sense. Correct?

If so, then maybe your fix is right. Do you want to send a PR?

mimse wrote Dec 11, 2013 at 12:08 PM

Yes that is what i'm trying to say :-)

Sure i can do that

davidebbo wrote Dec 11, 2013 at 5:24 PM

So thinking about this some more, I'm not convinced this it is always correct. Consider the following Action (e.g. try it on a default new MVC project):
        public virtual ActionResult Index(int? id)
        {
            if (id == 7)
            {
                return RedirectToAction(MVC.Home.Index(null));
            }

            return View();
        }
And then browse to /Home/Index/7. With the current behavior, the null will successfully yank the id, causing the redirect to go to /Home/Index (which actually just becomes /). But if we make this change, it will be ignored, and the redirect will go to /Home/Index/7.

So while I agree that in some cases, ignoring the null is correct, it can also cause some regressions in some existing apps. Do you agree?