T4MVC and custom model binding

Editor
May 28, 2012 at 3:05 PM
Edited May 28, 2012 at 3:09 PM

I actively use custom model binding in MVC projects, for example, I've a binder for my DB-entities, so when I hit an url
   /Home/User?user=1 
a user with ID=1 is automatically loaded from DB and bound to the action:
   public ActionResult User(User user)

The problems come with generating urls: MVC.Home.User(user) gives me /Home/User?user=Project.Domain.User since it simply converts User using .ToString()

Overriding .ToString to return ID is usually not desirable.

So, I have to hack T4MVC at

<#if (!controller.NotRealController) { #>
[<#= GeneratedCode #>, DebuggerNonUserCode]
public class <#=controller.DerivedClassName #>: <#=controller.FullClassName #> {
public <#=controller.DerivedClassName #>() : base(Dummy.Instance) { }

<#foreach (var method in controller.ActionMethods) { #>
public override <#=method.ReturnTypeFullName #> <#=method.Name #>(<#method.WriteFormalParameters(true); #>) {
var callInfo = new T4MVC_<#=method.ReturnType #>(Area, Name, ActionNames.<#=method.ActionName #><# if (method.ActionUrlHttps) {#>, "https"<#}#>);
<#if (method.Parameters.Count > 0) { #>
<#foreach (var p in method.Parameters) { #>
callInfo.RouteValueDictionary.Add(<#=p.RouteNameExpression #>, <#=p.Name #>);
<#} #>
<#}#>

to add some custom handling for parameters.

What I'd like to have is a callback to some function from .settings.t4 at that point (inside foreach (var p in method.Parameters)), so everyone would be able to define his own custom binding rules (without modifying T4MVC itself).

I'd be happy to implement that if we agree that this thing is actually needed :)

Editor
May 28, 2012 at 3:12 PM

Here's SO question that could also be solved with this feature

Coordinator
May 28, 2012 at 4:13 PM

Yes, I think something like that would make sense. Basically, there would need to be a method that takes in the custom object and an IDictionary<string, object>, and fills in all the relevant route values in there. Or something along those lines.

But I'm not sure that having that in the settings.t4 file is ideal. It might make more sense to have that logic that lives alongside the custom object.

Anyway, feel free to have a go at it! :)

Editor
May 28, 2012 at 6:02 PM
Edited May 28, 2012 at 6:07 PM

There's one more point I'm uncertain about: I see 2 possibilities to do this callback.

1) To call some handler at t4 generation time and let it inject some code into Controller.generated.cs. This would result in no runtime overhead and resulted Controller.generated.cs would then look like (sure, with some null-checking that will be added in the handler):

 

public override System.Web.Mvc.ActionResult User(User user) {
   var callInfo = new T4MVC_ActionResult(Area, Name, ActionNames.PostMessagePro);
   callInfo.RouteValueDictionary.Add("user", user.Id);
   return callInfo;
} 

 

2)  To call some handler at runtime, passing in an IDictinary and stuff and let it add what it wants. Generated result will be like

public override System.Web.Mvc.ActionResult User(User user) {
   var callInfo = new T4MVC_ActionResult(Area, Name, ActionNames.PostMessagePro);
   T4MVCHelpers.AddRouteValues(callInfo.RouteValueDictionary, "user", user);
   return callInfo;
}  

The first case looks more promising to me, since the second could be easily implemented by end-user if the first is present :) What are your thoughts?

Coordinator
May 28, 2012 at 7:00 PM

I have to say I like the runtime approach a bit better. The extra runtime cost should be negligible since it's just a function call, which won't do much in most cases.

The way I sort of see this is that you're introducing the concept of a model unbinder, which more or less does the reverse of the binder. So ideally, we'd find a way to have this code live next to the binder itself. While with #1, it would have to be in the t4.settings file, which feels a little less maintainable, and farther from where it belongs.

But I guess I'm not sold strongly either way :)

An interesting thing looking at the SO thread is that in that case, he's using the field name 'cityAlias' in the URL, while you're using the controller param name 'user' ('city' in his case). There are probably pros & cons:

  • In his case, if there were multiple params of that type he'd be stuck, while with your pattern it would work.
  • But if had an object with multiple PKs, it's less clear what you'd do. Maybe the route params would be named 'user.ID'?

Something to keep in mind in the design, to make sure it's flexible enough.

Jun 16, 2012 at 11:44 AM

I actively use this ModelBinder approach and would be very glad to see it in T4MVC by default. But I wanted to point to one thing. It would be great to keep capability to get ActionResult by id, not by the binder object: it is useful in cases you just need to generate url. In other words I want to see two overloads for an example above:

MVC.Users.User(User user)

MVC.Users.User(long userId)

Jun 16, 2012 at 3:39 PM

I was trying to write some code which would generate all overloads (like above) but found it is possible to do only by extensions methods as MVC.Something returns SomethingController but not T4MVC_SomethingController. David, why is it done this way?

Coordinator
Jun 16, 2012 at 3:58 PM

It's to allow 'Go To Definition' on Action calls to go to the user code instead of the derived class generated code. Instead of adding code to the derived class, try adding code to the partial class.

Editor
Jun 23, 2012 at 12:03 PM

I've implemented something and tests say that it somehow works :)

But maybe there would be some corrections/advices on what/how it's done. I don't know git/codeplex workflow much, is it ok to just send a pull request or there must be some reviews/discussions done before?

Here's the fork, could you please take a look:

http://t4mvc.codeplex.com/SourceControl/network/forks/Shaddix/ModelUnbinder

Editor
Jun 23, 2012 at 12:06 PM
AlexIdsa wrote:

I actively use this ModelBinder approach and would be very glad to see it in T4MVC by default. But I wanted to point to one thing. It would be great to keep capability to get ActionResult by id, not by the binder object: it is useful in cases you just need to generate url. In other words I want to see two overloads for an example above:

MVC.Users.User(User user)

MVC.Users.User(long userId)

  Actually, that's not exactly what I've planned to do. It's kinda hard to guess which overload in each particular case you want, you could easily have ModelBinder that binds user by Login (in that case you'd like it to be MVC.Users.User(string userLogin)) or you could have guid as id, etc.

 It seems for me, that two overloads for an action is not what a ModelUnbinder could do..

Coordinator
Jun 26, 2012 at 9:19 PM

@shaddix, thanks for the changes!

I took a quick look. Some initial comments:

  • Please rebase or merge with latest T4MVC changes.
  • You have a lot of new code in the settings file that is likely not meant to be commonly user modified, so it's probably not the best place for it.
  • The diff shows a number of brace style changes (e.g. in t4mvctest.cs, global.asax, ...). It's best to leave unchanged to keep the diff as small as possible, which makes it easier to review.
  • Make sure you use spaces instead of tabs (you have tabs in the .tt and seetings files)

thanks!
David 

Editor
Jun 30, 2012 at 8:09 AM

Thanks for your comments, I've created another fork (https://git01.codeplex.com/forks/shaddix/modelunbinder2), because starting from scratch and copy-paste the needed parts was easier than fixing braces/tabs in place :) Sorry for not paying attention to the project code-style in first place.. )

So, new fork is made from latest version, it's ready for review and even contains a couple more fixes, that initial implementation :)

 

Regards,

Artur

Coordinator
Jun 30, 2012 at 4:01 PM

Thanks Artur, I'll try to look through it at some point this weekend.

One really quick think I notices is that generated files all lost their ending '#pragma warning restore 1591', which I'm guessing was not intentional. Maybe the line got shuffled and ended up in not quite the right place in the tt file? It's always good to look at the full diff to make sure that you can account for everything you see in there.

thanks,
David 

Editor
Jun 30, 2012 at 6:13 PM
It definitely wasn't intentional :)
Thanks for noticing, corrected.

2012/6/30 davidebbo <notifications@codeplex.com>

From: davidebbo

Thanks Artur, I'll try to look through it at some point this weekend.

One really quick think I notices is that generated files all lost their ending '#pragma warning restore 1591', which I'm guessing was not intentional. Maybe the line got shuffled and ended up in not quite the right place in the tt file? It's always good to look at the full diff to make sure that you can account for everything you see in there.

thanks,
David

Read the full discussion online.

To add a post to this discussion, reply to this email (t4mvc@discussions.codeplex.com)

To start a new discussion for this project, email t4mvc@discussions.codeplex.com

You are receiving this email because you subscribed to this discussion on CodePlex. You can unsubscribe on CodePlex.com.

Please note: Images and attachments will be removed from emails. Any posts to this discussion will also be available online at CodePlex.com


Coordinator
Jul 2, 2012 at 12:15 AM

Can you submit a pull request from your branch? That makes it easier to do code review on a line by line basis. I'm sure we'll get this in after round of review :)

Coordinator
Jul 3, 2012 at 12:54 PM

Ok, the change is in. Last thing I'd like you to do: write basic docs! :)

I made you an editor, so you should be able to change https://t4mvc.codeplex.com/documentation. Search for 'Shaddix', in section 3.1.

Thanks for your contribution!

Editor
Jul 3, 2012 at 1:06 PM

Thanks, David! :)

I think, I'll manage to write something in a day or two.

Editor
Jul 4, 2012 at 3:55 PM

Could you please also review the updated docs, I'm not sure if I wrote too much/too little, or even just a bunch of non-sense :)

Coordinator
Jul 7, 2012 at 10:41 PM

Doc is perfect, thanks!

Coordinator
Jul 10, 2012 at 12:14 PM

BTW, do you have a twitter name? @shaddix doesn't seem to be you. I tweeted about it here.

Editor
Jul 11, 2012 at 5:44 AM
Hi David, no twitter unfortunately (..or may be fortunately, isn't it a timekiller? :))

2012/7/10 davidebbo <notifications@codeplex.com>

From: davidebbo

BTW, do you have a twitter name? @shaddix doesn't seem to be you. I tweeted about it here.

Read the full discussion online.

To add a post to this discussion, reply to this email (t4mvc@discussions.codeplex.com)

To start a new discussion for this project, email t4mvc@discussions.codeplex.com

You are receiving this email because you subscribed to this discussion on CodePlex. You can unsubscribe on CodePlex.com.

Please note: Images and attachments will be removed from emails. Any posts to this discussion will also be available online at CodePlex.com


Jul 23, 2012 at 12:23 PM

There is a typo in UserUnbinder example: "user" parameter is used in one place by "routeValue" name

Editor
Jul 30, 2012 at 11:34 AM
AlexIdsa wrote:

There is a typo in UserUnbinder example: "user" parameter is used in one place by "routeValue" name

Thanks for your notice, corrected now

Coordinator
Jul 31, 2012 at 1:26 AM

@Shaddix: please take a look at http://stackoverflow.com/questions/11566207/t4mvc-optionalparameter-values-implied-from-current-context/11603897. I think your change caused a small regression. I have a potential fix in there (follow up over there).

Editor
Jul 31, 2012 at 10:04 AM

Actually at the moment T4MVC behaviour is identical to MVC:

having an action:

 

public virtual ActionResult ActionWithId(int? id = null) { 
return View(id);
}

 And a view:

 

@model dynamic
<a href="@Url.Action("ActionWithId")">a1</a>
<a href="@Url.Action(MVC.Home.ActionWithId())">a2</a>
@Model

 

The generated urls are the same for both T4MVC and MVC in both cases: /Home/ActionWithId/3 and /Home/ActionWithId

But since previously T4MVC behavior was different it's kinda regression.. in that case your proposed fix looks alright to me :)

Coordinator
Jul 31, 2012 at 12:16 PM

The major difference is that in regular MVC, you can pass 'new { id = null }' to force it back to null. But in T4MVC, Url.Action(MVC.Home.ActionWithId(null)) doesn't work, and there is no good workaround. This is because at the IL level, there is no difference between passing null and not passing anything,

So honoring the null in T4MVC seems like the lesser evil here.

Editor
Jul 31, 2012 at 1:32 PM

Agree with you :)

Should I make that change in a fork, or you'll do it directly on the master?

Coordinator
Jul 31, 2012 at 4:40 PM

I just made the fix. It's in T4MVC 2.10.1

Coordinator
Oct 18, 2012 at 11:30 PM

FYI, this thread brings up a possible ModelUnbinder regression.