Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Allow customizing what constitutes a controller for POCO controllers #1274

Closed
yishaigalatzer opened this issue Oct 8, 2014 · 13 comments
Closed

Comments

@yishaigalatzer
Copy link
Contributor

Right now, when a user writes a POCO controller it has to end with the controller suffix.

After writing the ApiController (which is basically a POCO controller) it behaves slightly differently than the non POCO controller.

We can make an MVC Option that is either a predicate indicate what controller is, or a list of base types that behave similarly to controller along the lines of IsAssignableFrom(baseType)

@yishaigalatzer yishaigalatzer changed the title Allow customizing what consitutes a controller for POCO controllers Allow customizing what constitutes a controller for POCO controllers Oct 8, 2014
@danroth27 danroth27 added this to the 6.0.0-rc1 milestone Oct 8, 2014
@danroth27
Copy link
Member

Some ideas we discussed:

  • Add a ControllerAttribute
  • Walk the hierarchy for derived types
  • Add a NonControllerAttribute
  • Consistency with view components
  • Add a predicate in options

@yishaigalatzer yishaigalatzer modified the milestones: 6.0.0-rc1, 6.0.0-beta3 Jan 11, 2015
@atrauzzi
Copy link

Just researching support for POCO controllers. Will this enhancement mean I wouldn't have to use the suffix "Controller" on my controllers?

@yishaigalatzer
Copy link
Contributor Author

This could absolutely mean that, but of course you will have to do something to let us know your class is a controller. See the list above for details, but some short examples.

  1. Assuming you want to build your own base type that doesn't derive from controller
    options.ControllersBaseTypes.Add(typeof(MyCustomBaseClass));
// or
   [Controller]
   class MyCustomBaseClass
// or
   class MyCustomBaseController // <-- Controller suffix indicates controller
  1. Assuming you want a class that doesn't derive from controller but has a different naming convention
    options.IsController = (Type t) => return t.Name.StartsWith("Foo");

You can of course customize the existing service that picks up controllers, but we prefer to make it a bit easier, and we advocate that replacing services in MVC6 will most of the times mean we got something wrong. We'd rather get it right from the get go, or provide an option for the 99% case.

It would be nice if you can share you plans and reasoning for using POCO controllers, we might just be able to accomodate it when we lock the design for this feature.

@atrauzzi
Copy link

I'm interested in the technical reasoning for requiring the controller to be tagged somehow for the runtime. Mainly because I suspect there might be a much more deliberate way to go about this (or have both options if some prefer convention).

What would be nice is if the POCO controller didn't require annotations. I'm coming from a PHP background, specifically Laravel. In that system, they allow for you to author routes as follows:

$router->get("/my/resource/{id}", 'My\Namespace\Controller\Resource@show');

Out of all the routing conventions, this shorthand (something I think you guys are already very close to matching), has been very nice to use. The simplicity is in the DSL. It allows the framework authors to not have to rely on tagging/reflection mechanisms to route. The framework also reflects on both the class and the action method to see if dependencies need to be injected.

It's worth repeating that the framework also supports convention based routing, so if desired, the developer can write less specific routes. So, in MVC 6, my hope would be to see something like the following:

// Instantiates the class "Resource" if non-static.  Otherwise, static-calls.
router.map("GET#/my/resource/{id}", "My.Namespace.Controller.Resource@Show");

You'll note the distinction about static controllers which is a concept similar to Scala's object type being used as a controller. In those scenarios, you can offer to perform setter injection.
I've also taken a slight liberty with where the HTTP action is being specified. I'm not especially particular on that, and it could be the name called instead.

@davidfowl
Copy link
Member

Technical reason? Because sometimes discovery is nice. When you don't want discovery and want to be explicit about everything, you can be but discovery makes the getting started much simpler.

@atrauzzi
Copy link

So that's saying it's required out of wanting to offer the convenience. I was asking if there was a technical limitation mandating that the tagging had to happen. But judging by your answer, there isn't - which is good!

Right now writing routes in ASP.NET MVC 5/6 isn't the prettiest.

This more explicit scenario allows for a much cleaner routing config. I can express 90% of routes, whether convention or custom using a single line instead of having to pollute wherever I define my routes with multiple lines and anonymous objects.

@davidfowl
Copy link
Member

I'm guessing that the majority would prefer to write it that way. The rest that don't can use whatever mechanism this bug gets resolved with.

@atrauzzi
Copy link

If you're doing the right thing, I think you'll find that over time, the opposite is true. To date, ASP.NET MVC users have only had one option - so you might be right for existing users today - but even that changes when a better option becomes available.

Don't forget the attrition of documentation too. If your only documented method is one way, the community takes it as prescriptive and will not question. That could even be in spite of whether it's a best practice or the most convenient out of two options. All it proves is that that most people are - rightfully - there to get the job done and won't be bothered to dig into internals because they expect you to lead them to safety.

That means that if my suggestions here are implemented they must be clearly documented & featured. Otherwise you'd definitely be right because nobody would know about the more user-friendly syntax! ;)
Which means, while I would agree that there is a preference at any given moment, we have to measure this outside of the ASP.NET MVC microcosm. Here's a tour of the routing conventions from other frameworks:

(I should note, I continuously reference Laravel because it's a very well designed framework. As of today, I consider its routing the gold standard.)

Taking all these examples into account, ASP.NET MVC is clearly an outlier and route mapping is one area I think you can have a really positive impact.

@davidfowl
Copy link
Member

Thanks for the lesson. I think only a few of these apply since we have to look past route definitions and discovery and do what is idiomatic given that c# is the language.

If we did inline routes (and we'll probably have a low level way of doing that directly in routing as isn't mvc specific at all) that would be cool for simple stuff.

Now the mvc pipeline includes, action selection, model binding filter execution, conneg etc .

Some of these features completely would need to be handled differently when you change the pattern and I think it'll take alot more effort to design a system around those patterns (and some are worse imo, at least for c#).

That said, if you want an explicit way to map controller classes, there will probably be one. Other than that, you can extend the framework or use one of the many others that exist on .net.

For comparison though, you should look around at some existing .net web frameworks as well to compare and contrast the approaches. Nancy is one that stands out pattern wise.

@atrauzzi
Copy link

Nice as it may seem, that approach sounds like it could get pretty out of hand (see my notes on nancy below). Routing is something you want to try and keep as simple as possible, and encourage people to move on.
Fortunately, it seems like MVCs routing system is already prepared for my suggestions - just change the touchpoints to have signatures that don't force people to write a lot of ugly boilerplate. I'm not suggesting the resultant functionality be modified. All the current mechanisms can be leveraged and you can have an overload on the route builder that still takes an anonymous object of extended options when absolutely essential.

I'm familiar with Nancy and didn't reference it for a few reasons. Overall for routing, I don't think it's user friendly. It also imposes some compromises that don't make sense - the most ugly of which is that it requires that all dependencies be resolved up front when routes are defined (ouch!).
(I'm happy to see any project gain a following, but I wouldn't let the fact that Nancy is on .NET put it ahead of what developers are doing in other languages. Better progress has been made elsewhere.)

@yishaigalatzer
Copy link
Contributor Author

So in MVC6 we don't recommend going with the pattern of defining routes with default objects, this was a very cumbersome syntax. When it comes to "documentation", the new templates are going to show the new pattern right from the get go.

So you would just: routes.AddRoute("{controller=Home}/{action=Index}/{id?}". Of course when it comes to routes that don't mention the actual controller name that becomes a bit more complex in the default routes. But the suggestion there is to use attribute routes. Infact for APIs we are not even going to support conventional routes and you will have to decorate your actions/controllers with attribute routes. Again the pattern is established right in the Web API template (coming out in the next release).

Now just like David suggests it is rather easy to implement something like you are looking for, but we are still debating what would an approach closer to your suggestion looks like. #1877 discuss a potential way to apply attribute routes in a centralized way, it can completely be written on top of current MVC without changing any of the MVC services implementing an IApplicationModelConvention. One can also make these methods be discovered (but that will require a service change, still doable but not very recommended).

#1877 It does not resolve the issue of having to have a controller suffix or derive from controller. But honestly I don't quite get why you won't do that. Following framework conventions makes new developers on your team having to learn less and be more effective.

So in short this issue is really about the following scenario:

  1. I'm used to the fact that as long I can derive from a controller, I don't have to name my class in any special way.
  2. My team decides to build a different base controller type (starting from a POCO), for example see ApiController
  3. MVC now doesn't pick up the controllers deriving from the new base type if they don't end with a suffix, which is not expected compared to the default framework behavior.

@atrauzzi
Copy link

Yeah - the syntax for options is not nice at all. But that doesn't mean specifying controllers explicitly is a bad practice. It means MVC misjudged the importance of getting this right.

Catch-all mappings like routes.AddRoute("{controller=Home}/{action=Index}/{id?}") -- are a bit of an antiquated practice nowadays. You'll even see that some frameworks are eliminating this convention all together:

In fact, the first thing I do in a framework project is remove any generic class-name-maps-to-route defaults. They simply aren't helpful.

What if I want two controllers named the same? I obviously namespace the classes at that point. But how does the router differentiate when all the action methods have been flattened into one namespace? Now you're having to kludge your app's external signature to satisfy a hard-baked convention.

Attribute mapping for the paths of controllers is also an issue. Especially if your controllers are supplied by external libraries (and the mapping is left to you). It comes up when you want to add a prefix to groups of controllers http://myapi.com/v1/items or http://myapi.com/v2/items. The controller should only be dealing with producing results, not mounting itself to the router.

There is really no one-shot solution for routing that you can call a "convention". It's better to have a minimal and flexible syntax instead of creating a rigid system and then having to code in confusing ways for the implementer to pre-empt it.

At first read, #1877 seems to reflect what I'm suggesting so far. But these two issues are definitely connected.

@yishaigalatzer
Copy link
Contributor Author

Design -

We will walk up and look for a base class that is named according to the "standard"
Add a NonControllerAttribute

pranavkm added a commit that referenced this issue Feb 12, 2015
determine if any ancestor has the "Controller" suffix.

* Introduce NonControllerAttribute to opt out of Controller detection.

Fixes #1274
pranavkm added a commit that referenced this issue Feb 12, 2015
determine if any ancestor has the "Controller" suffix.

* Introduce NonControllerAttribute to opt out of Controller detection.

Fixes #1274
pranavkm added a commit that referenced this issue Feb 12, 2015
determine if any ancestor has the "Controller" suffix.

* Introduce NonControllerAttribute to opt out of Controller detection.

Fixes #1274
pranavkm added a commit that referenced this issue Feb 13, 2015
determine if any ancestor has the "Controller" suffix.

* Introduce NonControllerAttribute to opt out of Controller detection.

Fixes #1274
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants