Message order

[c-j-hughes]

This fixes #134.

@driseley would appreciate your feedback on this one. I've added a variant of your repro as an integration test - /test/AdoNetCore.AseClient.Tests/Integration/MessageOrderTests.cs - which is a good entry point to look at it.

[driseley]

@c-j-hughes - This is great - thanks! I will run this branch against our reporting code on Monday to see if it works in all our cases, and let you know the results.

[driseley]

Hi @c-j-hughes

I have done some testing this morning - and come across three issues so far:

1. When the stored procedure does not exist, the call to AseCommand.ExecuteReader() no longer throws an AseException
2. AseDataReader.HasRows is not set correctly when the results have been exhausted
3. If the stored proc has no results sets, the InfoMessage events are not fired

See pull request #136 to add test cases that cover these three issues

Hope that's ok

Dave

[c-j-hughes]

Hey @driseley,

Sorry for the delay - been traveling with work.

I've merged your tests into the branch and had another stab at this. New approach is:

1. If a message is encountered in the token stream prior to any data, it is dispatched to event handlers immediately.
2. Otherwise, it is tacked onto that TableResult, and only dispatched once that TableResult is fully processed - either by running out of rows via an AseDataReader.Read() call, or by navigating to the next result with AseDataReader.NextResult().

This passes all of your tests except the ExecuteReader_WithMessagesEmbeddedInResultsAndUsingDataTable_RetainsServerOrder. I'm a little confused by these tests as my understanding of the AseDataReader.HasRows property is that it merely indicates if the current TableResult has one or more rows. I didn't think that it's value changed while the TableResult was enumerated. The usual pattern is to call the AseDataReader.Read() and the result from that call tells you if there are more rows to enumerate. If I've misunderstood this, I can change the behaviour. Just not sure.

Some examples are here:

• https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/retrieving-data-using-a-datareader
• https://stackoverflow.com/questions/14196644/should-if-call-sqldatareader-hasrows-if-i-am-calling-sqlreader-read

And there's the source code for the SqlDataReader here as a reference: https://referencesource.microsoft.com/#system.data/system/data/sqlclient/SqlDataReader.cs

[driseley]

Hi @c-j-hughes

Sorry - I think I had misinterpreted the meaning of HasRows, and the test was not quite correct however there is still a problem. That test should probably look like this:

using (var command = connection.CreateCommand())
{
    command.CommandText = "[dbo].[sp_test_message_order]";
    command.CommandType = CommandType.StoredProcedure;
    command.AseParameters.Add("@runSelect", withSelect);

    using (var reader = command.ExecuteReader())
    {
        int resultCount = 0;
        if (withSelect == 'Y')
        { 
            Assert.False(reader.IsClosed);
            Assert.True(reader.HasRows);
        }
        else
        {
            Assert.True(reader.IsClosed);
            Assert.False(reader.HasRows);
        }
        while (!reader.IsClosed)
        {
            DataTable dataTable = new DataTable();
            dataTable.Load(reader);
            results.Add(dataTable.Rows[0].ItemArray[0].ToString());
            resultCount++;
            if (resultCount < 2)
            {
                Assert.False(reader.IsClosed);
                Assert.True(reader.HasRows);
            }
            else
            {
// Both assertions here fail
                Assert.True(reader.IsClosed);
                Assert.False(reader.HasRows);
            }
        }
    }
}

The issue is, when the dataTable.Load(reader) is called, internally it's calling

https://referencesource.microsoft.com/#system.data/system/data/DataTable.cs,4480

if (!reader.IsClosed && !reader.NextResult()) { // 
    reader.Close();
}

but after the last result reader.NextResult() still returning true, so the result set was not closed. It feels like an off by one error?

[c-j-hughes]

@driseley - I think I sussed it out.

So the behaviour of AseDataReader.HasRows is as I thought. What what going on was that the DataTable.Load(reader) method will internally call AseDataReader.Close() if there are no more results. After calling AseDataReader.Close(), the reference driver AseDataReader.HasRows returns false. I've added this support.

Those two tests still fail though, as the messages and data end up out of order.

What's happening is that:

• 'Report Header' is dispatched immediately.
• 'Table 1 Header' is dispatched immediately.
• 'value 1' is added to a TableResult and returned to the AseDataReader.
• 'Table 2 Header' is attached to the above TableResult to be dispatched during enumeration.
• 'value 2' is added to a TableResult and returned to the AseDataReader.
• 'Report Trailer' is attached to the above TableResult to be dispatched during enumeration.
• Then the DataTable.Load(reader) method is called, which dispatches 'Table 2 Header', and adds the 'value 1' to the result list (out of order).
• Then the DataTable.Load(reader) method is called, which dispatches 'Report Trailer', and adds the 'value 2' to the result list (out of order).

For this to work the way you've defined I think I'd have to change it so that messages got attached to subsequent TableResult objects, and processed before any data was processed, but then I'd need some way to deal with trailing messages.

Let me know your thoughts and I can have another go at it.

[driseley]

Hi @c-j-hughes,

Our posts overlapped! I think your suggestion is good.

If you'd like, I'll update the test cases (with another pull request) with the change above and to also change the stored proc so it returns an empty result set in the middle ( to ensure all corner cases are covered )?

[c-j-hughes]

Ok cool.

[driseley]

Added PR #137 for updated tests

[c-j-hughes]

@driseley - I compared your tests with the DataTable.Load(reader) in them to the SAP driver, and it follows a peculiar pattern:

1. Dispatches 'Report Header' and 'Table 1 Header' immediately.
2. Dispatches 'Report Header', 'Table 1 Header', and 'Empty Table 2 Header' while filling the first DataTable.
3. Dispatches 'Report Header', 'Table 1 Header', 'Empty Table 2 Header', and 'Table 3 Header' while executing NextResult().
4. Dispatches 'Report Header', 'Table 1 Header', 'Empty Table 2 Header', 'Table 3 Header' and 'Report Trailer' while filling the second DataTable.

I would surmise that the SAP behaviour is to have a list of messages inside the AseDataReader that is appended to as messages are found in the token response stream, and then all the messages in the list are sent each time the data is enumerated, or NextResult() is called.

As is, I can't see how to get ExecuteReader_WithMessagesEmbeddedInResultsAndUsingDataTable_RetainsServerOrder('Y',["Report Header", "Table 1 Header", "value1", "Empty Table 2 Header", "Table 3 Header", ...]) to pass while having the DataRow values added to the results collection after the data has been loaded - which dispatches the messages.

I've modified the test to use the DataTable.RowChanged event so that the results collection is modified as the data is encountered. This makes the test pass.

Thoughts?

[driseley]

Hi @c-j-hughes

Thanks for all the work on this so far!

I think what you have there works for me, I can preserve the message order without resorting to the RowChanged event, see PR #138. You just have to handle the output and results separately. This approach works with the SAP driver too.

I have also added an output parameter to the proc, just to make sure it still works (it did).

One further thing I did notice is that the IsClosed behaviour of the reader differs from the SAP driver.

Using the SAP driver you have to call command.ExecuteReader(CommandBehavior.CloseConnection) to get the command to close when it runs out of results sets - but when you do this it also closes the connection. I prefer your implementation, where only the the command is closed(by default), as this allows you to continue transactions, but I thought I should let you know the difference.

I'll take the latest branch and try it against our prod reports on Monday and let you know how it looks.

[driseley]

@c-j-hughes I have tested a local build of this branch against our reports and it all seems to work well. I can reconstruct the output as required. Many thanks!

[driseley]

Hi @c-j-hughes , @senseibaka - would it be possible to get this PR merged to master and incorporated in a release?

Many thanks Dave

[senseibaka]

Hi @driseley sorry about that, we've both been rather busy the last few months, I might be able to put out a release tomorrow, after testing this PR.

[driseley]

@c-j-hughes , @senseibaka - many thanks for the release!