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

Compiler warning CA1839 on DbSet.Add #31431

Open
jamesgurung opened this issue Aug 8, 2023 · 5 comments · Fixed by dotnet/roslyn-analyzers#6858
Open

Compiler warning CA1839 on DbSet.Add #31431

jamesgurung opened this issue Aug 8, 2023 · 5 comments · Fixed by dotnet/roslyn-analyzers#6858

Comments

@jamesgurung
Copy link

Since updating to .NET 8 Preview 7, all calls to DbSet<TEntity>.Add(TEntity entity) and DbSet<TEntity>.AddRange(params TEntity[] entities) result in a CA1839 compiler warning.

For example, calling context.Users.Add(user); results in:

Warning CA1849 'DbSet.Add(User)' synchronously blocks. Await 'DbSet.AddAsync(User, CancellationToken)' instead.

However, the summary for AddAsync reads:

This method is async only to allow special value generators, such as the one used by 'Microsoft.EntityFrameworkCore.Metadata.SqlServerValueGenerationStrategy.SequenceHiLo', to access the database asynchronously. For all other cases the non async method should be used.

Should this warning be suppressed by default?

@Spacefish
Copy link

Did a PR to fix this: dotnet/roslyn-analyzers#6858

@roji
Copy link
Member

roji commented Aug 16, 2023

This is a bit tricky: if the user happens to be using a value generator which does I/O (such as HiLo), then this warning makes sense just like it does for any other .NET API where an async counterpart exists. The problem is that usage of HiLo is rare, and for those cases using Add is fine and the warning is noise. On the other hand, there's no great disadvantage in calling AddAsync instead of Add.

Note #30957 which is the long-term solution here, i.e. obsolete AddAsync altogether.

@roji
Copy link
Member

roji commented Aug 17, 2023

Note: dotnet/roslyn-analyzers#6858 suppresses the diagnostic in the analyzer. We should consider doing this in the EF repo instead via a diagnostics suppressor, in order to eventually remove the Roslyn-side special-casing (though that's useful for now as it also takes care of older EF versions where our suppressor wouldn't be available, thanks @Spacefish).

Unless, of course, we go with #30957 which will obsolete AddAsync altogether.

@virzak
Copy link

virzak commented Sep 9, 2023

So the consensus for most projects is to keep Add since AddAsync might go away. Is that correct?

@ajcvickers
Copy link
Member

@virzak AddAsync isn't going to go away anytime soon. You should use it if your value generator needs to access the database outside of SaveChanges. This is uncommon.

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