Suppress warnings on uninitialized DbSet properties#26795
Conversation
8550edf to
bcffe52
Compare
bcffe52 to
a0b2f87
Compare
|
/cc @Youssef1313 |
| private static readonly SuppressionDescriptor _suppressUninitializedDbSetRule = new( | ||
| id: "SPR1001", | ||
| suppressedDiagnosticId: "CS8618", | ||
| justification: "DbSet properties on DbContext subclasses are automatically populated by the DbContext constructor"); |
There was a problem hiding this comment.
Is this being non-localized intentionally?
There was a problem hiding this comment.
Note that nothing else in EF Core is localized at this point.
It's true we manage exception strings via resx, but those sometimes get reused (or asserted upon in tests). I'd rather extract these strings out based on need rather than proactively, though can do that if the team prefers.
There was a problem hiding this comment.
@roji We have so far taken the approach that everything (possible) is localization ready. I'd prefer we keep doing that, but we can discuss with the team.
test/EFCore.Analyzers.Tests/UninitializedDbSetDiagnosticSuppressorTest.cs
Show resolved
Hide resolved
| static bool InheritsFrom(ITypeSymbol typeSymbol, ITypeSymbol baseTypeSymbol) | ||
| { | ||
| var baseType = typeSymbol.BaseType; | ||
| while (baseType is not null) | ||
| { | ||
| if (baseType.Equals(baseTypeSymbol, SymbolEqualityComparer.Default)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| baseType = baseType.BaseType; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Is it intentional to look at the full hierarchy, not only the parent?
There was a problem hiding this comment.
Yes - the user's DbContext type can be lower down the hierarchy, rather than directly extend DbContext. A good example is ASP.NET Identity, where user context types can extend IdentityDbContext, which itself extends DbContext. The moment DbContext is somewhere in the base type list, we know its constructor will get invoked, and therefore the DbSets will get populated.
9def9d5 to
da6b358
Compare
5acc0e3 to
c5f0ae7
Compare
|
@ajcvickers you may want to give my localization commit a quick look. |
There was a problem hiding this comment.
@roji Curious why this wasn't applied to get-only auto-properties. Feels like
public DbSet<Blog> Blogs { get; } is a very common thing - I've never had a reason to expose a setter on a DB set (it's not even instantiable other than via Set<T>()), and before get-only I would aways define them as private set.
|
@Bretttt because EF Itself doesn't automatically inject read-only DbSet properties with DbSet instances - it can't, since there's no setter. In other words, for writable DbSet properties, the warning is wrong since EF takes care of initialization behind the scenes; but for non-writable ones it does not, and so the warning is correct: you must initialize those properties yourself. Try it out yourself to see this behavior. |
Closes #21608