dotnet / aspnet-api-versioning

Provides a set of libraries which add service API versioning to ASP.NET Web API, OData with ASP.NET Web API, and ASP.NET Core.
MIT License
3.05k stars 703 forks source link

Configuring multiple routes each with their own Edm model throws an error #996

Closed darrenferne closed 1 year ago

darrenferne commented 1 year ago

Is there an existing issue for this?

Describe the bug

I am attempting to configure multiple odata routes, each with its own model, this gives the following error on startup:

The given key 'Asp.Versioning.OData.EdmTypeKey' was not present in the dictionary.

Example: `var builder = WebApplication.CreateBuilder(args);

        // Add services to the container.

        builder.Services.AddControllers();
        // Learn more about configuring Swagger/OpenAPI at https://aka.ms/aspnetcore/swashbuckle
        builder.Services.AddEndpointsApiExplorer();
        builder.Services.AddSwaggerGen();

        builder.Services
            .AddMvc()
            .AddOData();

        builder.Services
            .AddApiVersioning(options =>
            {
                options.DefaultApiVersion = ApiVersion.Default;
                options.AssumeDefaultVersionWhenUnspecified = true;
                options.ReportApiVersions = true;
            })
            .AddOData(options =>
            {
                **options.AddRouteComponents("lettersapi");
                options.AddRouteComponents("wordsapi");

                options.ModelBuilder.DefaultModelConfiguration = (builder, version, route) =>
                {
                    if (route == "lettersapi")
                    {
                        builder.EntitySet<LetterType>("Letter");
                        builder.EntityType<LetterType>()
                               .HasKey(e => e.Id);
                    }
                    else if (route == "wordsapi")
                    {
                        builder.EntitySet<WordType>("Word");
                        builder.EntityType<WordType>()
                               .HasKey(e => e.Id);
                    }
                };**
            })
            .AddODataApiExplorer(options =>
            {
                options.GroupNameFormat = "'v'VVV";
                options.SubstituteApiVersionInUrl = true;
            });

        var app = builder.Build();

        // Configure the HTTP request pipeline.
        if (app.Environment.IsDevelopment())
        {
            app.UseSwagger();
            app.UseSwaggerUI();
        }

        app.UseHttpsRedirection();

        app.UseAuthorization();

        app.MapControllers();

        app.Run();`

Full code attached in Steps to Reproduce

Expected Behavior

I should be able to configure multiple routes each having its own model without issue.

Steps To Reproduce

Asp.Versioning.Test.zip

Repo

Exceptions (if any)

No response

.NET Version

Net7

Anything else?

No response

commonsensesoftware commented 1 year ago

This is indeed a bug. I'm kind of surprised no one has reported it before. The mapping was grouping all possible types by API version because there can only be one actual CLR type definition. If one EDM has it, but another does not, then the expectation that some type exists fails and produces this error. I likely never covered this situation intentionally because it leads to a second problem.

If you configure two different EDMs using the same CLR type, but configured in different ways, this will result in a different, but related error. OpenAPI (formerly Swagger) does not allow/support multiple models with the same type name, but it's not a problem for OData. There is a workaround. If an alternate entity name is specified for the EDM type, then that type name can be used instead of the original CLR type, which is the default. This is a very obscure use case, but it would be possible to support.

I'm working on a fix. In the meantime, you can unblock yourself by moving the prefixes into the controller routes rather than using OData prefixes. This will result in a single EDM instead of two. If you were directly exposing and using the metadata endpoint (e.g. $metadata) for client generation, your use case would make more sense. Since you are using OpenAPI, the notion of EDM is erased. From the documentation there is no notion or separation of EDM. The route prefix could be on the controller or the whole EDM, but it's transparent to the client.

You can create configurations that vary by API version and route prefix, but it's not something I recommend. I would instead suggest using a single prefix or no prefix at all. The choice is ultimately up to you. Stay tuned for the fix.

commonsensesoftware commented 1 year ago

Apologies for the delay in publishing, but the updated packages have been released and should address your issue. Let me know if you run into anything else.

darrenferne commented 1 year ago

Thanks for the quick turn around. It has fixed the reported issue but now I have a separate issue. For each of the endpoints we expose we also expose a companion endpoint to expose extended metadata. Each of these endpoints has it's own route and EDM but the types used by each are the same. I thought this might be related to the second issue you mentioned but I'm not sure. I've updated the example project with a very crude example. The strange thing is that when I put this example together everything worked fine. It was only when I changed the model builder to use the non convention builder that I hit the problem. We build the EDM per type so we have more control over what's included which makes me think we've missed something but I've yet to track down what.

darrenferne commented 1 year ago

Update. After a comparison of the objects created using the two builders I don't think the issue stems from the way we create the Edm. Both objects appear to match. What it does seem to be down to is ignored properties. If the emd contains an ignored property then it fails. I think this brings us back to the related issue you mention. I've updated the project to show the issue.

commonsensesoftware commented 1 year ago

This is the second issue. It's really more of an issue at the OpenAPI level as opposed to OData or API Versioning. I should state (or restate) that OData prefixes are not a level of isolation. The EDM-per-route prefix mapping is a consequence of how OData's configuration works, but there is no separation. You are free to configure your app this way, but understand that everything is scope.

The crux of the problem is that you have one type - Metadata - that appears in multiple APIs with the same API version, but different prefixes. This leads to multiple types of the same name being emitted for documentation. OpenAPI does not allow more than one model of the same name in the document. There are several ways around this, but the easiest are:

  1. Use different types
  2. Use different type names

You don't seem to want to use different types (which is OK), so you'd need to change your configuration like so:

var entity = builder.EntityType<Metadata>();

entity.Name = "LetterMetadata";
entity.HasKey(e => e.TypeName).Ignore(p => p.DisplayName);

and repeat it for wordsapi too. This will return in 3 distinct types:

Even though they look, feel, and behave the same, they are distinct types. Making these changes worked as expected. This is an inconvenience on the client side, but there isn't much more code or maintenance added.

An alternative approach is to not use the OData-specific concept of a route prefix. You can define route prefixes in a generic way, which would produce a single type. Since you're defining Metadata as an entity set, OData might not be happy with that; however, a complex type may work.

darrenferne commented 1 year ago

I can understand why having multiple different configurations for the same type could cause an issue but what's confusing me is why it fails with multiple matching configurations. If I create the 3 edms without ignoring any of the properties then everything is fine. I get one metadata type in my documentation. If I just expose one of the metadata endpoints from my example then this is OK as well. The substitution of the model correctly filters out the excluded property. I can understand that different configurations would require different substitutions and that would cause a naming clash but if the configurations are the same then only a single substitution should be required and there should be no naming clash.

The production code I'm working on is a lot more complicated than the example. We do not use standalone controllers, they're all based on a generic base controller and created/loaded based on our own metadata model. This is why there's a single md type shared across each set of eps. The routes are configured according to product/functional areas so were my example on has 2 a production solution could have a lot more. While it's possible, it will get very messy if I have to start prefixing the metadata type per route.

The problem I have now is that my previous solution no longer works either. I was sharing the model builder across the endpoints so everything went in a single edm. It wasn't ideal but it was functional.

commonsensesoftware commented 1 year ago

Substitution has the following rules:

  1. If the CLR type is exactly the same as the EDM type, then no substitution occurs
  2. When substitution occurs it (now), it creates a surrogate type based on EDM + API version + route prefix

Once the substitution path is taken, there is no attempt or support to reuse a substituted type across EDM + API Version. Before the change, route prefixes were not considered; however, there is a valid use case to use the same CLR type with a different EDM, API version, and - potentially - route prefix combination.

A big part of the reason it works that way is performance. Reflecting over CLR types and doing comparisons is expensive and could happen on every HTTP request. This process is done just once as result. Once an EDM and CLR type have evaluated for a particular API version and route prefix combination, the cached result is used.

Adding the type of support you want is possible, but it's not clear off the top of my head if you can extend the DefaultModelTypeBuilder to suit your needs or whether you need to create a custom IModelTypeBuilder. At least for OpenAPI documents, EDM and route prefix are irrelevant. The name of the model must be unique across all APIs in the document, which is typically generated per API version.