Remarks for ExecuteNonQuery additional reasons to return -1

[joelpaula]

Summary

Added a remark to ExecuteNonQuery for an additional reason for it to return -1 - the connection's NO COUNT is ON.

[rpetrusha]

@divega, can you review this PR, please?

[divega]

Copying the proposed change (text added in bold) here to make the review easier :

For UPDATE, INSERT, and DELETE statements, the return value is the number of rows affected by the command. When a trigger exists on a table being inserted or updated, the return value includes the number of rows affected by both the insert or update operation and the number of rows affected by the trigger or triggers. For all other types of statements, the return value is -1. If a rollback occurs, the return value is also -1. If the statement sets NOCOUNT as ON (a SET NOCOUNT ON statement is part of the statement) or the connection is currently with the NOCOUNT mode ON, the return value will also be -1.

1. Unfortunately, I don't think the API documentation for SqlCommand recognizes the fact that multiple statements concatenated with ";" are supported, so we pretend that they are the same thing. But inside the parenthesis it becomes evident that we are using the word "statement" to mean two different things: the content of the CommandText property, and an individual T-SQL statement inside that CommandText.

2. Also, the sentence before the parenthesis and inside the parenthesis seem redundant. I would leave one or the other.

3. I am not sure that implying that any time SET NOCOUNT ON is used the return value will be -1 is going to help in all cases. SET NOCOUNT ON is recommend in the body of triggers or stored procedures to avoid impacting the count of rows affected with side effects. Rows affected while the setting is enabled should not contribute to the final count, but any rows affected before or after, AFAIR, should.

4. Perhaps this is complex enough that having multiple paragraph is justified.

I suggest to tweak it some something like this:

For UPDATE, INSERT, and DELETE statements, the return value is the number of rows affected by the command. For all other types of statements, the return value is -1.

When a trigger exists on a table being inserted or updated, the return value includes the number of rows affected by both the insert or update operation and the number of rows affected by the trigger or triggers.

When SET NOCOUNT ON is set on the connection (before or as part of executing the command, or as part of a trigger initiated by the execution of the command) the rows affected by individual statements stop contributing to the count of rows affected returned by this method.

If no statement are detected that contribute to the count, the return value is -1. If a rollback occurs, the return value is also -1.

cc @David-Engel this is for SqlClient specifically, so you may want to review or ask someone in your team to review.

cc @roji in case this is interesting for him.

[rpetrusha]

@joelpaula, I'm sorry that it's taken so long for me to respond. Would you like to make the changes suggested by @divega?

[dnfclas]

All CLA requirements met.

[divega]

Content looks good to me.

[roji]

Missed this back in February... It's definitely great to have this level of documentation on what actually happens in SqlClient.

Just out of curiosity @divega: do the extra rows updated by triggers interfere with our optimistic concurrency in any way, or we are checking for zero vs. one or more rows updated?

[divega]

IIRC EF6 and EF Core check for exactly 1 record affected per intended change, but @AndriySvyryd may remember better than I 😄

Generally SET NOCOUNT ON as the first line of a trigger is considered the best practice.

[AndriySvyryd]

@roji We check for exactly 1 currently, but we could relax this. dotnet/efcore#12064

[rpetrusha]

Thank you so much, @joelpaula, for contributing to the dotnet/dotnet-api-docs repo and enriching the documentation. I've left one minor suggestion for you to consider. Once you address it, I'll merge.

[roji]

@AndriySvyryd it's just a thought, relaxing it seems like it would allow optimistic concurrency and rowcount-enabled triggers to work together.

[divega]

@AndriySvyryd it's just a thought, relaxing it seems like it would allow optimistic concurrency and rowcount-enabled triggers to work together.

We could, but I think the recommendation to use SET NOCOUNT ON o triggers is reasonable and well-know (at least I remember learning about it since my RDO days), there is a risk that for UPDATE or DELETE statements to actually affect more than one row if you lie about the primary keys, plus I don't remember right now customers asking for it 😄

[roji]

@divega makes sense.

[rpetrusha]

Thanks, @joelpaula, for making the additional changes. I'll approve and merge your PR now. The changes should be live on docs.microsoft.com in the next day or two.