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

NotImplementedException is used in wrong meaning in EFCore DbSet class #20961

Closed
jinek opened this issue May 14, 2020 · 5 comments · Fixed by #23502
Closed

NotImplementedException is used in wrong meaning in EFCore DbSet class #20961

jinek opened this issue May 14, 2020 · 5 comments · Fixed by #23502
Labels
area-dbcontext closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@jinek
Copy link

jinek commented May 14, 2020

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.

@ajcvickers
Copy link
Member

@jinek Can you elaborate on "Wrong usage of NotImplementedException leads to wrong handling of exceptions in user-level code?"

@jinek
Copy link
Author

jinek commented May 14, 2020

@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:

switch(exception)
{
case CommunicationException: MessageBox("Can not reach the server");break;
case OutOfMemoryException: MessageBox("Not enough memory, please, try to close some applications"); LogForDeveloper(exception); break;
case SqlException: MessageBox("We have an error, please, restart the application"); LogForAdministrator(exception); break;
case **NotImplementedException**:MessageBox("We have an error, please, AVOID USING REQUESTED FEATURE until we roll out next release"); LogForDeveloper(exception) break;
}

If we mix exceptions, we have less possibilities to rely on exception type.

@ajcvickers
Copy link
Member

@jinek Thanks. We'll follow up on the expected usages of NotImplementedException.

@ajcvickers
Copy link
Member

Note for team: From https://docs.microsoft.com/en-us/dotnet/api/system.notimplementedexception?view=netcore-3.1

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."

@roji
Copy link
Member

roji commented May 15, 2020

FWIW in the same vein, some IDEs mark NotImplementedException in the same way as TODO.

@ajcvickers ajcvickers self-assigned this May 15, 2020
@ajcvickers ajcvickers added this to the Backlog milestone May 15, 2020
@ajcvickers ajcvickers modified the milestones: Backlog, MQ Sep 16, 2020
ajcvickers added a commit that referenced this issue Nov 26, 2020
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.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 26, 2020
@ajcvickers ajcvickers modified the milestones: MQ, 6.0.0 Nov 26, 2020
ajcvickers added a commit that referenced this issue Dec 18, 2020
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.
ajcvickers added a commit that referenced this issue Dec 20, 2020
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.
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview1 Jan 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview1, 6.0.0 Nov 8, 2021
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dbcontext closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants