search

jonwagner / Insight.Database

Fast, lightweight .NET micro-ORM
Other
857 stars 145 forks source link

Feat: Add Microsoft.Data.SqlClient Provider #427

Closed Jaxelr closed 4 years ago

Jaxelr commented 4 years ago

Description

Add an Insight Db Provider that integrates with the Microsoft.Data.SqlClient library.

Checklist

Please make sure your pull request fulfills the following requirements:

Type

This pull request includes what type of changes?

Breaking Changes

Any other comment

This addresses Issue #424

I added very few tests since i presume the functionality of the Insight.Tests should behave similarly to System.Data.SqlClient. If needed i could add more tests that mirror the Insight.Tests test proj.

jonwagner commented 4 years ago

Nice. This will definitely make me find time to do a round of builds.

As for the tests - copying the existing tests should be sufficient.

Jaxelr commented 4 years ago

I will add the tests during the week, thx for the quick feedback.

jonwagner commented 4 years ago

Just the few sql-server-specific tests. Unfortunately, they may not be carved out from the main test suite.

We'll want to make sure that stored procs, TVPs, and custom types work. I think the rest of the code is pretty generic.

Maybe an option is to keep the existing tests as-is but then run each of the tests with two different providers? That would be more comprehensive but probably more work.

bart-degreed commented 4 years ago

Instead of copy/pasting a lot of code, would it be feasible to make Microsoft.Data.SqlClient the new default? I think Entity Framework Core is doing that too.

Then unsupported framework versions (net45) can still fallback to the old behavior:

# if NET45
using System.Data.SqlClient;
#else
using Microsoft.Data.SqlClient;
#endif

Note that my projects are also using Insight.Database.Providers.MiniProfiler. Having a single variant for SQL Server may just keep things easier.

Jaxelr commented 4 years ago

Its a breaking change for sure, consider that some clients of the library work on constrained environments, were adding a newer dependency (like lets say when using the net framework) is not trivial.

Jaxelr commented 4 years ago

I will surely try that for the test prj though.

jonwagner commented 4 years ago

None of the core code depends on features for a particular driver. The provider model does all of the driver customizations and duplicating is the cleanest way. Btw - we have this approach for oracle drivers

On Jan 31, 2020, at 12:24 PM, Jaxel Rojas notifications@github.com wrote:



Its a breaking change for sure, consider that some clients of the library work on constrained environments, were adding a newer dependency (like lets say when using the net framework) is not trivial.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/pull/427?email_source=notifications&email_token=AAMTO5FQUHPVXMHD2GS6UE3RARUEFA5CNFSM4KL3ECE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKPRMPY#issuecomment-580851263, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAMTO5H43YR2DRRS4AOJB5TRARUEFANCNFSM4KL3ECEQ.

Jaxelr commented 4 years ago

Tried adding directives for Insight.Tests project using MsSqlClient lib, and it felt messy. Decided to instead copy a few tests from base class that includes TVP, procs and Types.

Jaxelr commented 4 years ago

@bart-degreed MiniProfiler provider doesnt include a ref to *.Data.SqlClient, so it should work with each.

andymac4182 commented 4 years ago

Any update on this? We have ef that is using the new libraries causing incomparability so we can't share the connection between both libraries. Any way we can help with it?

jonwagner commented 4 years ago

@andymac4182 rainy holiday weekend with no pandemic fires for me to fight. :)

andymac4182 commented 4 years ago

Thanks heaps for your work and @Jaxelr :)