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

Port over DbProviderFactories class #4571

Closed
FransBouma opened this Issue Nov 18, 2015 · 116 comments

Comments

Projects
None yet
@FransBouma
Collaborator

FransBouma commented Nov 18, 2015

I have seen no info about this with respect to .NET Core, and it's a crucial element for (micro) ORMs and data-access libraries. The .NET full API for DbProviderFactory contains usage of DataTable, so I recon that's not going to be ported anytime soon, but there's a bigger problem: most provider factories are defined in machine.config, which with 'bcl per app' isn't going to fly anymore.

Have there been any design thoughts on this? Will .NET core have DbProviderFactory machinery so an ORM can obtain the factory from a central API (I couldn't find the API of .NET full in corefx at the moment), and create ADO.NET elements using that factory?

Or is this in limbo just like DataTable (#1039)

@YoungGah

This comment has been minimized.

YoungGah commented Nov 25, 2015

@FransBouma, you are correct that DataTable is not the only design issue we need to solve here to make DbProviderFactory and DbProviderFactories to work. The registration mechanism for Full Framework was through config files. For NET Core, we will have to come up with a new registration mechanism, but we don't have concrete design. This will be addressed post v1.0.

@FransBouma

This comment has been minimized.

Collaborator

FransBouma commented Nov 25, 2015

Do realize that if this is introduced post v1.0, the v1.0 runtime will be a bit useless for ORMs: without a providerfactory, it's much harder to write db agnostic code that doesn't reference ADO.NET specific providers. .NET 1.x has learned a hard lesson: taking dependencies on ADO.NET providers in an ORM is a versioning nightmare. So not having any providerfactory support whatsoever (not even in the app config file or whatever it's called in corefx) is a showstopper.

@davidfowl

This comment has been minimized.

Contributor

davidfowl commented Nov 25, 2015

Just going to +1 this and say it should be all code based configuration. That's one thing I hated about those APIs in the past.

@schotime

This comment has been minimized.

schotime commented Nov 25, 2015

I have just added the DbProviderFactory as a constructor parameter now and it can be passed in when you reference your particular database driver. Eg for sql server. SqlClientFactory.Instance

It's a suitable workaround for now. I also agree with @davidfowl about a new code mechanism. However now that I think about it. You would have to register that in the app anyway so might as well just do what ive done.

@YoungGah

This comment has been minimized.

YoungGah commented Dec 1, 2015

@davidfowl and @schotime, yes, the current thinking is to provide code based configuration.

@YoungGah YoungGah assigned YoungGah and unassigned saurabh500 Dec 3, 2015

@eschneider999

This comment has been minimized.

eschneider999 commented Feb 10, 2016

My micro orm Symbiotic is at %97+ to .NET Core compatibility. Just need DbProviderFactory and some config junk.

@kkurni

This comment has been minimized.

Contributor

kkurni commented Mar 22, 2016

Here our proposal for DbProviderFactories for corefx by using Code based Configuration
#6476

As a background in .NET Framework DbProviderFactories is a configuration based mechanism which can be used to resolve a DbProviderFactory instance that is registered in configuration (most often in machine.config) based on a string containing a well-known “provider invariant name”. Applications using this mechanism generally don’t need to depend on the provider directly because the provider is also registered globally in the GAC.

As you probably know in CoreFx, there will be No GAC and global configuration anymore.
This make the concept of DbProviderFactories become less useful.
And for dependency injection purposes, there are lots of better alternative. e.g Contruction injection as @schotime mentioned, or using simple dictionary or your favorite dependency injection mechanism.

@FransBouma, @eschneider999, @schotime , @davidfowl or others, Please let us know whether
this DbProviderFactories will still be useful for you in Corefx context.
#6476

@kkurni

This comment has been minimized.

Contributor

kkurni commented Mar 22, 2016

@FransBouma

This comment has been minimized.

Collaborator

FransBouma commented Mar 22, 2016

I understand the lack of GAC / machine.config makes it all a bit problematic and therefore would fall back onto the mechanism of the app's config file configuration for DbProviderFactory (similar to what is possible in .NET full today), and if configuration in CoreFX is done through code, not config files then this is the wisest solution.

At first glance I see no way to obtain a factory when I have just an invariant name? As you register the factory under a name with DbProviderFactoriesConfiguration but that class stores the registered factory internally and there's no way to obtain the registered factory? Your proposal specifies contract changes, but it's unclear to me what changes where.

There's another problem, which is perhaps less of your concern, and that's factory overwriting is not supported with your solution. ADO.NET profilers (like Glimpse, EFProf and also ORM Profiler) tend to overwrite the static table in .NET full with their own factories which intercept any call to the factory instance and create wrapping elements instead to intercept calls to ADO.NET elements. Your proposed solution doesn't offer that flexibility anymore.

@kkurni

This comment has been minimized.

Contributor

kkurni commented Mar 22, 2016

@FransBouma you can access your registered factory the same ways as the old ways in .NET Frameworks.

DbFactories.GetFactory(string providerInvariantName);
DbFactories.GetFactory(DbConnection connection);

In CoreFx, you will need to use DbProviderFactoriesConfiguration to register your provider.

DbProviderFactoriesConfiguration.Add("Sytem.Data.SqlClient", () => SqlClientFactory.Instance);

But in .NET Frameworks, you don't need to do anything, it will read from the machine config as it is.

@kkurni

This comment has been minimized.

Contributor

kkurni commented Mar 22, 2016

The reason why we are not allowing to overwrite is to make the behavior less uncertain.
e.g libraries can just overwrite this code based config without any agreement. This will be hard to debug.

But an applications may be able to provide their own safe way to overwriting their provider config.
e.g file based config which allow others to override their config providers based on directory structure.

@FransBouma

This comment has been minimized.

Collaborator

FransBouma commented Mar 23, 2016

Re: retrieving the factory: ah yes, it was unclear a bit to me whether that was changed or not, this morning I realized it was indeed the same. Sounds good!

Re: overwriting: ok. It's not a big deal I think. The applications now also have to call a line in the profiler to make it overwrite the factories in the datatable, and instead of that the developer can simply register a different factory (i.e. the one from the profiler instead of the regular one). It's a bit more explicit but at least it is possible to do.

Regarding dependencies: I think the main change is: in regular .NET, an application doesn't have to reference an ADO.NET provider assembly directly, and in CoreFX this will be different: The application has to reference the ADO.NET provider assembly directly as the registration of the factory (which is in the ADO.NET provider assembly) has to be registered. I think this unavoidable as there's no central GAC/machine.config so it is what it is.

So IMHO it all looks OK to me: porting code over to CoreFX will now be easier as the mechanisms are the same. Thanks for this!

@kkurni kkurni assigned kkurni and unassigned kkurni Mar 23, 2016

@davidfowl

This comment has been minimized.

Contributor

davidfowl commented Mar 29, 2016

Here's a question for you @FransBouma, how are you using this API? Since there's no config system and the application is responsible for wiring everything up anyways, why not just provide an API that takes a factory directly. It's up to user code to wire it up

@FransBouma

This comment has been minimized.

Collaborator

FransBouma commented Mar 29, 2016

Here's a question for you @FransBouma https://github.com/FransBouma , how are you using this API?

What do you mean with this question? As there's hardly any API to use: you call the static class' method to obtain the factory for a known name, you get the factory, cache it (as it's a static instance), and use the instance to create ADO.NET objects for using with a database.

At least that's how I (and everyone else) use it on full .NET. On CoreFX I haven't used it yet, as it's impossible to create proper software with the pre-RC2 tooling.

Since there's no config system and the application is responsible for wiring everything up anyways,
why not just provide an API that takes a factory directly. It's up to user code to wire it up

No, that's way too simplistic. The application wires things up for the application, but it has not and should not know internals about the used ORM / Data access layer, other than perhaps which ADO.NET provider to use, but not how to set things up internally with that. Besides, it's not always known to the application when the ORM is used, or which ORM, or which database up front, only at runtime.

It also makes things overly complicated as current code can now easily be ported to CoreFX because there's no direct line between application logic and the query engine of an ORM: it obtains the factory through the system. Your proposal requires code to be refactored because the factory has to be supplied directly to the query engine of the ORM by the application logic.

In all honesty I then wonder what the real point of your question is: is this API being cut?

@joshfree joshfree modified the milestones: 1.0.0-rtm, 1.0.0-rc2 Mar 29, 2016

@kkurni

This comment has been minimized.

Contributor

kkurni commented Mar 29, 2016

@FransBouma sorry to put high expectation for this proposal initially.

At this moment this proposal is currently under reviewed by internal teams which raised couple concern.
The biggest concern at this moment is "this DbProviderFactoriesConfig will caused confusion for PCL Libraries". e.g if a PCL libraries register explicitly through DbProviderFactoriesConfig and bring this PCL to .Net Framework (e.g Asp.net), all those registration will get ignored. and .Net Framework will use Machine.config instead of whatever that had been registered in those PCL Library.

And other concern is about "the original reason why we don't bring this DbProviderFactories in CoreFx".
We try to clean the new CoreFx API as possible from anti pattern. and we think without this DbProviderFactories, library writer can still depend directly from DbProviderFactory. the application writer can provide / inject this DbProviderFactory directly through the library and the library will just use DbProviderFactory directly.
At the end of the day, this actual implementation of DbProviderFactory (e.g SqlFactory) will need to be registered anyway from the application with this proposal.
(The library/ORM,still can depend on DbProviderFactory - System.Data.Common, but the application who use that library will depend on SqlDataProvider even with this proposal)

So at this moment, we need to gather more information from the community for the usage and suggest alternative way to use DbProviderFactory directly. We still need to do more analysis and threat modeling for this proposal.

I will keep you up to date if there is any progress on this and please give us any input and suggestions

Thanks

@jnm2

This comment has been minimized.

Collaborator

jnm2 commented May 31, 2017

@karelz

One thing that strikes me a bit is that we had maybe 50 new contributors contributing to CoreFX this year and this is first feedback of this type ... so I wonder (truly, without any attacks or anything), if your standards are a bit higher? ;)

No, I agree with @FransBouma. The practices that have become obvious to regular contributors are still really confusing and time consuming to grep, even when you move to doing different kinds of contribution like API addition. Running and debugging tests is tedious and I couldn't always get it to work. Just a data point.

Every time I have asked for some straightforward docs on each type of procedure, I hear that your team practices are evolving too fast and people don't have time to document it. (And then I am welcomed to document your practices, even though I'm never going to have the full picture even when I figure out everything necessary for my contribution.)

It is essential to maintain and evolve crystal clear and comprehensive step-by-step contribution instructions along with the actual projects and build scripts. Having that makes your project more attractive to outside contributors and is more considerate of their time, to put it bluntly. I've contributed to a number of projects and this is the single biggest thing you notice about each project.

Sure, I'll contribute again because are you kidding, this is the BCL! =) It's worth the effort.

@karelz

This comment has been minimized.

Member

karelz commented May 31, 2017

Thanks for feedback @jnm2! It helps to know others are/were facing similar problems. It will help us bump priority of the docs.

Good new contrib docs is something I have on my todo list for quite a while. I hope that I will be able to ask someone on the team to help improve them during June.
So I personally understand you and can't agree more. The more contributors who confirm the feedback from you & @FransBouma, the easier it will be for me to get funding ;).

I believe we passed the point of "we are evolving too fast to have docs" in January. Our infra is pretty decent now and I don't think we can afford that excuse anymore. VS integration for contributors is lacking, we know that. At minimum we should call out huge warning in the docs. Ideally we would fix VS and then react to every single report of things not working.

cc @weshaggard @ericstj @mellinoe @danmosemsft

@jnm2

This comment has been minimized.

Collaborator

jnm2 commented May 31, 2017

@karelz Thanks for considering! I saw a similar comment here: #20325 (comment)

@karelz

This comment has been minimized.

Member

karelz commented May 31, 2017

@jnm2 that feedback is about using .NET Core 2.0 to develop libraries. It is different than the topic here - contributions to the CoreFX repo itself.
While they seem similar, there is huge difference - CoreFX has its own build system, test runner, etc. We do not reuse the same things that .NET Core 2.0 developers use. The requirements to build the platform (CoreFX) itself are slightly different from building on top of the platform (libraries targeting .NET Standard / .NET Core). The timeline is also different - the platform itself needs the support from day 0 to build the platform itself, while building on top of the platform can often wait for later, when the platform somewhat exists.

@jnm2

This comment has been minimized.

Collaborator

jnm2 commented May 31, 2017

Okay, my bad. I thought ImageSharp was a corefx-like project.

@willdean

This comment has been minimized.

Collaborator

willdean commented Jun 1, 2017

As one of those 50 new corefx contributors, I don't think I can allow myself to be co-opted in wholehearted support of the way things are.

I found corefx very difficult to work on:

  • I opened/contributed to a number of issues about tooling and doc issues faced by the newcomer
  • I politely discussed ways to work around xUnit's miserable assertion API
  • I politely discussed ways to work around xUnit's pathetic trace collection problems
  • Like all good subjects, I refrained from pointing out that xUnit has no clothes.
  • In order to actually debug tests I created new projects from scratch in VS, link-inserted all the relevant bits of corefx into them and then debugged in there. Only at the end did I go back to the execrable command line test runner experience.
  • I maintain a text file of preposterous command lines which are needed to run various bits of the test framework in various ways.

For balance, I'll say that my very positive experiences of working on corefx stem from the friendly and helpful attitude of the corefx MS guys on GitHub. I wish I could have @karelz to do first-line support on every OSS project I've worked on, and there were lots of others too...

I actually do get it why the framework's own development couldn't start from exactly the same tooling position as end-users will ultimately use, and corefx probably feels the need to outpace the lumbering old donkey which is VS. But corefx is basically only building class libraries, so I don't see why it needs to be so far in left field. However, I can't help thinking that if the corefx team actually had to develop corefx code in the same way as every other .NET library is developed (i.e. in VS, with 3rd-party test runners), then both VS and .NET Core would be better for it.

I know this is off-topic here but, genuine question, where would it be on-topic? Can I actually usefully open an issue in corefx's GitHub repo saying (more politely) "The VS .NET Core dev experience is unutterably awful"? The scale of the problem is way beyond filing VS bugs, even if I did fancy exchanging platitudinous snippets with the staff of a Chinese outsourcing company.

@jnm2 Has a perfectly valid point - these is absolutely no good reason at this time why the 3rd-party development of a library like ImageSharp should look any different to the MS development of the vast majority of corefx.

I'm ready for my telling-off about unprofessionalism now...

@FransBouma

This comment has been minimized.

Collaborator

FransBouma commented Jun 1, 2017

@willdean

In order to actually debug tests I created new projects from scratch in VS, link-inserted all the relevant bits of corefx into them and then debugged in there. Only at the end did I go back to the execrable command line test runner experience.

I think I lack this knowledge at the moment. Is there a page you can point me to so I can gain that knowledge and create these projects myself too? I know I can look at other project csprojs, and have done so, but as the machinery uses .props files I don't know how to do this properly. One question I haven't seen answered and which is related is: you do need 15.3 update for vs2017 for this? Thanks

I maintain a text file of preposterous command lines which are needed to run various bits of the test framework in various ways.

Would you like to share those for others to use? That would be great :)

I think it's a matter of once you 'know what to do and have the info bits to navigate around the dark pits of hell' it's easy to contribute and you forget you ever had problems. To get there is a bit of a struggle ;)

For balance, I'll say that my very positive experiences of working on corefx stem from the friendly and helpful attitude of the corefx MS guys on GitHub. I wish I could have @karelz to do first-line support on every OSS project I've worked on, and there were lots of others too...

Seconded

I also agree with the assertions you made regarding xunit and it's... let's say 'less ideal ways of doing things' to stay polite. I've never seen a framework that has one job and do it in such an user unfriendly manner. Almost as equally unpleasant as vi on an SCO unix prompt. Oh... did I just slip off the 'politically correct' path there? ;)

@willdean

This comment has been minimized.

Collaborator

willdean commented Jun 1, 2017

@FransBouma I don't think I have anything useful to share really - the stuff I worked on was a direct port of netfx code, and so to debug the tests I built them as a netfx project against the original netfx implementation, debugged the tests in netfx land and then went back to corefx.

I also never had to create a corefx project from scratch because someone in MS did that, so I can't offer any advice there, other than that I'd probably just copy a simple existing sub-project within corefx and run around changing names, etc.

I was using VS2015 at the time with corefx - I wasn't clear about VS2017 support in corefx at that point (earlier this year). I would be very wary of installing the 15.3 preview - it's billed as a safe 'side-by-side' install, but on my machine it trashed all the VS2017 15.2 settings back to defaults which is a huge pain, and the 15.3 preview crashes every 30 seconds anyway, so I didn't find it useful for anything.

Here's one of my test-running command lines, though I doubt that's really where you're stuck, and if you're on a development box then there are easier commands to run all the tests in a project which are part of the corefx build system. (Still all command-line though).

N:\will\corefx\bin/testhost/netcoreapp-Windows_NT-Debug-x64\dotnet.exe xunit.console.netcore.exe System.IO.Ports.Tests.dll  -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapp1.1tests  -parallel none -showprogress

If you can get your tooling expectations dialed-back about 25 years (but without the 8.3 filename restriction) then you might find less disappointment.

@FransBouma

This comment has been minimized.

Collaborator

FransBouma commented Jun 1, 2017

@willdean Thanks, any info is good info :) Especially regarding 2015 and 2017 15.3. It tells me the stuff I have in place should be sufficient, and that's not it. If no-one from MS has time to solve it, I'll indeed try to copy an existing project and see whether I can get that running.

If you can get your tooling expectations dialed-back about 25 years (but without the 8.3 filename restriction) then you might find less disappointment

heh :) good point. I'll dial it back to the experience with what I used back then haha ;) (vi editing, typing cc or make on command line for compilation, for debugging gdb without source overview, on a 80x24 WYSE terminal)

@jnm2

This comment has been minimized.

Collaborator

jnm2 commented Jun 1, 2017

For the most part it was fine but there were aspects of using xUnit that I didn't enjoy, also. I don't expect that they're interested in switching but as a data point, while I was contributing I wished I was using NUnit so many times. (This was before I became a member of NUnit, so no disclaimer needed.)

@karelz

This comment has been minimized.

Member

karelz commented Jun 1, 2017

First, thanks everyone for the feedback - I am ABSOLUTELY interested in this kind of feedback! Now, let's do something about it ...

I suggest to move the discussion into separate issue - that is totally appropriate way to discuss "how to contribute to CoreFX". I would suggest to split the discussion into 2 parts - one for contributor experience hassle (incl. VS) (see #20570) and second about xUnit (TODO anyone?).
Motivation: You might have guessed that changing fundamental part of our infra as xUnit, is a LOT of work and while I am interested in feedback there as well, it won't change overnight and potentially never (due to high cost) -- either way, it is good and healthy to have that discussion anyway.

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jun 1, 2017

I suggest in those issues we try to get wider consensus on a prioritized list of improvements. There are lower hanging fruit than xunit, I think.

@willdean

This comment has been minimized.

Collaborator

willdean commented Jun 1, 2017

I don't think there would be any need to replace xUnit, just have some of the more painful omissions fixed - I think there is some (historical?) connection between xUnit and MS, so there may be some encouragement could be brought to bear there?

@karelz

This comment has been minimized.

Member

karelz commented Jun 1, 2017

@willdean if there are painful omissions in xUnit, please file a new issue, so that we can understand them and then act on them. Thanks!

@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Jun 2, 2017

Nice to see I'm not alone in my xunit non-fandom. Here are my issues (from a very cmd-line bare-metal developer when it comes to tests.)

dotnet/buildtools#1538

Looking forward to that xunit thread...

saurabh500 added a commit that referenced this issue Dec 5, 2017

Implementation of DbProviderFactories. (#25410)
* Implements #19826: DbConnection doesn't offer an internal property ProviderFactory

* Added test for #19826. Due to project misconfiguration it fails locally.

* Removed writeline

* Added comment to illustrate the purpose of the property which currently is dead code.

* Implementation of DbProviderFactories, ported from NetFx and adjusted to CoreFx

See #4571, #19825 and #19826

* Update for project files to get a netcoreapp test set

* Changes to project system to get the stuff compiled. Failed.

* Various changes:

- Updated config/csproj files for netcoreapp for tests.
- Properly moved DbProviderFactories file to right location
- Added DbProviderFactories type to ref project, split across 2 files
(one for netcoreapp specific methods)
- Changed SetFactory to ConfigureFactory
- Changed generic type argument to normal method parameter
- Removed default parameter values and added overloads

* Added tests for DbProviderFactories.

* Better subclass testing added and more tests added

* Moved all hard-coded strings to SR/Strings.resx. Added test for factory retrieval using a connection

* Removal of now redundant comment. See: #19885 (review)

* Comment correction for bad English

* Refactored API a bit, see: dotnet/standard#356 (comment)

* Added tests for reworked API. Added tests for wrong type passing

* Reverting change in sln file: See #19885 (review)

* Almost done implementation using DataTable internal storage. Refactoring now to concurrentdictionary

* Final work for #20903

* Corrections of the implementation->

- registrations of assembly type name are now deferred
- storage is now a struct, as the typename has to be registrated as well.
- corrected a wrong nameof() call.
- added tests for the change behavior in RegisterFactory(string, string)

* Final implementation

* Corrections made to code by request of @saurabh500

* Github for windows didn't commit this file in last commit... :/

* Again correcting component elements. These are automatically inserted so if they're back again, I can't remove them

* ... annnnd another component element. Last one I hope

* @divega requested changes

- Corrected sln file netcoreapp->netstandard
- Corrected wording in exception messages.
- Component elements are back, I'll leave them now, they get back regardless.
- Renamed column name constants to have the 'ColumnName' suffix for more clarity
- Made Instance constant be initialized with nameof(Instance)
- Added using for System.Reflection so less verbose code

* More @divega requested changes

- factored out a clone into its own method (GetProviderTypeFromTypeName)
- Reverted 'nameof(Instance)' to string, and renamed field
- Removed redundant null/length check in ctor of ProviderRegistration ctor

* Last nit removal for @divega
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment