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

Disable the DynamicProxy for ConventionalControllers. #18874

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

maliming
Copy link
Member

From #18845 (comment)

Because it becomes an MVC controller, it has global filters.

Resolve #18845

Because it becomes an MVC controller, it has global filters.
Resolve #18845
Copy link

codecov bot commented Jan 27, 2024

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (da59ccb) 51.60% compared to head (2d31fc8) 51.59%.
Report is 6 commits behind head on rel-8.0.

Files Patch % Lines
...e/Volo/Abp/DynamicProxy/DynamicProxyIgnoreTypes.cs 0.00% 10 Missing ⚠️
...Abp/AspNetCore/Mvc/ConventionalController_Tests.cs 0.00% 6 Missing ⚠️
...etCore/VirtualFileSystem/WebContentFileProvider.cs 0.00% 3 Missing and 1 partial ⚠️
...Volo.Abp.Core/Volo/Abp/DynamicProxy/ProxyHelper.cs 0.00% 3 Missing ⚠️
.../Abp/TestApp/Application/ConventionalAppService.cs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           rel-8.0   #18874      +/-   ##
===========================================
- Coverage    51.60%   51.59%   -0.02%     
===========================================
  Files         3077     3079       +2     
  Lines        97270    97302      +32     
  Branches      7746     7747       +1     
===========================================
+ Hits         50200    50202       +2     
- Misses       45524    45549      +25     
- Partials      1546     1551       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maliming maliming requested a review from hikalkan January 28, 2024 01:58
@gizemmutukurt gizemmutukurt requested review from enisn and ismcagdas and removed request for hikalkan February 6, 2024 06:46
@ismcagdas ismcagdas requested review from hikalkan and removed request for enisn and ismcagdas February 6, 2024 07:48
@ismcagdas ismcagdas modified the milestones: 8.0-patch, 8.1-final Feb 6, 2024
@gizemmutukurt gizemmutukurt modified the milestones: 8.1-final, 8.2-preview Feb 6, 2024
@hikalkan
Copy link
Member

@maliming doesn't AppliedCrossCuttingConcerns system already solves the duplication problem?
I am unsure about that change. For example, if we add another interceptor for appservices, we should always also add action filters. Otherwise, the interception logic won't work. Even if we do it for the framework, custom interceptors (added by the application developer) won't work.

@maliming
Copy link
Member Author

We don't apply interceptors for controllers.
https://github.com/abpframework/abp/blob/dev/framework/src/Volo.Abp.Core/Volo/Abp/DynamicProxy/DynamicProxyIgnoreTypes.cs#L7-L14

The Authorization middleware do a check. After that the AuthorizationInterceptor is checked again.

I've only disabled the interceptor for the application service controllers. Because it doesn't inherit from ControllerBase

https://github.com/abpframework/abp/blob/dev/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/AbpAspNetCoreMvcModule.cs#L67

@hikalkan hikalkan merged commit 3e753b3 into rel-8.0 Feb 14, 2024
3 of 5 checks passed
@hikalkan hikalkan deleted the ConventionalControllers branch February 14, 2024 18:06
@hikalkan
Copy link
Member

Thanks for the explanation.

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

Successfully merging this pull request may close these issues.

4 participants