SqlDataReader.GetFieldValue{,Async}() should work for Stream and TextReader

[roji]

Following #30958, where @Wraith2 added the ability to retrieve an XmlReader via SqlDataReader.GetFieldValue() and its async counterpart, we should do the same for Stream and TextReader. These can already be retrieved via GetStream() and GetTextReader() respectively.

The main motivation behind this would be to allow getting Reader/TextReader asynchronously without introducing new GetReaderAsync() and GetTextReaderAsync() methods. See https://github.com/dotnet/corefx/issues/35012#issuecomment-484440830 and below for previous discussion.

[roji]

/cc @Wraith2 @divega @ajcvickers @saurabh500 @bricelam @bgrainger

@saurabh500 do you see any issue/objection with this?

[Wraith2]

@bgrainger has pointed out in one of the linked issues elsewhere that there is some slightly unexpected (but deliberate) behaviour when using the current sync stream methods, GetStream, GetTextReader and GetXmlReader all handle database NULL values by detecting it and returning an empty stream. Other methods of accessing the same fields will detect null and throw an appropriate exception.

Should GetFieldValue(Async)<T> for stream types emulate the current behaviour of returning a constructed empty stream or should they throw null as GetString or GetBytes would do?

I think it is up to the developer to ensure that their queries return sensible data and that if null can be present in the return from a query it should be explicitly handled by the consumer and not hidden by the ADO library. I'm in favour of throwing an exception if a stream over null data is requested through GetFieldValue(Async)

[saurabh500]

@roji I dont have any objections with this. This is a step in the right direction.

@Wraith2 , good that you brought up the behavior changes on the thread. I need to read a bit more about the nuances.

[divega]

As recently announced in the .NET Blog, focus on new SqlClient features an improvements is moving to the new Microsoft.Data.SqlClient package. For this reason, we are moving this issue to the new repo at https://github.com/dotnet/SqlClient. We will still use https://github.com/dotnet/corefx to track issues on other providers like System.Data.Odbc and System.Data.OleDB, and general ADO.NET and .NET data access issues.

[Wraith2]

@roji i've got this working on netcore but because of the different netcore and netfx codebases it's more difficult to get it working on netfx. Since this would primarily be useful from EF and as far as a know modern EF only works on core or net5 onwards is there a need to implement on netfx immediately? It will eventually be implemented through the merging of the codebases.

[roji]

(sorry for the slow response, back from vacation)

I don't see any particular problem with deferring this for netfx (apart from the general undesirability of having a behavior discrepancy between the versions of the driver); this just adds a new capability which didn't exist before, and it's unlikely anyone's going to be porting new code using it from netcore to netfx...

[Wraith2]

Looking at how it's all structured a related question occurred to me. What should happen for GetFieldValue<int?> if it were supported? Would we map language null to db null in the case and allow users to avoid the IsDbNull check? Would that be useful or confusing to users? Do other providers support that sort of thing?

[roji]

Yeah, GetFieldValue<int?> is something we actually implemented in Npgsql to make things slightly easier - even though it doesn't fully respect the DBNull/null distinction. Unfortunately, the way nullable reference types work, it would be impossible for an ADO.NET provider to distinguish between GetFieldValue<string> (which should throw on database null) and GetFieldValue<string?> which shouldn't.

[Wraith2]

streams strings and any other reference type such as UDT's wouldn't work out at all. As you say.
The standard value types could have nullable versions added. If npgsql does it then it might be worth opening an issue for SqlClient to do the same and possibly ask if mysql and sqlite are interested in the same extension.