This project is read-only.

Controllers with generic constraints are ignored

Jan 12, 2015 at 9:55 PM
Edited Jan 12, 2015 at 9:57 PM
T4MVC will process something like this:
public class FooController : BaseController<MyModel> {
}
But not something like this:
public class BarController<T> : BaseController<T> {
}
...because it's generic. But why?

When using T4MVC in conjunction with RazorGenerator, we have multiple assemblies with a number of controllers and subclassed controllers, and those controllers with generic constraints are ignored by T4MVC. And they need to be detected so we can use them.

Is there some config setting I've missed, or some trick I don't understand to get this working?
Jan 13, 2015 at 1:14 AM
It's probably just something that has not come up. See the logic in IsController, which I'm guessing ends up saying false for these.
Jan 13, 2015 at 8:57 AM
Here is a better example:
public partial class XController<T> : Controller { /* ... */ }
public partial class YController<T> : XController<T> { /* ... */ }
public partial class ZController : YController<MyModel> { /* ... */ }
I found the problem in ProcessControllerType():
// Don't process generic classes (their concrete derived classes will be processed)
if (type.IsGeneric)
  return;
So:
  • XController is ignored: as it should be
  • YController is ignored: but I want it to be processed <-- here is the problem
  • ZController is processed
However, it seems to me that processing one or the other is arbitrary and based on my personal preference, so maybe someone else might want to process XController<T> even though I don't.

There are lots of ways to deal with this, but before I hack up the template, are there any ways that are preferable?

I'm thinking maybe a whitelist in the T4MVC.tt.settings.xml file, a CSV of controllers to be processed even if they're generic?
Jan 13, 2015 at 9:01 AM
...and to reiterate why I believe this is useful (and worthy of inclusion in the template),is when you use T4MVC + RazorGenerator, you might get a tree of controllers, where some are abstract, some are concrete, some are generic, etc. And in your client assembly, you might want to reference stuff from any one of them.

As our projects get bigger, we run into this problem all the time, and I want to kill the magic strings already!

If you like my approach, I'll send a git pull request for consideration.
Jan 13, 2015 at 11:31 AM
Edited Jan 13, 2015 at 1:53 PM
I've made a pull request

I'm wondering whether abstract controllers should be added to this idea, to sort out the issue from that other thread as well. So one whitelist for all "exceptional" controllers which the user wants processed, and which are typically ignored?
Jan 13, 2015 at 3:43 PM
Instead of a whitelist in the settings file, I would prefer that we come up with a T4MVCAttribute that can go directly on the controllers. e.g.
  • no attribute: use the current logic
  • [T4MVC(false)]: don't process this controller, ignoring all rules and naming patterns
  • [T4MVC(true)] (can be shortened to [T4MVC]: process this controller, ignoring all rules and naming patterns
Advantages of this approach:
  • It's easier to add this because it stays in the context of the controller.
  • It's refactoring friendly: if you rename the controller, it still works. While in the settings files, it's a maintenance pain.
  • We can use the same attribute for action methods. Just like controllers, there have been cases where the default rules don't work so well, and this would be useful to get out of trouble. And this would not work well at all in the settings file, as you'd have to specify both the controller name and the method name, compounding the pain points above.
It's also probably easier to implement :)
Jan 13, 2015 at 4:02 PM
That way we can target generic/abstract controllers at the same time as well. I like it.

Not sure where to start though... Let me have a crack at it and what I come up with.
Jan 13, 2015 at 4:10 PM
My previous commit from yesterday doesn't seem to be in the master branch any more? Last commit is from Nov 15, 2014

Does this make sense or am I misusing git? New to git... Should I include those changes in my new code?
Jan 13, 2015 at 5:07 PM
Sorry, that's my bad, I forgot to push! Just did it now :)
Jan 13, 2015 at 5:13 PM
BTW, remember to set your git username/email. e.g.
git config --global user.name "bugmenot2"
git config --global user.email your.name@something.com
Jan 14, 2015 at 10:01 AM
Edited Jan 14, 2015 at 6:41 PM
OK I've uploaded my first attempt. I've removed the previous commit's regex for controller names, as it's not needed anymore if we're moving to the [T4MVC] attribute approach.

Was a lot of work to get that attribute working for all cases, and it works, EXCEPT for the case I originally wanted! ...generic controllers. It works for the regular case (i.e. backwards compatible), weirdly-names controllers (so it obviates that regex), and abstract classes.

But it doesn't work in the generic controller case, yet, because I need to emit FooController<T> in various places, instead of FooController. That works in the .generated.cs file. But in the T4MVC.cs file there is a static instance which breaks when you add that generic suffix.
Jan 15, 2015 at 1:10 AM
Sorry, crazy day and I haven't had a chance to look at this yet. I'll find the time tomorrow!
Jan 15, 2015 at 7:21 AM
Edited Jan 17, 2015 at 8:19 AM
OK ignore previous posts...

I got it to work, well, mostly... I sent a "pull request" accidentally, but ignore it until we've fully tested this thing.

All works as you wanted, but not as I wanted! :)

So there's a [T4MVC] attribute which can be used to mark:
  • regular controllers (however this is unnecessary, as current logic would be used to identify such controllers)
  • abstract controllers (solves that issue I logged the other day)
  • weirdly named controllers, e.g. ControllerFoo or FooControllerA (which obviates the previous commit for that regex setting)
  • generic controllers, which is what I actually wanted... but, there's a problem here, see below
The single remaining problem, is in T4MVC.cs (ie not in my code, but what was already there). If you compile you'll see it needs a <T> to work, but I'm not sure if we can do that? Maybe we should just remove that line and live with reduced functionality for the generic case (all will work but it won't have action constants). Prefer to fix it somehow though...
Jan 21, 2015 at 2:17 PM
Maybe it's easier to go with the whitelist approach? That works as is...
Jan 28, 2015 at 7:50 PM
Well let's make it simpler then... If I had access to the HoloLens, I wouldn't waste time on some silly open source project either! :)

How about I take the latest commit, remove everything to do with generic controllers (which is the only bit that doesn't work as required), and so we're left with a T4MVC attribute that can be used on:
  • regular controllers
  • abstract controllers
  • weirdly-named controllers
That takes care of almost everything. We could then work on the generic case separately another time.

Simple enough for me to do, but let me know if you're interested in this before I do it. It was your (good) idea after all. It would also be backward compatible, as if you don't use the attribute, the template would work as expected.
Jan 28, 2015 at 7:52 PM
Yes, I'm good with that! Definitely we want backwards compat. If you don't use it, it should work as before.

Note that we've moved to https://github.com/T4MVC/T4MVC, so you can have the first PR there :)
Jan 29, 2015 at 4:01 PM
Edited Jan 31, 2015 at 8:57 AM
Basic cases were accepted in this commit

Generic case still work in progress here
Feb 15, 2015 at 12:17 AM
Hi,

I am currently working on the vnext implementation of T4MVC (its called R4MVC in the T4MVC github repo) and am interested in getting feedback on the methods of identifying controllers and both white/blacklisting specific edge cases.

I'm currently going on the assumption that all public classes inheriting at any level from Microsoft.AspNet.Mvc.Controller are included by default. This includes abstract and generic controllers. Is this sensible, should I allow for users to specify what class they want to use if they roll their own controllers/mvc framework.. or perhaps I missing something you guys already discovered in T4MVC?

Also if you are interested in having the similar type of attribute [R4MVC(false)] to force inclusion or exclusion in the vNext R4MVC then raise an issue on the github repo for this. It's only in early design stages at this time but all feedback is welcome :)

https://github.com/T4MVC/R4MVC
Feb 15, 2015 at 6:17 PM
@wwwlicious yes all classes should be included, also generic and abstract.

I don't know why they were excluded from T4MVC, but getting them to work is a pain, because we are doing it after the fact, and the codebase is a mess without comments (because it was written by so many people over many years). It's still great though! :)

If you are starting a fresh approach, include everything, it doesnt make sense to exclude anything, as many will have more than one assembly, and abstract/generic classes in various places as you would expect. The easiest whitelist/blacklist approach is to do what I did here, add a T4MMVC attribute, so user can change the default if he doesn't like it.

Looking forward to see what you come up with!
Feb 15, 2015 at 6:56 PM
Yup, big fan of T4MVC which is why I want to help get R4MVC up and running.

Hindsight is 20/20 as they say regarding various choices made but I think the important thing is to try and gather all the various lessons learned and pain points that exist in T4MVC today and get the discussions going on the github R4MVC repo.

This will help as we built it out so we are aware of them from the beginning. My usage of T4MVC was always fairly standard so I never ran into some of the issues on codeplex.