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

Improve AspNetMvcHelper.ReferencesControllers method: add parameter for .NET Core #9251

Closed
zsolt-kolbay-sonarsource opened this issue May 3, 2024 · 1 comment · Fixed by #9309
Assignees
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: Improvement Making existing code better.
Projects
Milestone

Comments

@zsolt-kolbay-sonarsource
Copy link
Contributor

zsolt-kolbay-sonarsource commented May 3, 2024

Currently the AspNetMvcHelper.ReferencesController() method returns true if the analyzed project references ASP.NET MVC Core or ASP.NET MVC (.NET Framework).
Several rules misuse this method to check whether the analyzer should be registered for the given project.
e.g. AnnotateApiActionsWithHttpVerb is only implemented for ASP.NET Core, but it uses this method to check if the analyzer should be used on the project or not. If the project uses ASP.NET MVC controllers under the old .NET Framework then it will still (needlessly) analyze it, but it won't report anything, because the rule only works with types from ASP.NET Core.

TODO

Add a bool parameter to the method: ReferencesController(bool onlyAspNetCore). When set to true it will only return true if the project references ASP.NET Core MVC Controllers. When the parameter is set to false, it will return true if the project references either ASP.NET Core MVC Controller or the classical .NET Framework MVC Controllers.
Then update all ASP.NET rules to use it correctly: if the rule is only implemented for ASP.NET Core then call ReferencesController(onlyAspNetCore: true)

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource added Type: Improvement Making existing code better. Area: C# C# rules related issues. labels May 3, 2024
@antonioaversa
Copy link
Contributor

Alternative proposal: having two distinct methods, one for core (e.g. referencesCoreControllers) and one for framework (e.g. referencesFrameworkControllers).
We would have very few rules targeting ASP.NET 4.x in addition to ASP.NET Core, so where something like if (referencesCoreControllers() && referencesFrameworkController()) would be required.
I think it would be more explicit and simpler to read than a flag passed as input parameter.

@mary-georgiou-sonarsource mary-georgiou-sonarsource added the Sprint: Hardening Fix FPs/FNs/improvements label May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added this to the 9.26 milestone May 21, 2024
@github-actions github-actions bot moved this from To do to Review in progress in Best Kanban May 21, 2024
@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban May 22, 2024
Best Kanban automation moved this from Review approved to Validate Peach May 22, 2024
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource moved this from Validate Peach to Done in Best Kanban May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: Improvement Making existing code better.
Projects
Best Kanban
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants