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

Don't check for a connection string until after ConnectionOpening has been called #23085

Closed
3GDXC opened this issue Oct 23, 2020 · 13 comments · Fixed by #27874
Closed

Don't check for a connection string until after ConnectionOpening has been called #23085

3GDXC opened this issue Oct 23, 2020 · 13 comments · Fixed by #27874
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

@3GDXC
Copy link

3GDXC commented Oct 23, 2020

File a bug

using a connection interceptor I would expect that you should be able intercept prior to the connection string validation and/or before any logic is performed with regards to establishing a connection is performed.

at present the connection string is evaluated before the interceptor and therefore any additional information provided/added to the connection string for the interceptor is rejected,

@roji
Copy link
Member

roji commented Oct 25, 2020

The code sample below uses a connection interceptor to successfully modify the connection string before the connection is opened (in a way that makes the open fail). If you're still running into difficulties, can you please post a runnable code sample?

Code sample
await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

_ = ctx.Blogs.ToList();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    static ILoggerFactory ContextLoggerFactory
        => LoggerFactory.Create(b => b.AddConsole().AddFilter("", LogLevel.Information));

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlite("Filename=:memory:")
            .EnableSensitiveDataLogging()
            .UseLoggerFactory(ContextLoggerFactory)
            .AddInterceptors(new MyInterceptor());
}

class MyInterceptor : DbConnectionInterceptor
{
    public override InterceptionResult ConnectionOpening(
        DbConnection connection,
        ConnectionEventData eventData,
        InterceptionResult result)
    {
        Console.WriteLine("Intercepting");
        connection.ConnectionString += ";Foo=Bar";
        return result;
    }
}

public class Blog
{
    public int Id { get; set; }
    public string Name { get; set; }
}

@3GDXC
Copy link
Author

3GDXC commented Oct 25, 2020

@roji thanks but the sample provided is adding settings to the connection string after; I wish to supply a connection string with extra settings and then use these inside the interceptor and remove them so the connection can then be created.

If you modify you sample to have the Foo=Bar on the original connection string and have the interceptor remove this; that would be the correct type of implementation. However this would throw an exception as the connection string is not valid at point of creating the connection object; therefore I feel a CreatingConnection is required.

@roji
Copy link
Member

roji commented Oct 25, 2020

@3GDXC I don't follow... the code sample above modifies the connection string - it happens to be adding a keyword, but it could just as well perform any sort of manipulation on the connection string. You can set it to some other string, or parse the given string with SqliteConnectionStringBuilder, make changes on that, and assign the resulting string back.

More generally, a connection string can be assigned to DbConnection.ConnectionString at any point after the DbConnection is instantiated. It only has to happen before that connection is opened, in order for it to be taken into account. That's why I'm not clear on what CreatingConnection would add here compared to the existing ConnectionOpening.

@3GDXC
Copy link
Author

3GDXC commented Oct 25, 2020

@roji it seems that the ConnectionString is getting validated before the ConnectionOpening is called? as if you supply a connection string with extra key/value pairs (that aren't supported by the real conneciton) but would be used by the interceptor the interceptor ConnectionOpening never gets called ?

@roji
Copy link
Member

roji commented Oct 25, 2020

Are you saying that the original connection string (before any interceptor gets involved) is invalid, and you're trying to correct it with an interceptor? This would indeed not work with ConnectionOpening, but IMHO it's strange to want to deal with this via interception... Can you please provide more context on what you need to do?

@3GDXC
Copy link
Author

3GDXC commented Oct 25, 2020

@roji yes, lets correct

I would like to provide the interceptor some options for configuration; these options extend the options that are available on the providers connection string ie.

WASM=true; culture=en-gb;

would add WASM support to the connection and is the browser culture en-gb

these options are NOT valid for the original connection provider, but solely for the interceptors

as you are able to add addition key/value pairs to a connection string at the point 'ConnectionOpening' I would have expected to be able to remove key/value pairs also at this point prior to the opening of the connection; I would also expect that the connection string would be validate after this point by the original provider connection.

Therefore the interception could grab/correct/modify the connection string just prior to opening removing invalid values and/or values that are not intended for the connection provider.

@roji I hope trust this gives a better description of what I'm trying to do?

@roji
Copy link
Member

roji commented Oct 26, 2020

@3GDXC thanks for explaining, I'll take a deeper look at this (and your code sample) in the coming days.

@ajcvickers
Copy link
Contributor

@roji @3GDXC This might work in 5.0, but I'd be surprised if it works in 3.1. See #6525.

@ajcvickers
Copy link
Contributor

Note for triage: I investigated this and even though 5.0 allows the connection string to be mutated in ConnectionOpening, a valid connection string must still be initially supplied. This is unfortunate given that an empty connection string is invalid, which means doing optionsBuilder.UseSqlServer() cannot be used in combination with setting the connection string in ConnectionOpening

@ajcvickers
Copy link
Contributor

@bricelam @roji Found the source of confusion on the empty connection string issue: we throw from EF if the connection string is empty. We can likely move this check until after ConnectionOpening.

      Entity Framework Core 5.0.0-rc.2.20475.6 initialized 'SomeDbContext' using provider 'Microsoft.EntityFrameworkCore.SqlServer' with options: SensitiveDataLoggingEnabled 
Unhandled exception. System.InvalidOperationException: A relational store has been configured without specifying either the DbConnection or connection string to use.
   at Microsoft.EntityFrameworkCore.Storage.RelationalConnection.get_DbConnection()
   at Microsoft.EntityFrameworkCore.Storage.RelationalConnection.Open(Boolean errorsExpected)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerDatabaseCreator.<>c__DisplayClass18_0.<Exists>b__0(DateTime giveUp)
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.<>c__DisplayClass12_0`2.<Execute>b__0(DbContext c, TState s)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.Execute[TState,TResult](IExecutionStrategy strategy, TState state, Func`2 operation, Func`2 verifySucceeded)
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.Execute[TState,TResult](IExecutionStrategy strategy, TState state, Func`2 operation)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerDatabaseCreator.Exists(Boolean retryOnNotExists)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerDatabaseCreator.Exists()
   at Microsoft.EntityFrameworkCore.Storage.RelationalDatabaseCreator.EnsureDeleted()
   at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.EnsureDeleted()
   at Program.Main() in /home/ajcvickers/AllTogetherNow/FiveOh/Program.cs:line 28

@roji
Copy link
Member

roji commented Oct 30, 2020

Thanks @ajcvickers, makes sense!

@ajcvickers ajcvickers changed the title Connection Interceptors connection string Don't check for a connection string until after ConnectionOpening has been called Oct 30, 2020
@ajcvickers ajcvickers self-assigned this Nov 2, 2020
@ajcvickers ajcvickers added this to the Backlog milestone Nov 2, 2020
@maulik-modi
Copy link

@ajcvickers , In multi tenant systems, connection string is often not known until Tenant is resolved. How do you suggest we tacklet his scenario?

@ajcvickers
Copy link
Contributor

@maulik-modi Implementation of this issue would be a good start.

@ajcvickers ajcvickers modified the milestones: Backlog, 7.0.0 Oct 20, 2021
ajcvickers added a commit that referenced this issue Apr 23, 2022
Instead, let ADO.NET validate. This allows the connection string to be set as late as possible, for example in the ConnectionOpening interceptor.

Fixes #23085
@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 Apr 23, 2022
ajcvickers added a commit that referenced this issue Apr 25, 2022
Instead, let ADO.NET validate. This allows the connection string to be set as late as possible, for example in the ConnectionOpening interceptor.

Fixes #23085
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview5 Apr 25, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview5, 7.0.0 Nov 5, 2022
@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-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
Development

Successfully merging a pull request may close this issue.

4 participants