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 S2971 message to use AsEnumerable in LINQ database query #3604

Closed
mitchello opened this issue Sep 14, 2020 · 5 comments · Fixed by #8593
Closed

Improve S2971 message to use AsEnumerable in LINQ database query #3604

mitchello opened this issue Sep 14, 2020 · 5 comments · Fixed by #8593
Assignees
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: Improvement Making existing code better.
Milestone

Comments

@mitchello
Copy link

mitchello commented Sep 14, 2020

Description

S2971

When calling ToList or ToArray in the middle of a LINQ chain, the rule raised, which is correct in a way but there is an exception that should be managed. When using Entity Framework Core (3.x), it's recommended to manage data on client side as some features can't be parse into SQL.
See: https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-3.x/breaking-changes#linq-queries-are-no-longer-evaluated-on-the-client

Repro steps

Using EF Core:
return portLinq.OrderBy(v => v.Date).ToList().Select(x => new HistoricalValue(x.Date, x.Value)).ToList();

If we don't retrieve data on client side with the ToList in the middle of the chain, the Select will throw an exception at runtime as it's impossible for EF Core to parse what's in the Select statement.

Expected behavior

Rule shouldn't be raised

Actual behavior

Rule raised

@pavel-mikula-sonarsource pavel-mikula-sonarsource added this to background tasks (investigate, clarify) in Best Kanban Sep 14, 2020
@pavel-mikula-sonarsource pavel-mikula-sonarsource added the Status: On Hold Postponed or waiting for an answer. label Sep 21, 2020
@pavel-mikula-sonarsource
Copy link
Contributor

Hi @mitchello ,

Can you please provide some small, but complete reproducer for this case? I tried to reproduce it with similar select-to-local-class but EF 3.1.8 handled that without an exception.

@mitchello
Copy link
Author

Hi @pavel-mikula-sonarsource , you will find a small project reproducing our issue.

DemoEFCore.zip

Here's some more information regarding this behavior : https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-3.x/#linq-overhaul

@andrei-epure-sonarsource andrei-epure-sonarsource removed the Status: On Hold Postponed or waiting for an answer. label Nov 2, 2020
@pavel-mikula-sonarsource
Copy link
Contributor

Hi @mitchello,

Thank you for the reproducer project. I'm able to reproduce the issue now. While looking at the example, you don't really need .ToList() in the middle of the chain. Using .AsEnumerable() is more appropriate alternative.

So the intention and core implementation of the rule is correct. I've updated the rule specification with note about this specific case we'll try to improve the message in this case to suggest AsEnumerable instead in the future.

@pavel-mikula-sonarsource pavel-mikula-sonarsource changed the title "IEnumerable" LINQs should be simplified rule shouldn't be raised when ToArray or ToList in the middle of a chain Improve S2971 message for EF to use AsEnumerable Jan 4, 2021
@pavel-mikula-sonarsource pavel-mikula-sonarsource added Area: C# C# rules related issues. Type: Improvement Making existing code better. labels Jan 4, 2021
@pavel-mikula-sonarsource pavel-mikula-sonarsource removed this from background tasks (investigate, clarify) in Best Kanban Jan 4, 2021
@pavel-mikula-sonarsource pavel-mikula-sonarsource added this to New features and Improvements in Backlog Jan 4, 2021
@mitchello
Copy link
Author

Hi @pavel-mikula-sonarsource,
Thank you for your answer.

Regards

@pavel-mikula-sonarsource
Copy link
Contributor

ToDo:

  • Detect Linq sources of type DbSet<T>
  • Update message to suggest AsEnumerable for first ToArray occurance in the middle of the chain.

@antonioaversa antonioaversa added the Sprint: Hardening Fix FPs/FNs/improvements label Jan 22, 2024
@antonioaversa antonioaversa added this to the 9.18 milestone Jan 22, 2024
@antonioaversa antonioaversa added this to To do in Best Kanban Jan 22, 2024
@sebastien-marichal sebastien-marichal moved this from To do to In progress in Best Kanban Jan 23, 2024
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Jan 24, 2024
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Jan 24, 2024
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Jan 25, 2024
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Jan 25, 2024
@sebastien-marichal sebastien-marichal changed the title Improve S2971 message for EF to use AsEnumerable Improve S2971 message to use AsEnumerable in LINQ database query Jan 25, 2024
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Jan 25, 2024
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Jan 25, 2024
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Jan 25, 2024
@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban Jan 25, 2024
Best Kanban automation moved this from Review approved to Validate Peach Jan 25, 2024
@sebastien-marichal sebastien-marichal moved this from Validate Peach to Done in Best Kanban Jan 26, 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
Backlog
  
Improvement
Development

Successfully merging a pull request may close this issue.

6 participants