Skip to content

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

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

Closed
captainsafia opened this issue Jul 3, 2024 · 5 comments · Fixed by #1717
Closed

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

captainsafia opened this issue Jul 3, 2024 · 5 comments · Fixed by #1717

Comments

@captainsafia
Copy link
Member

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.

public static string GetDisplayName(this Enum enumValue)

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

public static T GetAttributeOfType<T>(this Enum enumValue) where T : Attribute

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
Copy link
Member

baywet commented Jul 3, 2024

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
Copy link
Member Author

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:

  • Remove reflection altogether and rely on a cached lookup.
  • Keep reflection, explicitly define the [DynamicallyAccessedMembers] attributes in the source for OpenAPI.NET, and apply them to the relevant codepaths.

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
Copy link
Member Author

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
Copy link
Member

baywet commented Jul 4, 2024

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
Copy link
Member Author

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 #1717. I tried to address the trim warnings with a focus on source/binary compat for end users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants