-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
NotImplementedException is used in wrong meaning in EFCore DbSet class #20961
Comments
@jinek Can you elaborate on "Wrong usage of NotImplementedException leads to wrong handling of exceptions in user-level code?" |
@ajcvickers In general, we can consider situation of throwing SqlException or NullReferenceException instead of this NotImplementedException. It would lead to similar, but, probably, bigger problems. Some exception types, like OutOfMemoryException,CommunicationException, SqlException or in our case NotImplementedException has special meaning, which can be used to handle an exception in a special way. For example:
If we mix exceptions, we have less possibilities to rely on exception type. |
@jinek Thanks. We'll follow up on the expected usages of NotImplementedException. |
Note for team: From https://docs.microsoft.com/en-us/dotnet/api/system.notimplementedexception?view=netcore-3.1
|
FWIW in the same vein, some IDEs mark NotImplementedException in the same way as TODO. |
Fixes #20961 > You might choose to throw a NotImplementedException exception in properties or methods in your own types when the that member is still in development and will only later be implemented in production code. In other words, a NotImplementedException exception should be synonymous with "still in development." I left one NotImplementedException in Migration.cs, because it really is there for when the application hasn't implemented Down yet, but tries to do a Down.
Fixes #20961 > You might choose to throw a NotImplementedException exception in properties or methods in your own types when the that member is still in development and will only later be implemented in production code. In other words, a NotImplementedException exception should be synonymous with "still in development." I left one NotImplementedException in Migration.cs, because it really is there for when the application hasn't implemented Down yet, but tries to do a Down.
Fixes #20961 > You might choose to throw a NotImplementedException exception in properties or methods in your own types when the that member is still in development and will only later be implemented in production code. In other words, a NotImplementedException exception should be synonymous with "still in development." I left one NotImplementedException in Migration.cs, because it really is there for when the application hasn't implemented Down yet, but tries to do a Down.
Methods, of abstract class DbSet are throwing NotImplementedException, which means that this code needs to be rewritten/implemented: https://docs.microsoft.com/en-us/dotnet/api/system.notimplementedexception?view=netcore-3.1
Wrong usage of NotImplementedException leads to wrong handling of exceptions in user-level code.
More appropriate exception type can be used instead: NotSupportedException, InvalidOperationException etc. Or, probably, new exception type can be implemented for DbSet purposes.
The text was updated successfully, but these errors were encountered: