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

Make OpenAPI.NET library trim-compatible #1717

Merged
merged 20 commits into from
Jul 22, 2024

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Jul 4, 2024

This PR removes the use of reflection in GetDisplayName for enums and adds [DynamicallyAccessMembers] attributes to CloneFromCopyConstructor.

Prior to this change, using the Microsoft.OpenApi package in an application with trimming enabled would present the following warnings:

    /_/src/Microsoft.OpenApi/Extensions/EnumExtensions.cs(27): Trim analysis warning IL2075: Microsoft.OpenApi.Extensions.EnumExtensions.GetAttributeOfType<T>(Enum): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.PublicNestedTypes', 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.PublicEvents' in call to 'System.Type.GetMember(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
    /_/src/Microsoft.OpenApi/Any/OpenApiAnyCloneHelper.cs(23): Trim analysis warning IL2075: Microsoft.OpenApi.Any.OpenApiAnyCloneHelper.CloneFromCopyConstructor(IOpenApiAny): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors' in call to 'System.Type.GetConstructors()'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

Closes #1715
Closes #1114

src/Microsoft.OpenApi/Extensions/EnumExtensions.cs Outdated Show resolved Hide resolved
src/Microsoft.OpenApi/Extensions/EnumExtensions.cs Outdated Show resolved Hide resolved
src/Microsoft.OpenApi/Any/OpenApiAnyCloneHelper.cs Outdated Show resolved Hide resolved
src/Microsoft.OpenApi/Extensions/EnumExtensions.cs Outdated Show resolved Hide resolved
src/Microsoft.OpenApi/Models/OpenApiReference.cs Outdated Show resolved Hide resolved
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be interesting to set-up a trimming test project to ensure no regressions happen in the future here.
microsoft/kiota-dotnet#291
microsoft/kiota-dotnet#295
microsoft/kiota-dotnet#296

darrelmiller
darrelmiller previously approved these changes Jul 16, 2024
MaggieKimani1
MaggieKimani1 previously approved these changes Jul 16, 2024
@captainsafia
Copy link
Member Author

Thanks for the reviews @darrelmiller and @MaggieKimani1 -- I spotted a few more issues after adding a TrimmingTest project (mostly related to ValidationRuleset) so we'll need to hold off on merging this a bit longer.

@captainsafia
Copy link
Member Author

@baywet @eerhardt I think this is good for one last review...

baywet
baywet previously approved these changes Jul 19, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this great contribution! And for the ongoing discussion here.

baywet
baywet previously approved these changes Jul 19, 2024
@baywet
Copy link
Member

baywet commented Jul 19, 2024

@MaggieKimani1 for final review and merge

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

MaggieKimani1
MaggieKimani1 previously approved these changes Jul 22, 2024
@MaggieKimani1 MaggieKimani1 dismissed stale reviews from baywet and themself via 6200767 July 22, 2024 08:58
@MaggieKimani1
Copy link
Contributor

LGTM!

@MaggieKimani1 MaggieKimani1 merged commit 1a2877f into microsoft:vnext Jul 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants