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

refactor: change the way action filter checks custom attribute in controller #3

Merged
merged 4 commits into from
Sep 5, 2023
Merged

refactor: change the way action filter checks custom attribute in controller #3

merged 4 commits into from
Sep 5, 2023

Conversation

alshuriga
Copy link
Contributor

I've changed the way of mvc action filter checking if controller has FluentValidationAutoValidation attribute.
Tested it in WeatherForecast (dotnet webapi template app) and it seems to work fine.

Copy link
Member

@mvdgun mvdgun left a comment

Choose a reason for hiding this comment

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

@alshuriga thanks for your contribution! I am seeing some binary files in your PR, perhaps some artifacts from your IDE? Could you remove those? I've marked those files for you.

@mvdgun
Copy link
Member

mvdgun commented Aug 30, 2023

Looking back at the code I think the annotations on methods will break since it only retrieves annotations on the type. Did you test putting an annotation on a controller method?

@mvdgun mvdgun added enhancement New feature or request and removed enhancement New feature or request labels Aug 30, 2023
fixed action filter would'nt work in case of using FluentValidationAutoValidation attribute on a controller's action
@alshuriga
Copy link
Contributor Author

alshuriga commented Aug 31, 2023

Looking back at the code I think the annotations on methods will break since it only retrieves annotations on the type. Did you test putting an annotation on a controller method?

thank you for noticing that, it works now for both controller and action annotation.

@alshuriga alshuriga requested a review from mvdgun September 4, 2023 15:55
@mvdgun
Copy link
Member

mvdgun commented Sep 4, 2023

@alshuriga I'm still debating whether this change will have a detrimental impact on performance since it uses reflection. Where possible I would like to avoid using reflection. I'm going to look into the EndpointMetadata some more. Maybe .NET offers some public API's that expose the applied attributes.

Change the way action filter checks custom attribute in controller without using reflection
@alshuriga
Copy link
Contributor Author

alshuriga commented Sep 5, 2023

@mvdgun Yes, GetCustomAttribute actually does impact performance if called every request. I found another way to retrieve attributes, it actually seems to work very similar to the original logic, but there's no restrictions in the documentation for using it in application code. Please take a look.

Copy link
Member

@mvdgun mvdgun left a comment

Choose a reason for hiding this comment

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

@alshuriga Looks like a great option! I have added some remarks.

added httpContext endpoint null check in action filter, removed empty lines
@alshuriga alshuriga requested a review from mvdgun September 5, 2023 16:55
@mvdgun
Copy link
Member

mvdgun commented Sep 5, 2023

Closes #4

@mvdgun mvdgun merged commit 1cccc81 into SharpGrip:master Sep 5, 2023
@mvdgun
Copy link
Member

mvdgun commented Sep 5, 2023

Merged, thanks for you contribution @alshuriga!

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