Throw a better exception when the user didn't add the MVC services #1045
Conversation
| // Verify if AddMvc was done before calling UseMvc | ||
| // Try to get 2 sample services. If it returns null then AddMvc was not called. | ||
| if(app.ApplicationServices.GetServiceOrNull(typeof(IActionDescriptorsCollectionProvider)) == null || | ||
| app.ApplicationServices.GetServiceOrNull(typeof(IInlineConstraintResolver)) == null) |
There was a problem hiding this comment.
It might be better to define a transient marker service specifically for this purpose so there are no wierd interactions.
For instance IActionDescriptorsCollectionProvider might have another dependencies that also now get eagerly instantiated. Even if this is not a problem now, it might introduce a bug later if we change the implementation - or worse it could break a user-defined implementation.
Also IInlineConstraintResolver is defined by routing, and has its own extension methods in routing to add it if you're not using MVC, leading to a false positive.
There was a problem hiding this comment.
Agree. This is a good idea. Will implement this in the next iteration.
Would like to point out one corner case though - If the user decides to add all the services one by one manually (for some reason) then he might miss this service (which we are going to specifically use to validate AddMvc()).
|
⌚ |
| @@ -24,6 +25,14 @@ public static IBuilder UseMvc([NotNull] this IBuilder app) | |||
|
|
|||
| public static IBuilder UseMvc([NotNull] this IBuilder app, [NotNull] Action<IRouteBuilder> configureRoutes) | |||
| { | |||
| // Verify if AddMvc was done before calling UseMvc | |||
| // Try to get 2 sample services. If it returns null then AddMvc was not called. | |||
| if(app.ApplicationServices.GetServiceOrNull(typeof(IActionDescriptorsCollectionProvider)) == null || | |||
There was a problem hiding this comment.
space after if; have you run Stylecop?
|
Have updated the PR. Please have a look.. |
Yeah that's what I mean about getting more feedback. It's an extension method, but I dunno if using the full name of the extension methods makes it more or less clear what to do. |
| using System; | ||
| using Microsoft.Framework.DependencyInjection; | ||
|
|
||
| namespace Microsoft.AspNet.Mvc.Core |
|
Updated the PR.. Please have a look.. |
| /// </summary> | ||
| /// <param name="services">The list of services.</param> | ||
| /// <param name="serviceType">The type of service which needs to be searched for.</param> | ||
| public static object ThrowIfServiceDoesNotExist(IServiceProvider services) |
There was a problem hiding this comment.
should probably be something like ThrowIfMvcNotRegistered and return void.
There was a problem hiding this comment.
I have something to return so that this can be properly Unit Tested.. If its void, how do I write a positive test case?
There was a problem hiding this comment.
It just has to not-throw. Assert.DoesNotThrow(...)
|
⌚ |
| { | ||
| /// <summary> | ||
| /// Throws InvalidOperationException when the given type of service is not present | ||
| /// in the list of services. |
|
Updated.. |
| /// This is a Marker class which is used to determine if all the services were added | ||
| /// to when Mvc is loaded. | ||
| /// </summary> | ||
| public class MvcMarkerService |
There was a problem hiding this comment.
Mark it sealed. There's no utility in modifying this type.
There was a problem hiding this comment.
this isn't something we're spending time on in general. It's part of the public by default, unsealed by default principle.
|
|
from me.
|
|
|
Checked in. 791518d |
Issue #347 - Added a check in UseMvc() to verify if AddMvc() was called before.
Added functional tests for the same.