Implementation of DbProviderFactories. #25410

FransBouma · 2017-11-21T15:49:02Z

This PR implements API reviewed here: #20903 "Additional API for DbProviderFactories in .NET Core".
Fixes #4571

Design decisions:

GetFactory(DataRow) follows the same behavior as on NetFX: it will return the factory which type is present in the DataRow present. This can lead to undefined behavior when a different factory is registered in the DbProviderFactories for a given invariant name than the one specified in the DataRow. As this is a catch-22 situation with no true answer, the decision for now was made to make it behave the same as on NetFX
RegisterFactory(string providerInvariantName, string factoryTypeAssemblyQualifiedName) registers the type name but doesn't check the type as that would require the load of the assembly and this method is meant to be used to pre-register factories which might not be present on the system (yet) (e.g. by an ORM). The type registration is deferred till the factory is requested through a GetFactory method. This might cause a false sense of correctness: the type registered might not be valid. This is a situation following from the nature of the method.
TryGetFactory(string providerInvariantName, out DbProviderFactory factory) will still throw an exception when the factory type associated with the providerInvariantName was registered with RegisterFactory(string, string) and the type is invalid, as the type checking was deferred when the registration took place. In the API review thread it was undecided what to do, so I decided to stick with the exception in this case: the alternative, swallow the exception and returning false could suggest the registration earlier was successful, or that the factory isn't there/not registered, which isn't the case: a factory is registered, the type specified just couldn't be loaded.
Internal implementation is using a ConcurrentDictionary instead of a DataTable, which is used in NetFx. This is done to avoid locks as the IEnumerable<string> GetProviderInvariantNames() method would lead to upscaling a ReaderWriterLockSlim lock to write level in the case of an internal Datatable
The method DataTable GetFactoryClasses() builds the datatable every time. Initially I had a cached version to avoid re-allocating columns again, but maintaining that construct was rather tedious and this method isn't expected to be on a hot path at all (it's likely called once in an application's life) so I've chosen for a lock-free rebuild of the datatable every time. NetFx creates a copy of the internal table, for comparison.
The method IEnumerable<string> GetProviderInvariantNames() returns a List<T> as that was the most simplest of implementations. If an array is preferred I could change that. As the internal structure is a concurrentdictionary and to keep things threadsafe, yield-ing over the keys set of the concurrent dictionary didn't feel correct, but please correct me if this should be updated. :)

…oviderFactory

…ly is dead code.

… to CoreFx See #4571, #19825 and #19826

- 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

…ection_ProviderFactory_19826

…ry retrieval using a connection

…erFactory_19826

…ing now to concurrentdictionary

- 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)

…erFactory_19826

saurabh500 · 2017-11-27T21:44:40Z

@FransBouma @karelz I will be looking into the changes tomorrow.

FransBouma · 2017-11-29T15:45:03Z

Ping @karelz @divega @saurabh500 and others?

saurabh500

Overall looks good to me. Left some minor comments to be addressed or answered.

saurabh500 · 2017-11-27T21:22:54Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+            DataColumn nameColumn = new DataColumn(Name, typeof(string)) { ReadOnly = true };
+            DataColumn descriptionColumn = new DataColumn(Description, typeof(string)) { ReadOnly = true };
+            DataColumn invariantNameColumn = new DataColumn(InvariantName, typeof(string)) { ReadOnly = true };
+            DataColumn assemblyQualifiedNameColumn = new DataColumn(AssemblyQualifiedName, typeof(string)) { ReadOnly = true};


Nit: true}; Space after true

saurabh500 · 2017-11-29T16:37:26Z

src/System.Data.Common/src/System.Data.Common.csproj

@@ -57,7 +57,9 @@
    <Compile Include="System\Data\ConstraintCollection.cs" />
    <Compile Include="System\Data\ConstraintConverter.cs" />
    <Compile Include="System\Data\ConstraintEnumerator.cs" />
-    <Compile Include="System\Data\DataColumn.cs" />
+    <Compile Include="System\Data\DataColumn.cs">


Nit: The <SubType>Component</SubType> was added by VS .
Can you remove these tags please ? I wish I knew a way to turn off their generation in VS.

saurabh500 · 2017-11-29T22:38:54Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                // Deferred registration, do checks now on the type specified and register instance in storage.
+                // Even in the case of throwOnError being false, this will throw when an exception occurs checking the registered type as the user has to be notified the 
+                // registration is invalid, even though the registration is there.
+                Type providerType = Type.GetType(registration.FactoryTypeAssemblyQualifiedName);


Do we need a check for null or empty for registration.FactoryTypeAssemblyQualifiedName ?
Or is it that Type.GetType would return a null for an null or empty parameter passed in?

The check for that is done at RegisterFactory(string, string), line 114:

public static void RegisterFactory(string providerInvariantName, string factoryTypeAssemblyQualifiedName) { ADP.CheckArgumentLength(providerInvariantName, nameof(providerInvariantName)); ADP.CheckArgumentLength(factoryTypeAssemblyQualifiedName, nameof(factoryTypeAssemblyQualifiedName));

It then creates a ProviderRegistration class which on its own again checks for the value being not null and have a length, so it's not possible to have a name that's null or empty.

Not a big deal, but my first thought when I saw the ProviderRegistration constructor is that it could validate that either the instance of the AQN are not null. If the instance is passed, the only case in which we may need to obtain the AQN again is GetProviderClasses, which should be used infrequently.

FWIW, even if we did that, this code would still be safe without a null check.

the struct is private and its ctor is used only in codepaths which indeed check inputs, the null check in the ProviderRegistration could currently be removed. I added it there for clarity as it shouldn't be null. It's easier for maintainers who might alter the code later on: they then don't have to worry if a code path could lead to a null value there. btw the netfx code contains duplicate null checks too in a code path, which I guess are there for similar reasons :)

I get all that, and I like that you put the check there... maybe I am not getting my main point across: if the instance is set, we can allow the AQN to be null.

ah I didn't understand that from your remark. If the instance is null, then indeed the name can be null, as the if at line 158 isn't true. But what shall I do now: wrap the check in an if (complexer code) ? Or leave it as is? Or remove the null check?

I don't feel strongly about this but thinking about it again, it seems that doing the validation on the constructor provides little value. E.g. there is still a default constructor that allows to create an invalid ProviderRegistration, and the validation hast to be performed elsewhere anyway.

I still believe we don't need to eagerly store the assembly qualified name in the ProviderRegistration when the instance is provided (we can compute it on the fly on GetFactoryClasses), but I don't feel super strongly about that either.

The default ctor is indeed a mitigating factor for the check in the other ctor. I'll remove that one, and leave it at that. storing the aqn isn't harmful in any way, in fact, explicitly storing a null might raise an eyebrow as the question might arise "why is it deliberately left empty" which then requires a comment "we can recompute it later", IMHO unneeded complexity.

saurabh500 · 2017-11-29T22:41:53Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+
+        public static DbProviderFactory GetFactory(string providerInvariantName)
+        {
+            return GetFactory(providerInvariantName, throwOnError:true);


Nit: Space before true throwOnError:true

FransBouma · 2017-12-01T09:26:33Z

The DataColumn class indeed got that Component element, I misunderstood the remarks made earlier, I thought it was about the Compile element. I've removed the component element. Decorating the class with [System.ComponentModel.DesignerCategory("Code")] might help though, e.g. double clicking the file will then open it in the text editor, and not in the 'designer view' which is useless for DataColumn anyway. But not sure of the impact on that wrt DataTable designers, so that's for another time.

Will correct the other requests too.

FransBouma · 2017-12-01T09:39:20Z

Ugh, I see a lot of other files have component elements now too in the csproj. Will remove them. Sorry about that.

FransBouma · 2017-12-01T09:45:48Z

I can't get rid of the component elements... I remove them, I reload the project, I rebuild, I run the tests, and viola there they are again. This is in 2017 btw. Will now try vs2015, perhaps that's less problematic.

(edit) Issuing a 'build' or rebuild on the solution in vs recreates them. they're recreated every time I build the solution now in vs. I'll remove them and then commit the file. Why is this task so jinxed...

(edit) fixing the elements and then loading it into 2017 and building with ctrl-shift-B doesn't reapply the elements. Anyway, I'll commit it now and hopefully they're not reintroduced by some ghost. :/

… so if they're back again, I can't remove them

FransBouma · 2017-12-01T09:55:22Z

src/System.Data.Common/tests/System.Data.Common.Tests.csproj

@@ -74,7 +77,9 @@
    <Compile Include="System\Data\InRowChangingEventExceptionTest.cs" />
    <Compile Include="System\Data\InvalidConstraintExceptionTest.cs" />
    <Compile Include="System\Data\MissingPrimaryKeyExceptionTest.cs" />
-    <Compile Include="System\Data\MonkeyDataSet.cs" />
+    <Compile Include="System\Data\MonkeyDataSet.cs">


Forgot one... 😢

@FransBouma Don't spend too much time removing <SubType>Component</SubType>
What we typically do on the team is, we make changes in VS and allow it to pollute the CSProj and then we open the CSProj in a text editor and fix the XML to remove the component tag and then run the tests.

Alternatively, the csproj can be manually modified in a text editor to add the Compile tags and the changes are committed. After that work on the project in VS and after editing code, discard any unnecessary changes to Csproj after previous commit to make sure that it is clean.

The reason I mention this cleanup is because VS tries to open Component marked files in Designer and many a times it fails and the developer has to right click on the file and select View Code.

However this is definitely not the crux of the PR :) Let's push to keep the ball rolling and address the other discussion comments opened by @divega
I could clean up the Component tags if they become a problem.

@saurabh500 :) The issue with opening the file in the designer view is indeed annoying :) It can be overcome with the attribute I mentioned earlier: decorating a class with [System.ComponentModel.DesignerCategory("Code")] and then opening it in vs will open it in the code editor. I think the only problem is if you decorate a class with that, it obviously will never open in the designer ;) which is a problem for typed datasets and the like ;)

But agreed let's finish the last points and then wrap it up :) I think we're almost done with the changes, only the point regarding the null check in the ProviderRegistration ctor is left I think?

Yeah that seems like the only open discussion point right now.

Didn’t know about [System.ComponentModel.DesignerCategory("Code")]. Thanks! I will leave it to @saurabh500 here but I will definitely consider using that in other repos. Even if it is a bit more code, it seems a small price to pay for not having to fight VS ever again on this.

@bricelam FYI

divega · 2017-12-01T10:03:07Z

I can't get rid of the component elements...

Up to @saurabh500 who I think asked for them to be removed, but in other repos we have decided not to fight VS on this. It is annoying but doesn't seem worth it until there is a way to turn the behavior off.

divega

Still haven't looked at it all, but here are some comments.

divega · 2017-11-30T02:14:51Z

src/System.Data.Common/System.Data.Common.sln

@@ -26,8 +26,8 @@ Global
 		Release|Any CPU = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(ProjectConfigurationPlatforms) = postSolution
-		{B473F77D-4168-4123-932A-E88020B768FA}.Debug|Any CPU.ActiveCfg = netstandard-Debug|Any CPU


I don't understand enough about how the build works to understand why this change was needed, whether it is correct, and if is going to be sufficient (e.g. why the release configurations are not changing)?

Perhaps @saurabh500 will know this, but perhaps we need help from an corefx expert?

This must be an automated change due to some other change? I haven't changed anything deliberately in the solution nor csproj files, only to add the DbProviderFactories.cs class of course and the tests.

That could be. Do things work if you revert it?

Another related concern: not sure if we need to do anything special with the fact that the rest of System.Data.Common is .NET Standard 2.0 but this new class isn't.

Reverting it doesn't have any effect, tests (the project is the tests project) works fine. Will revert to 'netstandard' in next commit

divega · 2017-11-30T02:18:40Z

src/System.Data.Common/src/Resources/Strings.resx

@@ -512,4 +512,9 @@
  <data name="DataColumns_RemoveExpression" xml:space="preserve"><value>Cannot remove this column, because it is part of an expression: {0} = {1}.</value></data>
  <data name="DataRow_RowInsertTwice" xml:space="preserve"><value>The rowOrder value={0} has been found twice for table named '{1}'.</value></data>
  <data name="Xml_ElementTypeNotFound" xml:space="preserve"><value>Cannot find ElementType name='{0}'.</value></data>
+  <data name="ADP_DbProviderFactories_InvariantNameNotFound" xml:space="preserve"><value>The specified invariant name '{0}' wasn't found in the list of registered .Net Framework Data Providers.</value></data>


.NET Framework Data Providers -> .NET Data Providers (applies to all strings in this list apparently).

Gotcha, left over from the error messages from NetFX.

divega

Looking good with some minor comments.

@ajcvickers any chance that you can review?

divega · 2017-12-01T10:59:46Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+        private const string InvariantName = "InvariantName";
+        private const string Name = "Name";
+        private const string Description = "Description";
+        private const string ProviderGroup = "DbProviderFactories";


Nit: all these except AssemblyQualifiedName can be moved into GetFactoryClasses, correct?

I would probably put something in the name to indicate which ones are column names, otherwise they could be defined as nameof() themselves, although that might look a bit weird.

I'll rename the fields, good idea. I kept them together as it otherwise is a bit scattered. Now everything is discoverable in fixed locations (as one const indeed needs to be outside the method. )

divega · 2017-12-01T11:00:14Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+        private const string Name = "Name";
+        private const string Description = "Description";
+        private const string ProviderGroup = "DbProviderFactories";
+        private const string Instance = "Instance";


Nit: perhaps just use nameof for this one?

Didn't know that worked, but it does indeed Instance = nameof(Instance)

divega · 2017-12-01T11:03:27Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+
+namespace System.Data.Common
+{
+    public static partial class DbProviderFactories


Add xml docs? The .NET Framework ones should be a good starting point.

I thought those weren't required? (as they're added elsewhere?) That's why I didn't add them

Not sure. @karelz, @saurabh500?

Looking at random other classes in this solution, none have xml doc comments.

divega · 2017-12-01T11:09:08Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+        {
+            ADP.CheckArgumentNull(providerRow, nameof(providerRow));
+
+            DataColumn assemblyQualifiedNameColumn = providerRow.Table.Columns[AssemblyQualifiedName];


IIRC @saurabh500 does not like var, but I don't like not using var when it is possible and clear, so I will call it out anyway 😸 Not sure if there are general guidelines for new code in corefx. @karelz?

Guidelines say afaik that 'var' is discouraged, but have to look it up.

Guidelines here: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md point 10 says only about when not to use var, but it doesn't say use var when appropriate. There are many other cases in the class where var is an option. I didn't use that as the original code didn't and altering just for the sake of changing is also a bit much ;)

divega · 2017-12-01T11:27:35Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                // Deferred registration, do checks now on the type specified and register instance in storage.
+                // Even in the case of throwOnError being false, this will throw when an exception occurs checking the registered type as the user has to be notified the 
+                // registration is invalid, even though the registration is there.
+                Type providerType = Type.GetType(registration.FactoryTypeAssemblyQualifiedName);


Not a big deal, but my first thought when I saw the ProviderRegistration constructor is that it could validate that either the instance of the AQN are not null. If the instance is passed, the only case in which we may need to obtain the AQN again is GetProviderClasses, which should be used infrequently.

FWIW, even if we did that, this code would still be safe without a null check.

divega · 2017-12-01T11:29:56Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                throw ADP.Argument(SR.Format(SR.ADP_DbProviderFactories_NotAFactoryType, providerFactoryClass.FullName));
+            }
+
+            System.Reflection.FieldInfo providerInstance = providerFactoryClass.GetField(Instance, System.Reflection.BindingFlags.DeclaredOnly | System.Reflection.BindingFlags.Public | 


Nit: using?

Added using, cleaned up the BindingFlags statements too.

divega · 2017-12-01T11:35:07Z

src/System.Data.Common/System.Data.Common.sln

@@ -26,8 +26,8 @@ Global
 		Release|Any CPU = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(ProjectConfigurationPlatforms) = postSolution
-		{B473F77D-4168-4123-932A-E88020B768FA}.Debug|Any CPU.ActiveCfg = netstandard-Debug|Any CPU


That could be. Do things work if you revert it?

Another related concern: not sure if we need to do anything special with the fact that the rest of System.Data.Common is .NET Standard 2.0 but this new class isn't.

- 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

ajcvickers · 2017-12-01T17:43:24Z

@divega I wasn't planning to since I figured you and @saurabh500 have it covered, but I can add it to my list if you think it would be worthwhile.

divega · 2017-12-01T18:39:27Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+        private const string NameColumnName = "Name";
+        private const string DescriptionColumnName = "Description";
+        private const string ProviderGroupColumnName = "DbProviderFactories";
+        private const string Instance = nameof(Instance);


What I had in mind for this one was to just inline it in GetFactoryInstance (I believe it is the only place where it is used), but regardless, now I think the recursive nameof() trick is not worth it. I would just use a string (either here or directly in the call to GetFiled. If you keep the const, perhaps rename me to InstanceFieldName.

They're all used once except AssemblyQualifiedNameColumName, which is used 3 times. Having that one as constant and the rest as literal string is a bit awkward too. But in any case, I don't really see any harm having them as constants up top, it's the same in the NetFX code. I've reverted the nameof() change.

divega · 2017-12-01T20:55:04Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                // Deferred registration, do checks now on the type specified and register instance in storage.
+                // Even in the case of throwOnError being false, this will throw when an exception occurs checking the registered type as the user has to be notified the 
+                // registration is invalid, even though the registration is there.
+                Type providerType = Type.GetType(registration.FactoryTypeAssemblyQualifiedName);


I don't feel strongly about this but thinking about it again, it seems that doing the validation on the constructor provides little value. E.g. there is still a default constructor that allows to create an invalid ProviderRegistration, and the validation hast to be performed elsewhere anyway.

I still believe we don't need to eagerly store the assembly qualified name in the ProviderRegistration when the instance is provided (we can compute it on the fly on GetFactoryClasses), but I don't feel super strongly about that either.

divega · 2017-12-01T20:59:13Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                {
+                    throw ADP.Argument(SR.Format(SR.ADP_DbProviderFactories_FactoryNotLoadable, registration.FactoryTypeAssemblyQualifiedName));
+                }
+                toReturn = GetFactoryInstance(providerType);


Nit: It seems the 6 lines above are repeated in GetFactory(providerRow), can extract another private method?

It's a clone indeed. I'll go with GetProviderTypeFromTypeName(aqn) and it will cover these lines, and always return a value (type instance) or throw the exception mentioned above and move the call inlined with getfactoryinstance call

divega · 2017-12-01T20:59:41Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                    throw ADP.Argument(SR.Format(SR.ADP_DbProviderFactories_FactoryNotLoadable, registration.FactoryTypeAssemblyQualifiedName));
+                }
+                toReturn = GetFactoryInstance(providerType);
+                RegisterFactory(providerInvariantName, toReturn);


Do we need to re-register here?

It's calling that method to simply re-use the mechanics used to register a factory, like overwriting an existing registration etc, as the current registration is a deferred one in that location. At that point it's the same situation as when a user had instantiated the factory manually and wants to register the factory. I didn't want to scatter manipulation of a registration object in multiple places.

Makes sense now that you explained it to me. It may be just me, but perhaps a code comment would help.

- 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

divega

LGTM with a few nits that you may want to address. Nice test coverage.

divega · 2017-12-02T14:46:44Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                throw ADP.Argument(SR.ADP_DbProviderFactories_NoAssemblyQualifiedName);
+            }
+
+            return DbProviderFactories.GetFactoryInstance(DbProviderFactories.GetProviderTypeFromTypeName(assemblyQualifiedName));


Nit: I would leave the class name out. That would make the line shorter and more readable IMO.

divega · 2017-12-02T14:58:44Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+
+        public static bool UnregisterFactory(string providerInvariantName)
+        {
+            return !string.IsNullOrWhiteSpace(providerInvariantName) && _registeredFactories.TryRemove(providerInvariantName, out ProviderRegistration providerRegistration);


Nit: break the line on && and/or use out parameter discard syntax (just out _) to make this line shorter/easier to read.

divega · 2017-12-02T15:01:49Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+            }
+            else
+            {
+                if (string.IsNullOrWhiteSpace(providerInvariantName))


Nit: collapse into else if?

I hate 'else if' as it has an 'else' block without curly braces, which I find personally not a good choice, every block should have curly braces, even if it's just 1 statement.

divega · 2017-12-02T15:04:20Z

src/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

+                    return null;
+                }
+            }
+            bool wasRegistered = _registeredFactories.TryGetValue(providerInvariantName, out ProviderRegistration registration);


Nit: inline/remove wasRegistered variable if possible

This isn't possible as the 'registration' variable is declared at that line, so inlining it makes it go into the scope of the if true block, while it's used after that as well :) This can be solved then by declaring a line with the registration variable and that is mitigating the whole inlining act ;)

divega · 2017-12-02T15:11:25Z

src/System.Data.Common/tests/Configurations.props

@@ -2,6 +2,7 @@
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <BuildConfigurations>
+	  netcoreapp;


Nit: indent

tabs vs. spaces... about that... haha no just kidding ;) Fixed.

saurabh500 · 2017-12-04T21:31:32Z

@dotnet-bot Test UWP CoreCLR x64 Debug Build please

FransBouma · 2017-12-05T07:38:14Z

Thanks for merging! :)

MarkusFritschi · 2017-12-05T15:40:54Z

Thanks to you and everybody envolved in this project. Great job!

karelz · 2017-12-05T16:49:07Z

Indeed, great job @FransBouma! Thank you!
Also thanks @divega @saurabh500 @ajcvickers for guidance and helping to make it happen.

@saurabh500

* Implements dotnet/corefx#19826: DbConnection doesn't offer an internal property ProviderFactory * Added test for dotnet/corefx#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 dotnet/corefx#4571, dotnet/corefx#19825 and dotnet/corefx#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: dotnet/corefx#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 dotnet/corefx#19885 (review) * Almost done implementation using DataTable internal storage. Refactoring now to concurrentdictionary * Final work for dotnet/corefx#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 Commit migrated from dotnet/corefx@7caa955

FransBouma added 23 commits May 17, 2017 12:24

Implements #19826: DbConnection doesn't offer an internal property Pr…

5fe21a9

…oviderFactory

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

7074007

Removed writeline

931f54c

Added comment to illustrate the purpose of the property which current…

7102d7c

…ly is dead code.

Implementation of DbProviderFactories, ported from NetFx and adjusted…

2edb985

… to CoreFx See #4571, #19825 and #19826

Update for project files to get a netcoreapp test set

ff76cef

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

03b9f72

Added tests for DbProviderFactories.

bc5badf

Merge branch 'master' of https://github.com/dotnet/corefx into DbConn…

8516a0b

…ection_ProviderFactory_19826

Better subclass testing added and more tests added

6415bc6

Moved all hard-coded strings to SR/Strings.resx. Added test for facto…

9473284

…ry retrieval using a connection

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

b7d78cf

Comment correction for bad English

920a0ba

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

c37583e

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

9168ac9

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

bc836c5

Merge remote-tracking branch 'dotnet/master' into DbConnection_Provid…

b83c67e

…erFactory_19826

Almost done implementation using DataTable internal storage. Refactor…

f84c3a8

…ing now to concurrentdictionary

Final work for #20903

356f393

Corrections of the implementation->

bf607f2

- 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

00cfd77

Merge remote-tracking branch 'dotnet/master' into DbConnection_Provid…

4b1fdcf

…erFactory_19826

divega requested review from ajcvickers, divega and saurabh500 November 21, 2017 16:07

karelz added the area-System.Data label Nov 21, 2017

karelz assigned FransBouma, divega and ajcvickers Nov 21, 2017

saurabh500 approved these changes Nov 29, 2017

View reviewed changes

FransBouma added 2 commits December 1, 2017 10:35

Corrections made to code by request of @saurabh500

8872282

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

89f73b2

Again correcting component elements. These are automatically inserted…

d19cf6a

… so if they're back again, I can't remove them

FransBouma commented Dec 1, 2017

View reviewed changes

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

df9d929

divega reviewed Dec 1, 2017

View reviewed changes

More @divega requested changes

6868673

- 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

divega approved these changes Dec 2, 2017

View reviewed changes

Last nit removal for @divega

8d9834f

saurabh500 merged commit 7caa955 into dotnet:master Dec 5, 2017

karelz added this to the 2.1.0 milestone Dec 5, 2017

FransBouma deleted the DbConnection_ProviderFactory_19826 branch December 5, 2017 18:22

dermeister0 mentioned this pull request Jun 21, 2018

Implement DbProviderFactory loading for .NET Standard saritasa-nest/saritasa-dotnet-tools#52

Open

nd-srogers mentioned this pull request Jan 31, 2020

DbProviderFactories not found in System.Data.Common dotnet/runtime#26229

Closed

Implementation of DbProviderFactories. #25410

Implementation of DbProviderFactories. #25410

Conversation

FransBouma commented Nov 21, 2017

saurabh500 commented Nov 27, 2017

FransBouma commented Nov 29, 2017

saurabh500 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FransBouma commented Dec 1, 2017 • edited Loading

FransBouma commented Dec 1, 2017

FransBouma commented Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

saurabh500 Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divega commented Dec 1, 2017

divega left a comment

Choose a reason for hiding this comment

divega Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FransBouma Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 commented Dec 4, 2017

FransBouma commented Dec 5, 2017

MarkusFritschi commented Dec 5, 2017

karelz commented Dec 5, 2017

FransBouma commented Dec 1, 2017 •

edited

Loading

FransBouma commented Dec 1, 2017 •

edited

Loading

saurabh500 Dec 1, 2017 •

edited

Loading

divega Nov 30, 2017 •

edited

Loading

FransBouma Dec 2, 2017 •

edited

Loading