Runtime: Look into removing DbDataReaderExtensions.Facade.cs

[roji]

There are currently two DbDataReaderExtensions classes under System.Data.Common, defining GetColumnSchema():

1. DbDataReaderExtensions.cs. GetColumnSchema() checks if the reader implements IDbColumnSchemaGenerator and calls into that, otherwise throws.
2. DbDataReaderExtensions.Facade.cs. GetColumnSchema()calls the traditional GetSchemaTable()` and translates to the new DbColumn (so a backwards compat shim). This file isn't referenced in the csproj.

The second seems to be a leftover from .NET Framework, since in .NET Core pre-2.0 DataTable didn't exist. However, I'm not sure if there's any sense in keeping this in the corefx repo today.

/cc @divega

[bricelam]

My vote is to combine the two—first check for IDbColumnSchemaGenerator then fallback to GetSchemaTable()

[Wraith2]

My vote is to combine the two—first check for IDbColumnSchemaGenerator then fallback to GetSchemaTable()

It's a good idea but I don't think it will work. IDbColumnSchemaGenerator was introduced in desktop 4.7.1 and core 2.0 so for any version equal or greater it'll use the interface, for any version lower the interface definition won't exist and you'll get a type load failure rather than fallback behaviour.

What is the minimum runtime for desktop and core that we're supporting and do they need this fallback?

[divega]

@Wraith2 as far as I can see, the base class does not implement IDbColumnSchemaGenerator, so unless a provider was updated to implement it on its data readers, the fallback should help. That said, I may be missing something.

[Wraith2]

DbDataReader doesn't. SqlDataReader does. In builds which contain the interface we can combine the two implementations as @bricelam suggested.

If we ever need to build something that targets an old version of the framework which doesn't contain the interface from the current master we can't use the version which tests the interface, we'd need the old option only. I know that some code from core can move upstream to other internal builds so I can't say whether this old version may still be needed.

[divega]

Some additional information:

• System.Data.Common.csproj used to include DbDataReaderExtensions.Facade.cs only for old .NET Framework versions that didn’t implement the functionality.
• Commit
  https://github.com/dotnet/corefx/commit/6a0ecec8279504c03433f689e4a7cf6cd63da27c#diff-7cf19ece383aed06b9001e91b02ce816 removed the reference to the file but left the file hanging there
• We haven’t shipped System.Data.Common as a package since 2016
• @Wraith2 makes a good point that if we ever need to do this again, or if this code “flows” to some internal build, we could need this version of the code. However things don’t seem to be setup in master for that. I will follow up to confirm that this is supposed to be the permanent state of things

[Wraith2]

I think a sensible next move would be to merge the façade functionality into the version of the file we compile and keep the old file around with a note somewhere in the header explaining that it isn't compiled and why, Then if we do get resolution from other users of the codebase that it's unused to can simply remove it.

[roji]

@divega seems like it would be good to get some guidance here, i.e. exactly which TFMs are supposed to be built with this codebase moving forward. We may get official confirmation that anything under net471/netstandard20 is off the table, in which case it seems safe to simply clean this up.

[ericstj]

System.Data.Common was absorbed into netstandard 2.0 and then with .NET 4.7.1 netstandard2.0 support was absorbed back inbox. As such, any changes to this API / implementation that need to make it into desktop should be made in the desktop codebase.

You shouldn't think of System.Data.Common as a package anymore. It's API is part of .NETStandard.

[roji]

~@ericstj thanks for chiming in. We currently seem to be stuck with this PR's tests failing because they can't find the newly-introduced extensions methods (on netfx and uwp). Are the tests supposed to be built and run only on netcoreapp? Can you confirm that it makes sense to add the extension methods in question without restricting them to netcoreapp?~

Apologies, I confused this PR with dotnet/corefx#36123.

The question in the context of this PR, is what TFMs is the current corefx codebase expected to build and run on? Specifically, is it permitted to assume that we have IDbColumnSchemaGenerator, which was only introduced in .NET Standard 2.0/.NET Framework 4.7.1/NET Core 1.0? Or do we need to take into account TFMs where this interface does not exist?

[ericstj]

System.Data.Common.dll is only building for netcoreapp and UAP (latest versions) :
https://github.com/dotnet/corefx/blob/483be9a348d9ea27ab36a74d5887981c8a7f79f8/src/System.Data.Common/ref/Configurations.props#L4-L5
https://github.com/dotnet/corefx/blob/483be9a348d9ea27ab36a74d5887981c8a7f79f8/src/System.Data.Common/src/Configurations.props#L3-L6

All of these have IDbColumnSchemaGenerator since they all support netstandard2.0.

Note that you're only shipping new API/implementation to these frameworks. If you wanted to expose new API to netstandard or desktop you'd need a new assembly for that (since S.D.Common was pulled inbox on desktop and all its types were pulled into netstandard.dll).

[roji]

Thanks @ericstj, I think this confirms that IDbColumnSchemaGenerator will always exist as far we're concerned, and it should be safe to merge the façade into the DbDataReaderExtensions.cs and delete the other file.