No Reflection

[Youssef1313]

{PR title}

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

{Detail}

Fixes #{bug number} (in this specific format)

[Youssef1313]

Hi @commonsensesoftware

This branch builds using a locally-built OpenApi assembly from the changes in dotnet/aspnetcore#66692.

The PR above makes three important changes:

• Allows AddOpenApi to be called with null document name - which means "register only core services"
• Adds a public IAdditionalOpenApiDocumentNameProvider interface that you can register to provide the document names.
• Uses AnyKey for most service registration. This is what gives the flexibility of not needing the document names early when building the host, and allows keyed service retrieval to work for any requested document. At that time, we can then validate the document names as the information will be available then.

Can you please double check that everything works correctly with those changes?

Once you confirm that everything works, I'll create a proposal in aspnetcore repo that replaces dotnet/aspnetcore#66408 and review it with the team.

I might as well tweak some stuff in the current shape I have. For example, I would like the proposed public interface to be not only responsible for the "additional" document names, but basically provides all the document names. That means that we can remove GetDocumentNames from IDocumentProvider. This will also clean up part of our code here.

[Youssef1313]

Ping @commonsensesoftware.

It would be good if you can test this soon so we have enough time to bring it in for .NET 11.

[Youssef1313]

Ping @commonsensesoftware

[commonsensesoftware]

@Youssef1313 I was under the weather this past weekend. I'll put eyes on this tonight.

FWIW, if you're considering changing the design, I would recommend seriously re-evaluating the idea of key services. It's the wrong approach IMHO. Keyed services have a place, but they are best used for a discrete, fixed set of services that would otherwise have ambiguity. That is not the case here. I don't have that much skin the decision, but that would my recommendation if changing parts of the design are on the table.

[commonsensesoftware]

This looks great and gets rid of a lot of ceremony. AFAIK, the OpenAPI examples work as expected.

Open questions:

1. Does this mean .NET 10 servicing is off the table? I just need to know so I can settle on the yucky Reflection approach and move the release out of RC
2. Are there still thoughts or plans around the XML Comment support? Honestly, I think this is common problem that could benefit other Microsoft teams as well as the community. I would imagine this as a new library and probably not in the BCL. I'm sure there's folks in the VB.NET community that would be happy.
3. I tried applying the same configuration and setup to the OpenApiExample that uses controllers, but it will not build. It fails with Property or indexer 'IOpenApiMediaType.Example' cannot be assigned to -- it is read only. Is that expected? This is coming from the source generator. There are two such errors in OpenApiXmlCommentSupport.generated.cs.

Feedback:

1. I'm still not convinced that keyed services are the right choice. Based on these changes, it appears that it would be orthogonal to me. It's more of a cautionary tale and if your touching things or otherwise changing the internal design, now might be the time to consider rethinking it.
2. The services.AddOpenApi(documentName: null); is a bit odd. I'm not sure if that was for illustrative purposes or strictly necessary. Explicitly using null to show how it would work is fine, but having null drive a specific behavior just feels wrong. If it is strictly necessary and it is otherwise ambiguous with AddOpenApi, consider a new overload that passes the behavior you expect/need; perhaps a new overload or additional to the options.

[commonsensesoftware]

nit: the field and constructor can be eliminated by using a primary constructor

[commonsensesoftware]

This is the only thing I really have feedback on. I presume this is required because of the source code generator for comments? 🤔 Do you have more ideas here? This seems wonky and likely to be confusing to devs. Does this also work as services.AddOpenApi(null);? I suspect one of the first things people will ask or expect is why they can't just use services.AddOpenApi();. Maybe they can. Perhaps it doesn't matter a ton since it will be behind the API Versioning DI extensions, but it does seem odd nevertheless.

The current built-in support for XML Comments relies looking at user code. Did you have thoughts on that? If this is buried inside the API Versioning DI extensions, it will never be visible or supportable. I realize it's a bit tangential to this first step, but I don't really want to provide my own XML Comment implementation. I'm only doing it now because there's no other choice. It's non-blocking, but if there is going to continue to be a relationship between the source code generator (alone) and inspecting the user's source, I thought it was worth mentioning.

[Youssef1313]

services.AddOpenApi() today has a meaning. It's equivalent to services.AddOpenApi("v1"). Changing that is a breaking change.

An alternative here is to have services.AddOpenApiCore().

For Xml, that needs to be its own discussion. We have already some open issues asking for improvements for similar scenarios.

[halter73]

I agree that we should avoid making a breaking change to AddOpenApi(). I don't mind AddOpenApiCore(). We have quite a few other "Core" IServiceCollection extension methods (e.g. AddAuthenticationCore()), so it's not without precedent.

I'm open to alternatives though.

[commonsensesoftware]

I can certainly get onboard with AddOpenApiCore.

It wasn't clear if breaking changes were off the table. The way I kind of imagined the way it would work is not dramatically dissimilar to changing the way OpenApiOptions work by supporting multiple documents. I would have just extended this class if it hadn't been sealed. This could have retained the surface area backward compatibility a la services.AddOpenApi(OpenApiOptions.Default) or something like that. This would still require pretty significant changes to OpenApiOptions.

Ultimately, you guys own it and have the final say. I don't have super strong opinion about your choice other than to say that AddOpenApi(documentName: null) looks strange and is very unintuitive that it will lead to multiple doucments. It sounds like AddOpenApiCore is the happy middle ground.

[commonsensesoftware]

🥲

[commonsensesoftware]

It's not really relevant to this specific review, but if this is the path we settle on, then this should probably also be marked as [Obsolete] with a message that indicates will is no longer needed and it will be removed in .NET 12 (the next LTS).

[Youssef1313]

Sure.

[Youssef1313]

Does this mean .NET 10 servicing is off the table? I just need to know so I can settle on the yucky Reflection approach and move the release out of RC

I have doubts it will meet the bar to make those changes for .NET 10.

I tried applying the same configuration and setup to the OpenApiExample that uses controllers, but it will not build. It fails with Property or indexer 'IOpenApiMediaType.Example' cannot be assigned to -- it is read only. Is that expected? This is coming from the source generator. There are two such errors in OpenApiXmlCommentSupport.generated.cs.

See the comment in "MinimalOpenApiExample.csproj" that's above Microsoft.AspNetCore.OpenApi. I think if you explicitly add the NuGet package (or alternatively create MSBuild target to drop the source generator), it will be resolved.

I'm still not convinced that keyed services are the right choice. Based on these changes, it appears that it would be orthogonal to me. It's more of a cautionary tale and if your touching things or otherwise changing the internal design, now might be the time to consider rethinking it.

That sounds more like implementation detail to me. But I'm happy if you have suggestions for a better implementation. Finalizing the public API design is more important though (unless you have ideas that would impact the public API design). Note that we definitely don't want to break backward compatibility here.

[commonsensesoftware]

I have doubts it will meet the bar to make those changes for .NET 10.

Not a problem. I've been holding of on promoting to full release in the event some changes would make it through. Since it's not, I'll move forward as is. There was a reasonable suggestion on the issue with respect to minimizing the Reflection issues. It's honestly not that bad and the real issue is the chance of things breaking from future changes in an unexpected way.

See the comment in "MinimalOpenApiExample.csproj" that's above Microsoft.AspNetCore.OpenApi. I think if you explicitly add the NuGet package (or alternatively create MSBuild target to drop the source generator), it will be resolved.

Gotcha. I just wanted to verify that path had, in fact, been tested and verified.

That sounds more like implementation detail to me. But I'm happy if you have suggestions for a better implementation. Finalizing the public API design is more important though (unless you have ideas that would impact the public API design). Note that we definitely don't want to break backward compatibility here.

💯 Fundamentally, you just need a list or map of document name to options. The name comes from the route and it can simply flow through. You don't have to use a keyed service. This is what you would have done before keyed services and it would have worked just fine. It's definitely an implementation detail and your call if you want to change it. Changing it now might prevent future, related issues, but I don't have a crystal ball. Given the current requirements and features, it's definitely going to work as is.