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

Add virtual methods to existing interceptor base classes #28516

Closed
frandazzo opened this issue Jul 26, 2022 · 11 comments · Fixed by #28542
Closed

Add virtual methods to existing interceptor base classes #28516

frandazzo opened this issue Jul 26, 2022 · 11 comments · Fixed by #28542
Labels
area-interception 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

@frandazzo
Copy link

frandazzo commented Jul 26, 2022

In the code snippet below is not possible to override the ConnectionCreating method.

public class ConnectionInterceptor:DbConnectionInterceptor
{
    public override InterceptionResult<DbConnection> ConnectionCreating(ConnectionCreatingEventData eventData,
        InterceptionResult<DbConnection> result)
    {
        return InterceptionResult<DbConnection>.SuppressWithResult(new 
           SqlConnection(@"Server=.\SQLEXPRESS;Database=mydb;Trusted_Connection=True;TrustServerCertificate=True"));
    }
       
}

Attached is a simple project test that cannot compile. If removing the override keyword the project compiles but the overload is obviously ignored.

WebApplication1.zip

EF Core version: 7
Database provider: (e.g. Microsoft.EntityFrameworkCore.SqlServer)
Target framework: (e.g. .NET 7.0)
Operating system: windows
IDE: (e.g. Visual Studio 2019 16.3)

@ajcvickers
Copy link
Member

ajcvickers commented Jul 26, 2022

@frandazzo The method is defined in the interface, and not implemented by the abstract base class. This means it can't be overridden, merely implemented, and hence should have the override keyword.

@dotnet/efteam Where we have existing abstract base types as well as interfaces with default implementations, should we consider adding methods to the abstract base types to avoid this confusion?

It's also interesting that it is now very easy to add a method that, accidentally, does not implement a method from the interface. This is, I believe, why override is required in C# when overriding a virtual method--I remember it making it better than some other languages, possibly Java, in this respect.

@frandazzo
Copy link
Author

Ok i've removed the override keyword but if i try to debug the interceptor method, nothing happens. It seems like no interceptor was added or that the ConnecitonCreating Method is not called.

@frandazzo
Copy link
Author

Moreover this ticket #23087 was closed stating that it was implemented from version 6
if i've well understood. Or i'm missing something? Thank you in advance

@ajcvickers
Copy link
Member

@frandazzo You're right; this doesn't work unless the interface is implemented explicitly as well as inheriting from the base class:

ConnectionInterceptor : DbConnectionInterceptor, IDbConnectionInterceptor

I will discuss with the team.

@frandazzo
Copy link
Author

Thank you @ajcvickers ;
The code now compile and gets executed but i don't know why i get and exception.
My goal is to use a connection factory on the connection interceptor and the code

return InterceptionResult.SuppressWithResult(new
SqlConnection(@"Server=.\SQLEXPRESS;Database=mydb;Trusted_Connection=True;TrustServerCertificate=True"));

is an exmple of what i have in mind.
Can you help me explaining why i get an exception

thank you in advance

@ajcvickers
Copy link
Member

@frandazzo Please post the exception message and stack trace.

@frandazzo
Copy link
Author

image

the db mydb3 does not exist! if i remove the interceptor the db is correcly created. At this point if rerun the app reenabling the interceptor it works. Why the ensureCreated does not work?
Thanks

@frandazzo
Copy link
Author

Why the ensureCreated does not work (with the interceptor)?

@frandazzo
Copy link
Author

Ok probably the problem is due to the attempt to open the connection for a not yet existing db. There is a workaround?

@ajcvickers
Copy link
Member

@frandazzo When EnsureCreated needs to create a database, it does so by opening a connection to the "master" database on the server for which the database needs to be created. (Since it can't open a connection to a database that doesn't exist.) Your code is blindly replacing that with a connection to the database that does not exist, and hence it is failing. You need to be more selective about when you replace the connection so that the connection to the master database is not replaced. The connection string can be obtained from, eventData.Context.Database.GetConnectionString().

Note for triage: we should make the RelationalConnection, or at least the connection string more easily accessible from

@frandazzo
Copy link
Author

@ajcvickers Thank you for your answer. Unfortunately i've found no way to use interceptors to pass a connection fatory to the dbcontext.
Thankyou the same

@ajcvickers ajcvickers reopened this Jul 26, 2022
@ajcvickers ajcvickers changed the title ConnectionCreating in DbConnectionInterceptor is not overridable Add virtual methods to existing interceptor base classes Jul 28, 2022
@ajcvickers ajcvickers self-assigned this Jul 28, 2022
@ajcvickers ajcvickers added this to the 7.0.0 milestone Jul 28, 2022
ajcvickers added a commit that referenced this issue Jul 29, 2022
@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 Jul 29, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-rc1 Aug 6, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-rc1, 7.0.0 Nov 5, 2022
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-interception 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
2 participants