Skip to content

IControllerConfiguration: An item with the same key has already been added #313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ai-fwd opened this issue Jun 21, 2018 · 12 comments
Closed

Comments

@ai-fwd
Copy link

ai-fwd commented Jun 21, 2018

I'm getting an error about the same key being added when trying to loop through the ApiDescriptions. My setup is pretty simple in that i have:

[Authorize]
public class BaseController : ApiController { ... }
[AllowAnonymous, MyCustomAuthorizationFilterAttribute, MyCustomIControllerConfiguration]
public abstract class BaseEntityController : BaseController { ... }

I have two controllers that inherit from these base classes that have no duplicate names or actions.

Observations:

  1. If both controllers inherit from the same base class there is no problem
  2. If both controllers inherit a different base class the error is thrown
  3. If (2) and I remove the MyCustomIControllerConfiguration attribute no error is thrown.

MyCustomIControllerConfiguration is as follows:

public class MyCustomIControllerConfiguration  : Attribute, IControllerConfiguration
    {
        public void Initialize(HttpControllerSettings controllerSettings, HttpControllerDescriptor controllerDescriptor)
        {
            var formatter = controllerSettings.Formatters.OfType<JsonMediaTypeFormatter>().Single();
            controllerSettings.Formatters.Remove(formatter);

            formatter = new JsonMediaTypeFormatter
            {
                SerializerSettings = { ... }
            };

            controllerSettings.Formatters.Add(formatter);
        }
    }

Removing the attribute isn't an option. It's odd that (1) is true with the attribute considering the error. Has anyone seen this type of error before?

Stack Trace:

[ArgumentException: An item with the same key has already been added.]
System.ThrowHelper.ThrowArgumentException(ExceptionResource resource) +56
System.Collections.Generic.Dictionary2.Insert(TKey key, TValue value, Boolean add) +14763607 System.Web.Http.HttpRouteCollection.Add(String name, IHttpRoute route) +44 System.Web.Http.Routing.AttributeRoutingMapper.AddGenerationHooksForSubRoutes(HttpRouteCollection routeTable, IEnumerable1 entries) +147
System.Web.Http.Routing.<>c__DisplayClass2.b__0(HttpConfiguration config) +149
System.Web.Http.HttpConfiguration.ApplyControllerSettings(HttpControllerSettings settings, HttpConfiguration configuration) +94
System.Web.Http.Controllers.HttpControllerDescriptor.InvokeAttributesOnControllerType(HttpControllerDescriptor controllerDescriptor, Type type) +200
System.Web.Http.Controllers.HttpControllerDescriptor.InvokeAttributesOnControllerType(HttpControllerDescriptor controllerDescriptor, Type type) +63
System.Web.Http.Controllers.HttpControllerDescriptor..ctor(HttpConfiguration configuration, String controllerName, Type controllerType) +131
Microsoft.Web.Http.Description.VersionedApiExplorer.FlattenApiVersions() +474
Microsoft.Web.Http.Description.VersionedApiExplorer.InitializeApiDescriptions() +264
System.Lazy1.CreateValue() +236 System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() +31 System.Lazy1.get_Value() +14620009

[TargetInvocationException: Exception has been thrown by the target of an invocation.]
System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor) +0
System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments) +128
System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) +142
Owin.Loader.<>c__DisplayClass12.b__b(IAppBuilder builder) +93
Owin.Loader.<>c__DisplayClass1.b__0(IAppBuilder builder) +212
Microsoft.Owin.Host.SystemWeb.OwinAppContext.Initialize(Action1 startup) +873 Microsoft.Owin.Host.SystemWeb.OwinBuilder.Build(Action1 startup) +51
Microsoft.Owin.Host.SystemWeb.OwinHttpModule.InitializeBlueprint() +101
System.Threading.LazyInitializer.EnsureInitializedCore(T& target, Boolean& initialized, Object& syncLock, Func`1 valueFactory) +135
Microsoft.Owin.Host.SystemWeb.OwinHttpModule.Init(HttpApplication context) +160
System.Web.HttpApplication.RegisterEventSubscriptionsWithIIS(IntPtr appContext, HttpContext context, MethodInfo[] handlers) +581
System.Web.HttpApplication.InitSpecial(HttpApplicationState state, MethodInfo[] handlers, IntPtr appContext, HttpContext context) +168
System.Web.HttpApplicationFactory.GetSpecialApplicationInstance(IntPtr appContext, HttpContext context) +414
System.Web.Hosting.PipelineRuntime.InitializeApplication(IntPtr appContext) +369

[HttpException (0x80004005): Exception has been thrown by the target of an invocation.]
System.Web.HttpRuntime.FirstRequestInit(HttpContext context) +532
System.Web.HttpRuntime.EnsureFirstRequestInit(HttpContext context) +111
System.Web.HttpRuntime.ProcessRequestNotificationPrivate(IIS7WorkerRequest wr, HttpContext context) +714

@commonsensesoftware
Copy link
Collaborator

Looks like this might be an issue with your routing configuration. I don't have the complete picture of your setup, but the exception is occurring where attribute-based routes (formally known as direct routes) are being added to the pipeline. The RoutePrefixAttribute and RouteAttribute are typically not honored in inheritance scenarios. That could explain why you see it for some scenarios.

If you can share more of your setup, that will help. Better still, if you have complete, albeit stripped down, repro, that will go a long way at trying to troubleshoot the issue.

@ai-fwd
Copy link
Author

ai-fwd commented Jun 22, 2018

Thanks for the quick reply. When you say "an issue with your routing configuration" do you mean specifically related to the VersionedApiExplorer or in general? Before integrating the VersionedApiExplorer I had no issues getting Swagger to document with my current setup.

None of my BaseControllers are decorated with the RoutePrefixAttribute or the RouteAttribute and neither of them contain any actions. Those attributes are only present in the sub classes.

The entire solution only uses attribute based routing (i.e there is no route table setup in the WebApiConfig). The versioning setup is as follows:

// Web API routes with URL path versioning (i.e. /v1)
var constraingResolver = new DefaultInlineConstraintResolver { ConstraintMap = { ["apiVersion"] = typeof(ApiVersionRouteConstraint) } };
// reporting api versions will return the headers "api-supported-versions" and "api-deprecated-versions"
config.AddApiVersioning(o => o.ReportApiVersions = true);
config.MapHttpAttributeRoutes(constraingResolver);

// add the versioned IApiExplorer and capture the strongly-typed implementation (e.g. VersionedApiExplorer vs IApiExplorer)
// note: the specified format code will format the version as "'v'major[.minor][-status]"
var apiExplorer = config.AddVersionedApiExplorer(
    options =>
    {
        options.GroupNameFormat = "'v'VVV";

        // note: this option is only necessary when versioning by url segment. the SubstitutionFormat
        // can also be used to control the format of the API version in route templates
        options.SubstituteApiVersionInUrl = true;
    });

While trying to pinpoint the issue this morning I was able to find another piece to the puzzle. In one of my controllers that inherits from BaseController i had the following action:

[ApiVersion("1.0")]
    [RoutePrefix("v{version:apiVersion}/account")]
    public class AccountsController : BaseController
    {
        [AllowAnonymous]
        [HttpGet]
        [Route("confirmEmail", Name = "ConfirmEmailRoute")]
        public async Task<IHttpActionResult> ConfirmEmail(string userId, string code) 

When I removed the Name = "ConfirmEmailRoute", everything started working correctly. I can't seem to find any mention of "ConfirmEmailRoute" so maybe this is legacy code but that route name was not duplicated anywhere, nor the controller prefix so why would it have already been added to the route table?

It's odd that it worked when removing the IControllerConfiguration and now works correctly after removing the Route's Name. Is there something in the VersionedApiExplorer that would cause it to be added twice because of that specific setup?

@commonsensesoftware
Copy link
Collaborator

This is exactly the type of information I was hoping to see. The issue is not with the VersionedApiExplorer, that just happens to be where the issue is ultimately manifesting.

The issue you're facing appears to be related to the presence of the route name a la [Route(Name="...")]. The Web API infrastructure builds up routes from this information using the IDirectRouteProvider and inserts them into the route table. The names cannot be duplicated. Since you are using inheritance, multiple controllers are registering the same route template (ok) and name (not ok).

Unfortunately, route names are not API version-aware. It's novel ideal to support in some future release, but I'm not even sure if it's technically possible. The guidance and only feasible solution I know of is to either ditch using names or append a version to the name (ex: ConfirmEmailRouteV1). The second option doesn't work if you use inheritance and place the attributes on the base class. Each subclass needs to define the route attributes.

A lot of people don't like to hear it, but honestly feel that inheritance is the wrong way to think about the problem. I'm all for reducing duplication; however, most don't understand all the other complexities that come with inheritance:

  1. The default IDirectRouteProvider does not honor inherited routing attributes
    a. You can make it work with minimal effort, but this approach doesn't translate to ASP.NET Core if/when you migrate
  2. You cannot uninherit a route. Many service authors only think about being additive, when it's quite reasonable that a route will be completely sunset
  3. If the route is defined by a base class, the subclass cannot change the route template. Changes to the route template can be subtle. Consider v1.0 had api/values/{id} and v2.0 wants api/values/{id:int}. If you change the template in 1.0, it might break something.

If you're going to use inheritance, I suggest having the base class only include protected methods that are generally reusable by subclass. If you're really careful, it's possible to make public virtual methods without route information work, but it can get tricky. Most of the logic should be in the proverbial business layer or in reusable decorators (i.e. IHttpActionFilter) for things like validation. I've found this to reduce action code to 5-10 lines max. Duplicating that is a non-issue since it's literally a copy and paste. If there is a change between implementations, I know I didn't break the other. Having a defined versioning policy like N-2 versions also make things easier to reason about.

Stepping off soapbox

Hopefully that unblocks you and gets you back on your way.

@ai-fwd
Copy link
Author

ai-fwd commented Jun 22, 2018

OK you lost me a little bit there. In my specific scenario the base controllers are abstract and don't have any Route specific attributes and contain no actions. They do contain the attributes I've shown above (i.e. AllowAnon, IControllerConfiguration, etc.) but nowhere do they define any routes configuration.

Are you saying having any attributes (like what i have) is still the root cause of the issue? In short, I need to move all of those to the specific controllers to resolve the issue permanently if I wanted to use the "Name" and avoid future problems?

@commonsensesoftware
Copy link
Collaborator

Ah … sorry … misread.

In short, if two routes defined by routes anywhere have the same name (e.g. key), you'll face this problem. It doesn't matter if you're using inheritance or not. The names used in the route table are unioned.

@ai-fwd
Copy link
Author

ai-fwd commented Jun 22, 2018

Right but that is not the case. That route name was never duplicated... so that problem should not have manifested, correct?

@commonsensesoftware
Copy link
Collaborator

You stated:

When I removed the Name = "ConfirmEmailRoute", everything started working correctly...

This should mean the route was either duplicated somewhere in code or was duplicated via inheritance when the route is registered 2+ times.

From the API Explorer's perspective, it doesn't care about such things. It's simply asking Web API for the all the routes and then explores them. This appears to happen when the routes are explored. Does the application even run? I would expect the same problem to arise when the route table is built in all cases, not just for the API Explorer.

@ai-fwd
Copy link
Author

ai-fwd commented Jun 22, 2018

That's the thing -- it's been running without issue. It's in production, so the only net new addition to the code was the versioning support. There has never been an issue with the route table before.

This problem has only arisen after using the VersionedApiExplorer.

At the end of the day it's working without the Name="ConfirmEmailRoute" but that Name was never duplicated (or used) throughout the solution.

Again, at first it seemed like an issue with the IControllerConfiguration so maybe when the VersionedApiExplorer is "FlattenVersionApi" it's doing something that is causing it to be registered twice. Nothing in my code is registering twice...

@commonsensesoftware
Copy link
Collaborator

Congratulations! You are the proud winner of a shiny, new bug! =P

So the issue is actually in the VersionedApiExplorer, but it's so weird. The API Explorer creates a dummy HttpControllerDescriptor to collect the API version information because that's what all the extension methods as such are already based on. Apparently the constructor that I chose to use has side effects that I didn't know about. Specifically, if the associated controller type implements IControllerConfiguration, it tries to initialize it. This results in a recursive call that produces this issue because the configuration and route table has already been initialized.

This is a very simple fix. I should have something out shortly.

@ai-fwd
Copy link
Author

ai-fwd commented Jun 22, 2018

OK great. No rush on my side since I removed Name anyway.

@commonsensesoftware
Copy link
Collaborator

This also surfaced a related, but previously undetected issue. The formatters were always coming from the original configuration, but a IControllerConfiguration can modify the formatters, which yields an entirely new configuration. It's not particular common and know one has ever reported it before, but it could happen. Interestingly, my OData version does the right thing. I suspect this was an issue in the original API Explorer code I forked from Web API. Both issues will be fixed and published soon.

@commonsensesoftware
Copy link
Collaborator

The fix has been published as https://www.nuget.org/packages/Microsoft.AspNet.WebApi.Versioning.ApiExplorer/1.2.1

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants