Enable BeginExecuteReader methods #33660

galakt · 2018-11-22T06:39:19Z

According to #17126.
Methods have been made public,
overloads have been added,
single manual test on success has been added.
This is my first PR to corefx.

dnfclas · 2018-11-22T06:39:30Z

All CLA requirements met.

Wraith2 · 2018-11-22T21:15:52Z

Looks pretty simple. You'll want to CC in a few people to get their attention since this is their area, @afsanehr, @keeratsingh, @David-Engel.

saurabh500 · 2018-11-26T23:11:54Z

src/System.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/BeginExecReaderAsyncTest.cs

+                IAsyncResult result = command.BeginExecuteReader();
+                while (!result.IsCompleted)
+                {
+                    System.Threading.Thread.Sleep(100);


This test is supposed to throw InvalidOperationException when The name/value pair "Asynchronous Processing=true" was not included within the connection string defining the connection for this SqlCommand.
Doc Link

~~Can you check for that condition? If the connection string that is passed in doesn't have Asynchronous Processing set to true, then the test should add the parameter to the connection string.~~

Instead of testing BeginExecuteReader with a Sleep statement, could you test an overload which takes in a callback, which is called when the execution finishes? This way you can test EndExecuteReader in the same function and it could provide more branch coverage.
Alternatively, could you add a test which tests the callback paths?

And I read more documentation

https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=netframework-4.7.2

Please ignore the struck out comment above.

I was confused with "Asynchronous Processing" too. I have added test with callback.

keeratsingh · 2018-11-27T22:10:54Z

@dotnet test Windows x64 Debug Build

saurabh500 · 2018-11-28T19:42:53Z

src/System.Data.SqlClient/ref/System.Data.SqlClient.cs

@@ -425,6 +425,11 @@ public sealed partial class SqlCommand : System.Data.Common.DbCommand, System.IC
        public System.IAsyncResult BeginExecuteXmlReader() { throw null; }
        public System.IAsyncResult BeginExecuteXmlReader(System.AsyncCallback callback, object stateObject) { throw null; }
        public System.Xml.XmlReader EndExecuteXmlReader(System.IAsyncResult asyncResult) { throw null; }
+        public System.IAsyncResult BeginExecuteReader() { throw null; }


@galakt These edits should be added to System.Data.SqlClient.NetCoreApp.cs and not the System.Data.SqlClient.cs file.
These new APIs should be exposed in netcoreapp specific build. Netstandard cannot be updated.

I really confused by this:

This is exposing APIs which are available on .NET Framework (see https://apisof.net/catalog/System.Data.SqlClient.SqlCommand)

How i can break Netstandard while AFAIK Netstandard have no SqlClient mentions (https://github.com/dotnet/standard/tree/master/netstandard/ref)

What difference of this PR and Enable BeginExecuteNonQuery functions in .NET Core #26016

Btw, i have moved ref changes to the NetCoreApp.cs

Thanks @galakt
@danmosemsft My understanding is that any new APIs added even to existing types, should be moved to netcoreapp specific builds based on the conversations we had on this thread. #32245 (review)

Is this assumption correct? I guess we will need to update the baseline files to represent this.

@danmosemsft Can we finally resolve question about place for my Refs please?

I'm not sure we have been consistent - @ericstj?

@karelz Sorry, can you push this?

@saurabh500 @ericstj @danmosemsft Any updates on this?

SqlClient is not part of netstandard, so its APIs are not pinned by being in netstandard. You do however, need to consider support. If you add API to the reference assembly you should be sure it is supported everywhere that API can be used. So if you're exposing it in a netstandard2.0 reference assembly this means it needs to be supported in .NET 4.6.1 and later, .NETCoreApp 2.0 or later, UAP, etc (see https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). For UAP and NETCoreApp you can simply update the implementation assembly that is in the package, desktop is the limiting concern here since you cannot update that (and perhaps mono/Xamarin).

@saurabh500 If i understood correct, i should revert "move to NetCoreApp.cs" commit. Am i right?

@galakt Yes that is right.
@ericstj Thanks for the clarification.

saurabh500

Move the ref changes to the netcoreapp.cs file.

galakt · 2018-12-01T22:16:56Z

@dotnet test NETFX x86 Release Build

Wraith2 · 2018-12-05T22:36:01Z

SQL\AsyncTest\BeginExecReaderAsyncTest.cs(43,47): error CS1061: 'SqlCommand' does not contain a definition for 'BeginExecuteReader'

looks like you might need to add platform specific exceptions for the test(s).

galakt · 2018-12-05T23:45:37Z

SQL\AsyncTest\BeginExecReaderAsyncTest.cs(43,47): error CS1061: 'SqlCommand' does not contain a definition for 'BeginExecuteReader'

looks like you might need to add platform specific exceptions for the test(s).

@Wraith2 See no sense in any new changes, we still on stage of deciding where Refs must be (look at last review conversation)

This reverts commit 860ef7c.

…__ExecuteReader_methods

galakt · 2018-12-20T11:29:28Z

Reverted references move
Synced with changes from #34112

saurabh500 · 2018-12-20T18:39:55Z

Ready to merge after @afsanehr confirms a test pass.

AfsanehR-zz

Tests passing.

…der_methods Enable BeginExecuteReader methods Commit migrated from dotnet/corefx@fea701f

Galaktionov_v added 4 commits November 21, 2018 17:34

Enable *ExecuteReader methods

595a1f6

Fix return type

6064218

Fix ExecuteReaderAsync

4820e5f

Add test for BeginExecuteReader

31a4944

David-Engel added the area-System.Data.SqlClient label Nov 26, 2018

David-Engel requested review from AfsanehR-zz, keeratsingh and saurabh500 November 26, 2018 21:55

saurabh500 reviewed Nov 26, 2018

View reviewed changes

Add test with callback

d3440c0

saurabh500 approved these changes Nov 28, 2018 •

edited

Loading

View reviewed changes

saurabh500 reviewed Nov 28, 2018

View reviewed changes

saurabh500 suggested changes Nov 28, 2018

View reviewed changes

Move Api definition to NetCoreApp

860ef7c

karelz assigned galakt, keeratsingh and AfsanehR-zz Dec 19, 2018

Galaktionov_v added 3 commits December 20, 2018 12:06

Revert "Move Api definition to NetCoreApp"

6d286d3

This reverts commit 860ef7c.

Merge branch 'master' of https://github.com/dotnet/corefx into Enable…

e64f019

…__ExecuteReader_methods

Replace CheckConnStrSetupFact

d6a4710

saurabh500 approved these changes Dec 20, 2018

View reviewed changes

AfsanehR-zz approved these changes Dec 20, 2018

View reviewed changes

AfsanehR-zz merged commit fea701f into dotnet:master Dec 20, 2018

galakt deleted the Enable__ExecuteReader_methods branch December 21, 2018 09:16

karelz added this to the 3.0 milestone Dec 21, 2018

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#33660 from galakt/Enable__ExecuteRea…

6762fae

…der_methods Enable BeginExecuteReader methods Commit migrated from dotnet/corefx@fea701f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable BeginExecuteReader methods #33660

Enable BeginExecuteReader methods #33660

galakt commented Nov 22, 2018

dnfclas commented Nov 22, 2018 •

edited

Loading

Wraith2 commented Nov 22, 2018

saurabh500 Nov 26, 2018 •

edited

Loading

saurabh500 Nov 26, 2018

galakt Nov 27, 2018

keeratsingh commented Nov 27, 2018

saurabh500 Nov 28, 2018 •

edited

Loading

galakt Nov 29, 2018

saurabh500 Nov 30, 2018

galakt Dec 4, 2018

danmoseley Dec 4, 2018

galakt Dec 10, 2018

galakt Dec 18, 2018

ericstj Dec 19, 2018

galakt Dec 19, 2018

saurabh500 Dec 19, 2018

saurabh500 left a comment

galakt commented Dec 1, 2018

Wraith2 commented Dec 5, 2018

galakt commented Dec 5, 2018 •

edited

Loading

galakt commented Dec 20, 2018

saurabh500 commented Dec 20, 2018

AfsanehR-zz left a comment

Enable BeginExecuteReader methods #33660

Enable BeginExecuteReader methods #33660

Conversation

galakt commented Nov 22, 2018

dnfclas commented Nov 22, 2018 • edited Loading

Wraith2 commented Nov 22, 2018

saurabh500 Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keeratsingh commented Nov 27, 2018

saurabh500 Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 left a comment

Choose a reason for hiding this comment

galakt commented Dec 1, 2018

Wraith2 commented Dec 5, 2018

galakt commented Dec 5, 2018 • edited Loading

galakt commented Dec 20, 2018

saurabh500 commented Dec 20, 2018

AfsanehR-zz left a comment

Choose a reason for hiding this comment

dnfclas commented Nov 22, 2018 •

edited

Loading

saurabh500 Nov 26, 2018 •

edited

Loading

saurabh500 Nov 28, 2018 •

edited

Loading

galakt commented Dec 5, 2018 •

edited

Loading