Add GetFieldValue(Async)<T> support for XmlReader, TextReader, Stream

[Wraith2]

Fixes: #27

Before M.D.SqlClient was created from the netfx and netcore versions of S.D.SqlClient i had added support for GetFieldValue<XmlReader> in netcore. This PR replaces that version of the code with a more appropriate version that correctly implements SequentialAccess behaviour (by returning a sequential reader immediately instad of buffering) and extends the support for GetFieldValue and it's async version to include Stream TextReader and XmlReader parameter types.

I had previously raised questions about what the null behaviour should be for these types. Currently the behaviour when requesting the value of a stream column which has a null value in the current row is that an empty object of the correct type will be returned. I think this behaviour is in place to make stream usage easier for consumers and have not changed it. If an error were thrown instead it would be more difficult to write a clear consumer of the api. The IsDbNull(async) will correctly return true for null row values.

It is not clear that this support is needed in netfx builds at this time. Eventually netcore and netfx codebases will be merged and all capabilities will be equal in all versions (depending on platform support), at that time this support will reach netfx. Porting this change to netfx is possible but time consuming due to the different implementations of async and the many small but important differences in core classes between the two.

All existing tests pass. New tests for the specific types (Stream, XmlReader, TextReader), their accessors (GetXmlReader, GetFieldValue<XmlReader>, GetFieldValueAsync<XmlReader> etc) and behaviours (Default, SequentialAccess) have been added. The new tests are in a new class because I found that the existing tests test many things in a single method making debugging them more difficult that I would like, the new tests are slightly less complex per test so it's clearer what aspect of behaviour has changed when they fail.

/cc @roji as requester, @ErikEJ as interested party

[DavoudEshtehari]

This change needs to update docs and includes the new supported types with more samples. I'm not sure if it's going to be included with this PR or in a separate PR! My preference is to wrap it up here.

CC @cheenamalhotra

[Wraith2]

I added the newly supported types to the tables for the two affected methods. The streaming docs were already linked so I don't think anything more is needed.

[DavoudEshtehari]

Greate!
Since those types are not fully supported in SqlClient with different frameworks, could you specify that(maybe adding in a Note box under the supported types table)? In addition, we'd better mention the empty object behavior as you've mentioned in your comment.

I had previously raised questions about what the null behaviour should be for these types. Currently the behaviour when requesting the value of a stream column which has a null value in the current row is that an empty object of the correct type will be returned. I think this behaviour is in place to make stream usage easier for consumers and have not changed it. If an error were thrown instead it would be more difficult to write a clear consumer of the api. The IsDbNull(async) will correctly return true for null row values.

[Wraith2]

Since those types are not fully supported by all frameworks,

They aren't? FileStreams aren't supported on unix but binary data in a database field is entirely supported, same with xml and text.

The null object behaviour is identical to that of GetXmlReader, GetTextReader and GetStream, we could link to those pages but i don't think I'd want to duplicate the docs for that.

[DavoudEshtehari]

Updated that comment!

I meant they're not fully supported in SqlClient yet:

It is not clear that this support is needed in netfx builds at this time. Eventually netcore and netfx codebases will be merged and all capabilities will be equal in all versions (depending on platform support), at that time this support will reach netfx. Porting this change to netfx is possible but time consuming due to the different implementations of async and the many small but important differences in core classes between the two.

[Wraith2]

Oh, I see. That needs an official decision on whether this feature is wanted on netfx. If it is needed/wanted on netfx then it can probably be implemented as long as the changes in SqlDataReader are thoroughly reviewed, they're pretty low level which means if they're wrong there would be hard to reproduce errors.

[Wraith2]

@David-Engel @cheenamalhotra this is waiting on a decision on whether this functionality is needed in netfx.
The original request comes from EF Core which does not support netfx. The implementation detail is intricate and I feel that if it is needed in netfx i'd like to get it validated through review before I try to port it.

[cheenamalhotra]

We need to keep NetFx and NetCore as much close to each other as possible to aid merging codebases. I agree there are differences in design, but if we don't port things over, we are increasing that gap even further and that increases possibility of issues when we do port them over later or someday merge these classes.

e.g. there were issues in PR #1084 that is not related to any of the changes that were made in the related PR in NetCore but it could have required thorough debugging and time investment to investigate failures to figure out why things weren't porting over successfully due to some other existing design gaps.

As we always give equal importance to NetFx for all the new features that are added to the driver, same should be done for anything else. Our ultimate goal is to merge these codebases, and so we need to give importance to maintain design parity as much as possible.

[Wraith2]

Ok, so netfx wanted. In which case I'll wait for review on the SqlDataReader changes which are the intricate part.

[Wraith2]

Any update on this? The further from doing the work it gets the less likely I am to be able to remember all the reasons for changes if they're not clear from the code.

[cheenamalhotra]

@Wraith2 We'll start reviewing it soon, please resolve conflicts to prepare for review.
Thanks! :)

[Wraith2]

Done, but as usual there wouldn't be conflicts to resolve if it hadn't been waiting here so long.

[cheenamalhotra]

Let's make changes for NetFx too, the changes in NetCore look good to me 👍🏻

[cheenamalhotra]

Hi @roji

Just wanted to grab your attention on this PR, the changes look promising as per requested. Thanks to @Wraith2 :)
Let us know your feedback on the same. We're considering this support to be included in the next preview.

[Wraith2]

If you want it for the next preview what are the timescales on that? I've got a big rebase to do and then I've got to work these changes back into netfx which has some different parts since data reader isn't synced. Plus, you know, my day job.

[cheenamalhotra]

Please ensure your branch has all updates from dotnet/main, we've stopped building with .NET Core 2.1 now - the PR was merged just yesterday. But if that's a warning, it doesn't mean anything, you can ignore it. You can compare output with some other test u know works, with the failing tests, because the tests hang up, so no output probably means there's a hung up test.

[Wraith2]

Fun. Nothing quite like debugging against a moving target. I've rebased and rebuilt now and I still get the same thing. Something in the tests is building against 2.1 and it's being inappropriately used in the net461 build.
Here's the build batch file, note that there's a clean at the start to make sure nothing gets used between different build targets.

@echo off

set netfx=net461
set config=Debug
set platform=AnyCpu

msbuild -t:clean
msbuild -p:BuildNetFx=true -p:Configuration=%config% -p:Platform=%platform%
msbuild -t:BuildTestsNetFx -p:TargetNetFxVersion=%netfx%

dotnet test "src\Microsoft.Data.SqlClient\tests\ManualTests\Microsoft.Data.SqlClient.ManualTesting.Tests.csproj" -p:Platform="%platform%" -p:Configuration="%config%" -p:TestTargetOS="Windowsnetfx" --no-build -v n --filter "GetFieldValueAsync_OfStream"

I've identified where the error is but as you hopefully remember it's slow going when you have to remote attach the debugger to the test to look at what's going on.

[cheenamalhotra]

Just to let you know, there are quite many PRs queued today, so it builds may not run soon enough.

[Wraith2]

That's fine, I've got a local repro of the hang and haven't worked out the cause yet. i just pushed to make sure the rebase was done properly.

[Wraith2]

Actually the message has now changed to use 3.1 and not 2.1, it's still an inappropriate dll to use though.

Test run detected DLL(s) which were built for different framework and platform versions. Following DLL(s) do not match current settings, which are .NETFramework,Version=v4.6.1 framework and X86 platform.
ManualTests.dll is built for Framework .NETCoreApp,Version=v3.1 and Platform AnyCPU.
Go to https://aka.ms/tp/vstest/multitargetingdoc?view=vs-2019 for more details on managing these settings.

[Wraith2]

Woo!