Repodb: Bug: ExecuteQueryMultiple/Extract is throwing SqlException: Login failed for user <username>

[BieleckiLtd]

Am I using ExecuteQueryMultiple / Extract incorrectly here or there is a bug?

var sql1 = @"SELECT * FROM UsersTbl;";
var sql2 = @"SELECT * FROM RolesTbl;";
var sql3 = @"SELECT * FROM UserXRoleTbl;";

using var connection = await new SqlConnection(connectionString).EnsureOpenAsync().ConfigureAwait(false);
//WORKS FINE
var users = await connection.ExecuteQueryAsync<User>(sql1).ConfigureAwait(false);
var roles = await connection.ExecuteQueryAsync<Role>(sql2).ConfigureAwait(false);
var userXRoles = await connection.ExecuteQueryAsync<UserXRole>(sql3).ConfigureAwait(false);

//FAILS
using var results = await connection.ExecuteQueryMultipleAsync($"{sql1}{sql2}{sql3}").ConfigureAwait(false);
var users = await results.ExtractAsync<User>().ConfigureAwait(false); //THROWS SqlException: Login failed for user <username>
var roles = await results.ExtractAsync<Role>().ConfigureAwait(false);
var userXRoles = await results.ExtractAsync<UserXRole>().ConfigureAwait(false);

From what I understand ExecuteQueryMultiple runs all queries in one trip hence I would prefer it over individual ExecuteQuery's.

[mikependon]

Yes, you are correct, it does execute in once. OMG, this complex problem happens here. This issue is very familiar, I will fix this before the actual release. This is related to Integrated Security thing, a temp fix is to put a PSI = true (Persists Security Info) in the connection string.

Btw, can you try if such issue happens in a non-async method with the PSI?

[BieleckiLtd]

Hi. Thanks, I can confirm that setting Persist Security Info=True works around the issue for both sync and async methods.

[mikependon]

Yes, but it should not be like that. Anyway, it seems the issue is happening to both async and non-async methods based on your reply. Thanks for confirming.

[mikependon]

Hey, I cannot replicate this problem on my end. I am trying to analyzed your case actually, please be reminded of the empty new lines here. Can you wrap your 'using' calls with the curly braces and see if the problem still exists?

I am thinking that before the calls below, the connection has been closed already.

//FAILS
using var results = await connection.ExecuteQueryMultipleAsync($"{sql1}{sql2}{sql3}").ConfigureAwait(false);

EDIT: RepoDB is trying to call the EnsureOpen() method internally, which forces your closedconnection to throw an exception if the PSI is not defined. If that's the case, then, that is an expected behavior from the ADO.NET point of view.

[BieleckiLtd]

Hi. Wrapping (inner, outer, both) using statements with braces {} did not helped nor broke anything. Is setting Persist Security Info=True in the connection string the definite solution for using this method?

[mikependon]

PSI is only used if you open a connection then closes it, and then reusing the same instance of connection. The reason to that is the preservations of the security credentials within the connection object.

In the case of using only the connection without closing it, I would assume, you do not need the PSI there. I tested this one on my development machine, and also added more Integration Tests in relation to this, and everything seems to be working fine.

Can you verify in your end if you are closing the connection before the ExecuteQueryMultiple calls happen? If yes, then this is an expected behaviour.

[BieleckiLtd]

No, I do not close the connection. In fact, just before ExtractAsync it is still open. Here's my complete use:

var sql1 = @"SELECT * FROM UsersTbl;";
var sql2 = @"SELECT * FROM RolesTbl;";
var sql3 = @"SELECT * FROM UserXRoleTbl;";

using (var connection = await new SqlConnection(connectionString).EnsureOpenAsync().ConfigureAwait(false))
{
    using (var results = await connection.ExecuteQueryMultipleAsync($"{sql1}{sql2}{sql3}").ConfigureAwait(false))
    {
        Debug.Print(connection.State.ToString()); // Prints "Open"
        var users = await results.ExtractAsync<User>().ConfigureAwait(false); // FAILS
        var roles = await results.ExtractAsync<Role>().ConfigureAwait(false);
        var userXRoles = await results.ExtractAsync<UserXRole>().ConfigureAwait(false);

        foreach (var u in users)
        {
            /// ... 
        }

        return users;
    }
}

Just checked synchronous methods for this scenario and it throws the same error in the same place.

[mikependon]

I will try to give it a shot again, but it seems to be very identical to our Integration Test, please see it here.

[BieleckiLtd]

Yes, I saw these, quite similar use so I do not understand why Extract is throwing at me in my case.

For completeness, I use SQL Server Authentication (not Windows Authentication) so my connection string is following: "Server=<server>;Database=<database>;User Id=<sqlserveruser>;Password=<sqlserverpassword>;" Only after I add Persist Security Info=True; it stops throwing.

My SQL Server version: Microsoft SQL Server 2014 (SP1-CU13) (KB4019099) - 12.0.4522.0 (X64) Jun 28 2017 17:36:31 Copyright (c) Microsoft Corporation Standard Edition (64-bit) on Windows NT 6.3 (Build 9600: ) (Hypervisor)

[mikependon]

TBH, this prevents me of pushing the v1.12.0 :) - Anyway, I am now creating a small application to replicate this problem. The integration test is also using the Integrated Security = FALSE with a valid UID/PWD credentials (see here).

[mikependon]

Now I got it, it happens if you have the EnsureOpenAsync() call during the connection object instantiation. Just to make it work for now in your side, please remove that EnsureOpenAsync() call.

[BieleckiLtd]

Yes, that was it! Actually, getting rid of manually calling EnsureOpen() would be my preference in every other case but I think other methods fails if I don't do that.

[mikependon]

All the extended methods are calling the EnsureOpen() by default as it uses the underlying single method internally to create the command parameters. If the call to it has not happened, then we consider it as a bug as well.

Honestly, we are not using the EnsureOpen() method as it's casting back the connection object into an IDbConnection object. Your preference! :smile: Anyway, the fix is on-going now.

[BieleckiLtd]

Actually, you're right, now when I check I don't have to manually EnsureOpen()! I remember my first encounter with RepoDB was that it was failing until I checked docs and figured out I must call EnsureOpen(). Can't tell now what the method was.

[mikependon]

This library is a buggy library before, a lot has changed and a lot has been covered with Integration and Unit Tests. I could not even pin point the changes made that fixed this, but I am sure of what the code written as of today's date. :smile:

[BieleckiLtd]

Suggestion - remove EnsureOpen() from docs if not absolutely necessary for example code to work. Sometimes simple things like that might deter lazy people when they think they'll have to add this to change from Dapper for example :)

[mikependon]

Suggestion - remove EnsureOpen() from docs if not absolutely necessary for example code to work.

Sure, I will do this!

Just to let you know. I really liked this incident TBH. I am planning to make a release with Obselete to the ToEnumerable() (see here) compiler method as I am planning to make the changes there not on this release, but in the future. The changes are also related to this kind of incident historically.

It is also related to a performance of RepoDB in which it is initially beaten the Hand Coded materializer, but after I introduce the hacky changes here, managing the connection, it falls to a bit thin margin in the bencher.

The QueryMultiple and ExecuteQueryMultiple operations are just special and they are delaying the opening of the connection, up until the Result Extractor is being disposed, and also, through these methods, I need to add a logic to avoid the creation of multiple DbDataReader. However, through this issue as you reopen it, it gives me more clarities, given with the new cleaner and enhanced compiler.

And since this incident really touched the RepoDB compiler, it pushes me to optimize the mentioned obsolete method in advance. Therefore, this will delay the release of v1.12.0. Though I already issued 4 betas for that version. I think, I will do the testing for the actual release instead.

[BieleckiLtd]

I'm glad if I helped, I really like this project. I plan to use it in my next project so it must work well :). I think I found another problem after I stripped EnsureOpenAsync(). I will describe in separate thread.

[mikependon]

Yes, please describe it so we can include it on the next release. If that is not related to this issue, it is good to have it separated on a different issue.

[mikependon]

This is now available at RepoDb v1.12.0.

[mikependon]

Also this one, it is now available at RepoDb v1.12.3. Closing this incident ticket now.