Added GetFieldValue test to retrieve Stream

[YohDeadfall]

Fixes #144

[roji]

Shouldn't these be in the upstream tests? Or is the plan to remove these once they're implemented there?

Oops, didn't realize which repo I'm in 🤣

@YohDeadfall note that nobody has discussed making this a global requirement across all ADO.NET providers - this is just an idea we had in Npgsql. I'm not sure it belongs here until that happens.

[YohDeadfall]

What do you mean? As I understand the base class made to share tests across provides and all of them should support the behavior.

[YohDeadfall]

As I understood Wraith2, this is supported by SqlClient and by Npgsql now. Moreover, getting TextReader is a standard thing, so why not for Stream?

[roji]

What do you mean? As I understand the base class made to share tests across provides and all of them should support the behavior.

I do agree this is a good to have as a standard provider behavior - I'm just not sure this has been properly discussed etc.

As I understood Wraith2, this is supported by SqlClient and by Npgsql now. Moreover, getting TextReader is a standard thing, so why not for Stream?

I tried running the following code with 2.1.0-preview1.20235.1:

using var conn = new SqlConnection("...");
conn.Open();
using var cmd = new SqlCommand("SELECT @p", conn);
cmd.Parameters.AddWithValue("p", new byte[] { 1, 2, 3 });
using var reader = cmd.ExecuteReader();
reader.Read();
_ = reader.GetFieldValue<Stream>(0);

This gives me the following exception message:

Unable to cast object of type 'System.Byte[]' to type 'System.IO.Stream'.

Maybe @Wraith2 can provide some info...

[Wraith2]

I think the expectation in SqlClient is that if you want a stream you should call GetStream, no need for generics. There is logic in GetFieldValue which checks for XmlReader which means that you don't have to call GetXmlReader so there's a good argument that behaviour should be consistant for these types. I can't see any reason that we couldn't add in Stream as a valid generic parameter type for GetFieldValue(Async) we just haven't had reason to do so yet.

[roji]

@Wraith2 IIRC we discussed this together at some point, and the reason for allowing Stream and TextReader as generic arguments to GetFieldValue was that there aren't async methods for these (i.e. GetStreamAsync, GetTextReaderAsync). Rather than adding these new APIs, it was suggested to simply allow them to be passed to GetFieldValueAsync (and therefore also to GetFieldValue for consistency). I think it was even you who proposed this 😄

[Wraith2]

I remember that. The work just hasn't been done for Stream, I was probaby distracted. Throw an issue into the sqlclient repo and to track i'll pick it up when I can.

[roji]

Thanks @Wraith2, no worries obviously. I've opened dotnet/SqlClient#732 to track, the original discussion between us on this starts in dotnet/runtime#28596 (comment).

[bgrainger]

Thanks @YohDeadfall!

I made a small change in 2efb473 to test for the database provider's standard exception type; this allows SqlClient to keep throwing SqlNullValueException (instead of InvalidCastException) for these methods.

[bricelam]

I think I started this weirdness. While doing some initial investigation of dotnet/efcore#19991, I noticed that not everything was possible via GetFieldValue<T> which would require unnecessary special handling of these types.

[bricelam]

I think I filed dotnet/runtime#26756 with the idea of allowing users to write a value converter that goes from XmlReader to XmlDocument/XDocument, but I don't think I ever followed up on that after it was fixed...