search

microsoft / OpenAPI.NET

The OpenAPI.NET SDK contains a useful object model for OpenAPI documents in .NET along with common serializers to extract raw OpenAPI JSON and YAML documents from the model.
MIT License
1.36k stars 226 forks source link

Avoid use of unbounded reflection to resolve Enum display names #1715

Open captainsafia opened 6 days ago

captainsafia commented 6 days ago

In a few places in the OpenAPI.NET codebase, the GetDisplayName method is used to resolve a prettified display name associated with an enum value from the [Display] attribute.

https://github.com/microsoft/OpenAPI.NET/blob/3d971dea96e1d531f395d808743b0d1e863c1b23/src/Microsoft.OpenApi/Extensions/EnumExtensions.cs#L40

The use of unbounded reflection in this codepath makes it difficult to enable trimming for applications that use Microsoft.OpenApi in any capacity.

https://github.com/microsoft/OpenAPI.NET/blob/3d971dea96e1d531f395d808743b0d1e863c1b23/src/Microsoft.OpenApi/Extensions/EnumExtensions.cs#L24

Given the limited set of enums used in the application, I'd recommend an alternative reflection-free approach for resolving display names associated with enums. Something like a Enum <> string cache can be used to resolve this.

Assuming folks are OK with this approach, I'd be happy to submit a PR for review.

cc: @MaggieKimani1 @baywet

baywet commented 5 days ago

Thanks for bringing this up. Do you think we could simply add the AOT attributes (DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields) and whatnot ) and call it a day? This way the trimming would know not to trim the things we need here and we wouldn't have to implement a registry mechanism.

captainsafia commented 5 days ago

Yep, that's an option. It does get a little bit interested since we're targeting netstandard2.0. The DynamicallyAccessedMembers aren't included for that target so you'll have to source include them yourself. There's guidance on how to do this in this blog post.

So our two approaches would be:

The first seemed cleaner to me, especially given that there is a finite set enums and I don't expect them to change much (as long as we are targeting OpenAPI v3.x)

captainsafia commented 5 days ago

While digging into implementing this, I realized that the CloneFromCopyConstructor implementation in the library also presents a challenge for us making it fully trim compatible.

I believe that the annotations referenced above will help with this but I have to give it a try.

baywet commented 5 days ago

In the kiota libraries, we've used the first option from the blog post which is using if definitions. This has been successful for us, it just makes the code a little bit more complicated to read.

If we decided to go with a registration option instead, I like to see what would the developer experience be before we start doing any work.

captainsafia commented 1 day ago

In the kiota libraries, we've used the first option from the blog post which is using if definitions. This has been successful for us, it just makes the code a little bit more complicated to read.

This approach would require that we add targets for net8+ place to the Microsoft.OpenApi library. It's feasible, but I think it's a bit more expensive implementation wise then figuring out a way to get rid of the reflection altogether.

If we decided to go with a registration option instead, I like to see what would the developer experience be before we start doing any work.

Would love your thoughts on https://github.com/microsoft/OpenAPI.NET/pull/1717. I tried to address the trim warnings with a focus on source/binary compat for end users.