ladenedge / NLog.EntityFramework

An NLog database target that is configurable via an Entity Framework connection string.
Other
5 stars 2 forks source link

Remove System.Data.Entity dependency - use DbConnectionStringBuilder instead of EntityConnectionStringBuilder #2

Open Misiu opened 7 years ago

Misiu commented 7 years ago

I was able to parse out connection string from EntityFramework connection string using below code

const string ProviderConnectionStringParameterName = "provider connection string";
static void Main(string[] args)
{
    var connString = ConfigurationManager.ConnectionStrings["ApplicationDbContext"].ConnectionString;
    var dbConnectionStringBuilder = new DbConnectionStringBuilder {ConnectionString = connString};
    var key = dbConnectionStringBuilder.Keys.OfType<string>().FirstOrDefault(s => String.Equals(s, ProviderConnectionStringParameterName, StringComparison.OrdinalIgnoreCase));
    if (key == null) throw new ArgumentException(ProviderConnectionStringParameterName);
    var realConnectionString = dbConnectionStringBuilder[key];
    Console.WriteLine(realConnectionString);
    Console.ReadLine();
}

Instead of using EntityConnectionStringBuilder I'm using DbConnectionStringBuilder and getting provider connection string manually.

Please see http://www.dotnetframework.org/default.aspx/4@0/4@0/DEVDIV_TFS/Dev10/Releases/RTMRel/ndp/fx/src/DataEntity/System/Data/EntityClient/EntityConnectionStringBuilder@cs/1305376/EntityConnectionStringBuilder@cs or https://github.com/mono/entityframework/blob/master/src/EntityFramework/Core/EntityClient/EntityConnectionStringBuilder.cs

Basically EntityConnectionStringBuilder extends DbConnectionStringBuilder, after studying it code I was able to isolate part responsible for getting connection string.

Please take a look, if this will work fine we will be able to add Your target as official NLog Target.

@304NotModified could You take a look at this too?

304NotModified commented 7 years ago

@Misiu if you get this working, please send a PR to NLog.

If the sealed is really a problem, let me know

304NotModified commented 7 years ago

dbConnectionStringBuilder.Keys.OfType().FirstOrDefault(s => String.Equals(s, ProviderConnectionStringParameterName, StringComparison.OrdinalIgnoreCase));

This is a bit odd code? It's already a dictionary?

Misiu commented 7 years ago

@304NotModified I've figured that out, I simplified code even more. Sorry for delay but I had no time lately.

I'm getting provider and connection string this way:

const string providerConnectionStringParameterName = "provider connection string";
const string providerParameterName = "provider";

var dbConnectionStringBuilder = new DbConnectionStringBuilder {ConnectionString = cs.ConnectionString};

//CurrentValues Dictionary in DbConnectionStringBuilder is case insensitive (StringComparer.OrdinalIgnoreCase)

//should we use SimpleLayout.Escape here?
string provider = (string)dbConnectionStringBuilder[providerParameterName];
if (string.IsNullOrWhiteSpace(provider))
{
    throw new NLogConfigurationException("Provider not found");
}
this.ProviderFactory = DbProviderFactories.GetFactory(provider);

string connectionString = (string)dbConnectionStringBuilder[providerConnectionStringParameterName];
if (string.IsNullOrWhiteSpace(connectionString))
{
    throw new NLogConfigurationException("Connection string not found or empty.");
}

this.ConnectionString = SimpleLayout.Escape(connectionString);

I'll try to test my implementation of EntityFrameworkTarget and if it will work I'll let You know and I'll ask for review before doing PR.

Misiu commented 7 years ago

@304NotModified I've added my code for EntityFrameworkTarget here: https://github.com/Misiu/NLog/blob/master/src/NLog/Targets/EntityFrameworkTarget.cs please take a look. I've added in to netfx45 project. In what other project I should include it? All of them? I'll try to test everything this weekend.

304NotModified commented 7 years ago

@Misiu please send a PR

I've added in to netfx45 project. In what other project I should include it? All of them?

Yes, but not by hand. There is a script for it. see https://github.com/NLog/NLog/blob/master/CONTRIBUTING.md#sync-projects

Thanks in advance!

Kinda busy, but maybe we could add it soon to NLog!

304NotModified commented 7 years ago

PS: we do require unit tests O:)

Misiu commented 7 years ago

@304NotModified thanks for script hint. I never wrote test before (shame on me), but I'll try to do my best to write them correctly.