Action parameter gets defined twice when using multiple odata routes

[martindafonte]

Hi, I'm building a sample OData project with versioning in AspNet Core. And I've encountered a problem when trying to define an Action on one of my controllers.

When trying to build the swagger.json file the following Exception is thrown:

Activated	Event	Time	Duration	Thread
	Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware:Error: An unhandled exception has occurred while executing the request.

System.ArgumentException: Duplicate type name within an assembly.
   at System.Reflection.Emit.ModuleBuilder.CheckTypeNameConflict(String strTypeName, Type enclosingType)
   at System.Reflection.Emit.AssemblyBuilderData.CheckTypeNameConflict(String strTypeName, TypeBuilder enclosingType)
   at System.Reflection.Emit.TypeBuilder.Init(String fullname, TypeAttributes attr, Type parent, Type[] interfaces, ModuleBuilder module, PackingSize iPackingSize, Int32 iTypeSize, TypeBuilder enclosingType)
   at System.Reflection.Emit.ModuleBuilder.DefineType(String name, TypeAttributes attr)
   at Microsoft.AspNet.OData.DefaultModelTypeBuilder.CreateTypeBuilderFromSignature(ModuleBuilder moduleBuilder, ClassSignature class) in E:\BA\56\s\src\Common.OData.ApiExplorer\AspNet.OData\DefaultModelTypeBuilder.cs:line 236
   at Microsoft.AspNet.OData.DefaultModelTypeBuilder.NewActionParameters(IServiceProvider services, IEdmAction action, ApiVersion apiVersion, String controllerName) in E:\BA\56\s\src\Common.OData.ApiExplorer\AspNet.OData\DefaultModelTypeBuilder.cs:line 60
   at Microsoft.AspNetCore.Mvc.ApiExplorer.PseudoModelBindingVisitor.CreateResult(ApiParameterDescriptionContext bindingContext, BindingSource source, String containerName) in E:\BA\56\s\src\Microsoft.AspNetCore.OData.Versioning.ApiExplorer\AspNetCore.Mvc.ApiExplorer\PseudoModelBindingVisitor.cs:line 85
   at Microsoft.AspNetCore.Mvc.ApiExplorer.PseudoModelBindingVisitor.Visit(ApiParameterDescriptionContext bindingContext, BindingSource ambientSource, String containerName) in E:\BA\56\s\src\Microsoft.AspNetCore.OData.Versioning.ApiExplorer\AspNetCore.Mvc.ApiExplorer\PseudoModelBindingVisitor.cs:line 45
   at Microsoft.AspNetCore.Mvc.ApiExplorer.ODataApiDescriptionProvider.GetParameters(ApiParameterContext context) in E:\BA\56\s\src\Microsoft.AspNetCore.OData.Versioning.ApiExplorer\AspNetCore.Mvc.ApiExplorer\ODataApiDescriptionProvider.cs:line 447
   at Microsoft.AspNetCore.Mvc.ApiExplorer.ODataApiDescriptionProvider.NewODataApiDescriptions(ControllerActionDescriptor action, String groupName, ODataRouteMapping mapping)+MoveNext() in E:\BA\56\s\src\Microsoft.AspNetCore.OData.Versioning.ApiExplorer\AspNetCore.Mvc.ApiExplorer\ODataApiDescriptionProvider.cs:line 388
   at Microsoft.AspNetCore.Mvc.ApiExplorer.ODataApiDescriptionProvider.OnProvidersExecuted(ApiDescriptionProviderContext context) in E:\BA\56\s\src\Microsoft.AspNetCore.OData.Versioning.ApiExplorer\AspNetCore.Mvc.ApiExplorer\ODataApiDescriptionProvider.cs:line 197
   at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.GetCollection(ActionDescriptorCollection actionDescriptors)
   at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.get_ApiDescriptionGroups()
   at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwagger(String documentName, String host, String basePath, String[] schemes)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context) 	3.75s		

I declare two routes one for access by the users and another one for the services running by other components of the application.
My startup looks like this:

            modelBuilder.DefaultModelConfiguration = (b, v) => UniversityModelBuilder.BuildEdmModel(b, v);
            var models = modelBuilder.GetEdmModels();
            app.UseMvc(builder =>
                        {
                            builder.Select().Expand().Filter().OrderBy().MaxTop(100).Count();
                            builder.SetUrlKeyDelimiter(ODataUrlKeyDelimiter.Slash);
                            builder.MapVersionedODataRoutes("tenant", "tenant/{tenant}", models);
                            builder.MapVersionedODataRoutes("host", "host", models);
                            builder.MapRoute(name: "default",
                                template: "{controller=Documentation}/{action=Index}");
                        });

If I comment the line builder.MapVersionedODataRoutes("host", "host", models); everything works.

The model where I declare my Action is:

if (v == Versiones.V2)
            {
                var sequence = modelBuilder.EntitySet<Secuencia>("Sequence").EntityType;
                sequence.HasKey(x => x.Id);
                sequence
                    .Action("AdvanceSequence")
                    .Returns<SequenceResult>()
                    .Parameter<int>("Ammount").HasDefaultValue("1");
            }

And the controller is:

    [ODataRoutePrefix(nameof(Secuencia)), V2]
    public class SequenceController : ODataController
    {
        private readonly UniveristyContext _db;
        private readonly DbSet<Secuencia> _dbSet;
        private readonly ISequenceService _seqService;

        public SequenceController(UniveristyContext db, ISequenceService seqService)
        {
            _db = db;
            _dbSet = _db.Set<Secuencia>();
            _seqService = seqService;
        }

        [HttpPost]
        public async Task<IActionResult> AdvanceSequence([FromODataUri] string key, ODataActionParameters param)
        {
            int ammount = 0;
            if (param != null)
            {
                param.TryGetValue("Ammount", out object ammountObj);
                ammount = (int)ammountObj;
            }
            return Created(_seqService.GetNumbers(key, (int)ammount));
        }

I tried to debug the problem using the source code and the problem seems to be that the method NewActionParameters is called once for each route declared, and doesn't check if the parameter was defined previously in the moduleBuilder, or use a different key for every mapping

        public Type NewActionParameters( IServiceProvider services, IEdmAction action, ApiVersion apiVersion, string controllerName )
        {
            Arg.NotNull( services, nameof( services ) );
            Arg.NotNull( action, nameof( action ) );
            Arg.NotNull( apiVersion, nameof( apiVersion ) );
            Arg.NotNull( controllerName, nameof( controllerName ) );
            Contract.Ensures( Contract.Result<Type>() != null );

            var name = controllerName + "." + action.FullName() + "Parameters";
            var properties = action.Parameters.Where( p => p.Name != "bindingParameter" ).Select( p => new ClassProperty( services, p, this ) );
            var signature = new ClassSignature( name, properties, apiVersion );
            var moduleBuilder = modules.GetOrAdd( apiVersion, CreateModuleForApiVersion );

            return CreateTypeInfoFromSignature( moduleBuilder, signature );
        }

        static TypeInfo CreateTypeInfoFromSignature( ModuleBuilder moduleBuilder, ClassSignature @class ) => CreateTypeBuilderFromSignature( moduleBuilder, @class ) .GetTypeInfo();

static TypeBuilder CreateTypeBuilderFromSignature( ModuleBuilder moduleBuilder, ClassSignature @class )
        {
            Contract.Requires( moduleBuilder != null );
            Contract.Requires( @class != null );
            Contract.Ensures( Contract.Result<TypeBuilder>() != null );

            var typeBuilder = moduleBuilder.DefineType( @class.Name, TypeAttributes.Class );

            foreach ( var attribute in @class.Attributes )
            {
                typeBuilder.SetCustomAttribute( attribute );
            }
            foreach ( var property in @class.Properties )
            {
                var type = property.Type;
                var name = property.Name;
                var propertyBuilder = AddProperty( typeBuilder, type, name );

                foreach ( var attribute in property.Attributes )
                {
                    propertyBuilder.SetCustomAttribute( attribute );
                }
            }
            return typeBuilder;
        }

Maybe changing this part can fix the issue

static TypeInfo CreateTypeInfoFromSignature( ModuleBuilder moduleBuilder, ClassSignature @class ) =>
            ( moduleBuilder.GetType( @class.Name, false, false ) ?? CreateTypeBuilderFromSignature( moduleBuilder, @class ) ).GetTypeInfo();

[commonsensesoftware]

I'm afraid the real problem here is that OData isn't doing what you want or expect. The prefix is virtually cosmetic and doesn't mean anything to OData. The OData parser doesn't even consider it. OData was never designed to support multiple EDMs or handle duplicate routes. This makes it a challenge to implement API versioning. When you use the MapVersionedODataRoutes extension method, all possible OData routes have to be considered and collated. This is a problem if you're trying to split across prefixes. I believe you should be able to make it work by registering the EDMs one at a time via MapVersionedODataRoute instead. This may lead to other issues or limitations if you're trying use Swagger.

I seen a number of people try to use this approach and run into problems. Not to be overly critical, but there seems to be something inherently wrong with having duplicate entities, actions, etc with different behaviors that only vary by prefix (ex: students/classes vs teachers/classes). Classes should have the same meaning in both contexts, which would also negate the need for separate prefixes. Filtering classes by navigation property makes complete sense.

While you've identified one issues, I'm not sure that is the end of it. Solving this issue may just lead to the next one. If you have a repro you can share, it would be useful. Ultimately, any time that you have a duplicate entity, etc that stripes across the prefix, you may hit this problem or something like it. There may be a solution to it yet, but it's a hard problem to solve. (e.g. there's way more to this than a simple bug fix - unfortunately).

[martindafonte]

Hi, thanks for the quick and comprenhensive response.

My use cases has more to do with permissions and multi-tenancy. Whenever I'm trying to access data from a tenant I navigate using /tenant/{tenant} prefix and has some EF filters that prohibits accessing other tenants data, and when I access with /host it means I'm acting as the platform itself giving me access to all the data.

I guess I could make all the entities related to Tenant and only allow accessing them through tenant, but it presents other problems with multi level nested URL (e.g. /tenants/t1/Students/s1/Courses)

What caught my attention was that if I use a Function instead of an Action all seems to work.
I will try to register one EDM at a time and share my results.

I've included the sample repo I'm using.
SicfePy.OdataSample.zip

edit: I've updated the Repo with some code improvements

[commonsensesoftware]

There's a couple of other possibilities. Putting them in them in the same application is a convenience. One approach is to use completely separate applications. This would be a much better security and isolation boundary. You could then use host headers to give the impression to clients that they are actually talking to the same endpoint when they are, in fact, mapped separately behind the scenes.

I won't pretend to know the particulars or idiosyncrasies of your application, but it certainly feels like there are authentication and authorization mechanisms you can use to identify yourself (or team) as rulers of the universe when you call into the API that would give you access to any tenant.

To be fair, I won't rule out the possibility of a bug. The matrix of usage and combinations is very large and complex. Your repro will be useful in confirming that. It also illuminates a common scenario that may yet be solvable.

Sorry that doesn't give you an immediate fix, but hopefully that moves you in the right direction.

[martindafonte]

Hi, thanks for the pointers, knowing it's not a good way to have more than one OData route I will try to explore other solutions and ways of building this separation.

As a side note, I tried registering the routes one by one, even having Sequence class excluded from one of the prefixes and the problem persists.

app.UseMvc(builder =>
                        {
                            builder.Select().Expand().Filter().OrderBy().MaxTop(100).Count();
                            builder.SetUrlKeyDelimiter(ODataUrlKeyDelimiter.Slash);
                            foreach (var v in Versiones.VERSION_LIST)
                            {
                                builder.MapVersionedODataRoute("tenant" + v, "tenant/{tenant}", BuildCustomEdmModel(v, false, ModelType.Tenant), v);
                                builder.MapVersionedODataRoute("host" + v, "host", BuildCustomEdmModel(v, false, ModelType.Host), v);
                            }

 if (includeAll || (type.HasValue && type.Value == ModelType.Host))
                {
                    var sequence = modelBuilder.EntitySet<Secuencia>("Sequence").EntityType;
                    sequence
                        .Action("AdvanceSequence")
                        .Returns<SequenceResult>()
                        .Parameter<int>("Ammount")
                        .HasDefaultValue("1");
                }

It seems that ApiExplorer is taking the values from the controllers it finds, even if it's not registered in the EdmModel for that versión and prefix.

Best regards

[commonsensesoftware]

That's unfortunate. =/

Every time I run into this situation I feel like it should be solvable, but when I get down in the weeds, I end up hitting a roadblock. There is has to be a reliable solution to this yet!

Thanks for the all the info. Real world configurations and challenges help flush out things out. Crossing my fingers there's a solution to this once and for all. :)

[commonsensesoftware]

I know it's been a while, but I wanted to add some clarifying notes. Is the problem limited to the API Explorer (e.g. generating OpenAPI/Swagger documents) or does the routing to the APIs themselves not work either. In reviewing this, I'm wondering if everything else may work except for the API Explorer stuff. Thanks.

[commonsensesoftware]

I managed to replicate this problem and I believe it's related to #667. You're 100% correct that IModelTypeBuilder.NewActionParameters doesn't not check for previous type generation. I agree that this method should have idempotent behavior. That fix will be made in the next round of changes.

The real issue seems to be a duplicate or otherwise ambiguous names. This should be the only way that NewActionParameters is called multiple times (which is almost certainly why I didn't have this check in there). I have confirmed a separate bug that duplicate names across controllers could cause this.

As an aside, I'm working on a massive refactoring for the OData internals. I've come to the conclusion that IModelConfiguration needs a string routePrefix for you to decide whether you want to define anything for a particular API version and route prefix combination. This means you'll also be getting a new overload for VersionedODataModelBuilder.GetEdmModels(string routePrefix) that will flow through to each IModelConfiguration. This will truly also bucketizing EDM models. This still poses a bit of challenge for the API Explorer. The API Explorer doesn't intrinsically support grouping. There's pseudo-support by way of extension methods for API version. Something similar will be provided for you to filter or do additional grouping by route prefix.

[commonsensesoftware]

Both of these issues are resolved in 5.0.0-RC.1. The official release will likely occur after 2 weeks of burn-in. If you encounter any issues, feel free to reopen the issue. Thanks.