SqlParameter Scale and Precison properties should override DbParameter implementations

[AlgorithmsAreCool]

Background

In the desktop framework, The System.Data namespace included a set of interfaces from the .net 1.1 era. DbParameter implemented the IDbParameter interface which included the Scale and Precision properties. When System.Data was added to corefx, the interfaces were dropped since the abstract classes had been preferred since .net 2.0 and forward.

@mgravell noticed ( dotnet/runtime#14526 ) however, that when the interfaces were removed, the Scale and Precision properties were broken. For some reason they used the interface to call the child implementation instead of traditional class inheritance. You can see the old implementation here : http://referencesource.microsoft.com/#System.Data/System/Data/Common/DBParameter.cs,68

The net result is that calling Scale or Precision on a SqlParameter object casted to a DbParameter is a no-op. This is the problem.

Motivation

The are 3 motivators for this change.

1. Maintaining perceived behavior from the desktop framework
2. Implementing the path of least surprise
3. Enabling DB agnostic code (very important)

The first point being that the following 2 statements are equivalent in .net 4.5

SqlParameter param = new SqlParameter();
param.Scale = 1;

and

DbParameter param = new SqlParameter();
param.Scale = 1;

Whereas they are not currently equivalent in corefx. The 2nd statement will not update the value of Scale. The same behavior applies to the Precision property.

The second motivation is that this kind of behavior difference is very unexpected. Most developers would assume that the behavior of setting or reading a property would not change due to inheritance.

Lastly if you wanted to write DB agnostic code that only used the Db* classes, you would be unable to set these parameters without a test and cast special case down to SqlParameter.

Proposed Change

Simply alter SqlParameter to make Scale and Precision override the DbParameter implementations.

Risks

I can see two possibly breaking changes here:

1. Calling Scale and Precision from DbParameter now returns the SqlParameter implementation
2. Scale and Precision on SqlParameter are now virtual and therefore overridable

The first one, is actually the existing behavior under .net. The SqlParameter members implicitly implemented the interface and get called from the parent (DbParameter) by it casting itself to the interface. The net result was the child implementation getting called.

The second is not actually observable since SqlParameter is sealed and therefore no child classes can break.

Now lets say that a 3rd party library was inheriting from DbParameter. They would more than likely just override Scale and Precision directly in .net. In core they could only do that, so this should not present a breaking change to existing or ported libraries.

So i think that this is a safe change to make unless someone wrote code against corefx that was counting on DbParameter.Scale always being zero regardless of what they assigned it.

Therefore I believe this change has minimal risk in breaking code.

Pull Request

The changes are implemented here dotnet/corefx#4902

[AlgorithmsAreCool]

cc @saurabh500

[AlgorithmsAreCool]

I noticed the needs work designation, were there any specific concerns that i could tidy up?

[saurabh500]

Reviewed the changes in dotnet/corefx#4902 with ApiCompat and the changes are not breaking. Merged changes. Closing this review