Skip to content
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

Add synonym to AddMvc method. #1089

Open
voroninp opened this issue May 1, 2024 · 7 comments
Open

Add synonym to AddMvc method. #1089

voroninp opened this issue May 1, 2024 · 7 comments
Assignees

Comments

@voroninp
Copy link

voroninp commented May 1, 2024

Background and Motivation

Extension method which operate on IServiceCollection type are usually chainable.
When I saw examples it was not immeidately obvious that AddMvc is not the same from Asp.Net Core:

public static IApiVersioningBuilder AddMvc(this IApiVersioningBuilder builder)

vs

public static IMvcBuilder AddMvc(this IServiceCollection services)

It took me a while to understand why versioning was not working for controllers.

Proposed API

My proposal is to add alternative method named WithMvcSupport() which, IMO, will cause less confusion and will be more descriptive. (Also WithApiExplorerSupport)

Usage Examples

services.AddApiVersioning(cfg => { ...}).WithMvcSupport().WithApiExplorerSupport();

Risks

If this proposal is accepted, the interface will epand, so is the need to support it. Maybe old methods can be deprecated with time.

@commonsensesoftware
Copy link
Collaborator

This is intentional and by design. The new approach was outlined in the roadmap 2 years ago. The full gambit is:

services.AddApiVersioning()     // Core API Versioning services with support for Minimal APIs
        .AddMvc()               // API version-aware extensions for MVC Core
        .AddApiExplorer()       // API version-aware API Explorer extensions
        .AddOData()             // API versioning extensions for OData
        .AddODataApiExplorer(); // API version-aware API Explorer extensions for OData

Existing users have been thrown off a bit because AddMvc wasn't necessary in the past, but that's because Minimal APIs didn't exist nor were supported.

Only some DI extensions are directly chainable. As you've even shown above, IServiceCollection.AddMvc doesn't return IServiceCollection, it returns IMvcBuilder. AddMvcCore returns IMvcCoreBuilder. IApiVersioningBuilder works in a similar fashion. All of these builders provide IServiceCollection Services { get; }.

If you wanted to make one big chain, you still can with:

builder.Services.AddApiVersioning().AddMvc().Services.AddSwaggerGen();

There were several motivations behind this change:

  1. Remove temporal registration issues; for example: calling services.AddVersionedApiExplorer() before services.AddApiVersioning()
  2. Use simpler names: services.AddApiVersioning().AddApiExplorer() vs services.AddVersionedApiExplorer()
  3. Centralize all things related to API Versioning against the IApiVersioningBuilder
  4. Enable extenders to hook into the builder pipeline with their own customizations and services

In addition to the roadmap, this information has been conveyed in:

I've gone to great lengths to try and make it concise, but still obvious. There are only two scenarios that seem to consistently hang people up:

  1. Forgetting to call IApiVersioningBuilder.AddMvc with controllers, which I don't have a better solution for
  2. Thinking that .AddMvc adds the full MVC stack versus MVC Core (which is silly for APIs so I thought it was obvious; in hindsight perhaps AddMvcCore would have been better)

The convention for adding services, even via a builder, is with the AddXXX prefix. You can see that even IApiVersioningBuilder.AddApiExplorer has to call through to IServicesCollection.AddMvcCore → IMvcCoreBuilder → IMvcCoreBuilder.AddApiExplorer.

At this point, I'm not really inclined to change things. There hasn't been an outcry from the community. I'm still willing to entertain the idea however. I will leave the issue open and if there is a significant upvote, I'll consider it. Aliases are nice, but they also have the risk of causing confusion and changing them will result in a breaking change - at some point (there's no point in having both - forever).

@voroninp
Copy link
Author

voroninp commented May 2, 2024

Then I'd propose to emphasize in wiki and examples that these methods are not the standard one from the framework.

@commonsensesoftware
Copy link
Collaborator

In the wiki, there is already the text:

.AddMvc(); // ← brings in MVC Core; unnecessary for Minimal APIs

There's even an entire page dedicated to migration that convers all of the changes that came in 6.0 that should help people moving from Microsoft.AspNetCore.Mvc.Versioning to Asp.Versioning.Mvc.

Perhaps more text in the examples. The examples are setup to boil things down to just the bare minimum API Versioning configuration with comments and options highlighted for the common knobs that someone might want to turn. The example itself is kind of the documentation. IMHO, it should be obvious that everything shown is working and required. Turning a knob is one thing, but removing a knob, such as removing the call to .AddMvc(), should be expected to break things.

The simplified setup for WebApplication removes the necessity for AddMvcCore and AddMvc. That should also be a signal that it's different. It would probably be more obvious if they were still side-by-side:

builder.Services.AddMvcCore();
builder.Services.AddApiVersioning().AddMvc();

This is actually the case with OData, which requires:

builder.Services.AddOData();
builder.Services.AddApiVersioning().AddAddOData();

@voroninp
Copy link
Author

voroninp commented May 2, 2024

Let me explain how it was for me.

The last time I used your package was before minimal APIs.
For minimal APIs to work you also do not need to call standard AddMvc() or AddMvcCore(). So when I read that comment, my thought was: "Thank you, I know." 😂

My code was:

services.AddMvc().AddApplicationPart(...);

Then I added versioning:

services.AddMvc().AddApplicationPart(...);
services.AddApiVersioning();

MVC added? Check.
Versioning added? Check.

;-)

@commonsensesoftware
Copy link
Collaborator

Yep, I get the surprise and there was no great way to fully advertise that. Behavioral changes always concern me the most because they are the least obvious and hardest to find. Unfortunately, they aren't always fully avoidable.

It's been 2+ years, so I could update the announcement banner to reflect a migration notice and like to the appropriate wiki topic instead of just an announcement. Would that have - perhaps - made things slightly more obvious? That's pretty quick and easy change.

@voroninp
Copy link
Author

voroninp commented May 2, 2024

I guess so.

But you know the curse of knowledge. :-) It's always hard to predict how those who are unaware can be surprised:-)

@commonsensesoftware
Copy link
Collaborator

POLA is part of every decision. Unfortunately, it's just not always easy to advertise changes.

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