Additional "post-false" reads needed with non-default CommandBehavior
- Dominant language
- C#
- Stars
- 989
- Forks
- 340
- Avg merge
- 4d 19h
- Merged PRs (30d)
- 72
Description
context, investigating https://github.com/DapperLib/Dapper/issues/2077 , I put together [this test](https://github.com/DapperLib/Dapper/blob/cc3d5425e1ecf53170f63c581c7c185a40efbb3d/tests/Dapper.Tests/ErrorTests.cs#L39), which is just using raw ADO.NET (no Dapper), using `SingleResult`, `SingleRow` and "both"; I am familiar with the problem of trailing faults in the TDS flow, hence Dapper does always read-to-end, but this test shows that *extra* row-reads **after** `reader.Read()` has reported `false` are necessary to observe the exceptions, with the number of extra `Read()` calls being query-dependent. The coded example needs at least 2 extra reads, but the OP has reported higher numbers by changing the SQL
Having to perform extra reads *after* seeing `reader.Read()` report `false` feels like a bug.
This behaviour is currently consistent between System.Data.SqlClient and Microsoft.Data.SqlClient
to be explicit, to see the exception in the example shown, we need to:
1. we call `Read()` to test for the expected row, get `true`
2. we call `Read()` to test for unexpected rows, get `false`
3. we call `Read()` *again* "just because", get `false`
4. we call `Read()` *once more*, boom: `SqlException`
it seems like this `SqlException` should have happened before `Read()` returned `false` at step 2, *or* should not have occurred at all; but needing to call `Read()` an arbitrary and unpredictable number of *extra* times is... not good.
risk: faults go unobserved, transactions get incorrectly committed, human sacrifice, dogs and cats living together, MASS HYSTERIA
simplified repro (using `reader.NextResult()` does not help, note):
``` c#
[Fact]
public void ManualADONETSimplified()
{
// note test shows *undesirable* outcome as "pass"
using var conn = GetOpenConnection();
using var cmd = conn.CreateCommand();
cmd.CommandText = SQL;
using var reader = cmd.ExecuteReader(CommandBehavior.SingleResult);
Assert.True(reader.Read()); // 1. expected data
Assert.False(reader.Read()); // 2. check for unexpected data
Assert.False(reader.Read()); // 3. just because
Assert.ThrowsAny(() => reader.Read()); // 4. boom
}
// from https://github.com/DapperLib/Dapper/issues/2077
const string SQL = """
-- MOVE the 'SELECT 7;'-Statement here, to increase the required while(read()) calls by one.
BEGIN TRANSACTION
BEGIN TRY
SELECT 7; --SELECT any value INSIDE TRANSACTION
--FORCE Error
DECLARE @intvar INT
SET @intvar = 'A'
COMMIT TRANSACTION
END TRY
BEGIN CATCH
ROLLBACK TRANSACTION
;THROW 50000, 'ERROR in Transaction', 1;
END CATCH
""";
```
Contributor guide
Assessment
This issue has not been assessed yet.