Cannot add a new client application to OpenID because of an exception

[domonkosgabor]

Set up the Authorization server to allow client credentials flow:

Add a new application with a previously created client credential role:

Need to create the application first without any client credential, then edit it to add the credential, because of the following error:

System.AggregateException: One or more errors occurred. (SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.) ---> Microsoft.Data.Sqlite.SqliteException: SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.
at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteScalar()
at System.Data.Common.DbCommand.ExecuteScalarAsync(CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
at Dapper.SqlMapper.ExecuteScalarImplAsync[T](IDbConnection cnn, CommandDefinition command) in C:\projects\dapper\Dapper\SqlMapper.Async.cs:line 1226
at YesSql.Session.SaveEntityAsync(Object entity) in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 192
at YesSql.Session.CommitAsync() in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 541
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at YesSql.Session.Dispose() in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 466
at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose()
at OrchardCore.Hosting.ShellBuilders.ShellContext.ServiceScopeWrapper.Dispose() in C:\Repos\Total Coding Platform 2\src\OrchardCore\OrchardCore.Abstractions\Shell\Builders\ShellContext.cs:line 250
at OrchardCore.Modules.ModularTenantContainerMiddleware.Invoke(HttpContext httpContext) in C:\Repos\Total Coding Platform 2\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 102
at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
---> (Inner Exception #0) Microsoft.Data.Sqlite.SqliteException (0x80004005): SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.
at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteScalar()
at System.Data.Common.DbCommand.ExecuteScalarAsync(CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
at Dapper.SqlMapper.ExecuteScalarImplAsync[T](IDbConnection cnn, CommandDefinition command) in C:\projects\dapper\Dapper\SqlMapper.Async.cs:line 1226
at YesSql.Session.SaveEntityAsync(Object entity) in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 192
at YesSql.Session.CommitAsync() in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 541<---

Tried with SQL Server, tried with predefined roles, but get the same issue.

[kevinchalet]

That's not the first time I see this exception and I recently found a way to reproduce it consistently. I'll chat with @sebastienros and see if he has a magical sauce to find the root cause.

There's also this one, that appears from time to time:

System.NullReferenceException
  HResult=0x80004003
  Message=Object reference not set to an instance of an object.
  Source=YesSql.Core
  Arborescence des appels de procédure :
   at YesSql.Session.<>c__DisplayClass35_1.<ReduceAsync>b__3(IIndex x) in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 584

[sebastienros]

I think yessql has SourceLink, can you debug?

[sebastienros]

One or more errors occurred. (SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.) this one is usually from the caller code, not a YesSql issue, I can check what is wrong.

[kevinchalet]

I think yessql has SourceLink, can you debug?

Will try the next time I see it.

this one is usually from the caller code, not a YesSql issue, I can check what is wrong.

What's weird is that this exception is thrown for a simple query (the CreateAsync method is not even called, as it doesn't pass the validation stage, which tries to validate the uniqueness of the client_id):

public virtual async Task<TApplication> FindByClientIdAsync(string identifier, CancellationToken cancellationToken)
{
    if (string.IsNullOrEmpty(identifier))
    {
        throw new ArgumentException("The identifier cannot be null or empty.", nameof(identifier));
    }

    cancellationToken.ThrowIfCancellationRequested();

    // Crashes here:
    var result = await _session.Query<TApplication, OpenIdApplicationIndex>(index => index.ClientId == identifier).FirstOrDefaultAsync();
    return result;
}

[kevinchalet]

I spent some time reproducing and debugging this issue and I believe it's caused by two things:

• We have a bug in the YesSql application store: OpenIdApplicationStore.SetRolesAsync() calls _session.Save(application) (which is not something it's supposed to do at all). Under the hood, this method is used to populate an OpenIdApplication YesSql entity based on an OpenIdApplicationDescriptor, that will be later persisted by OpenIdApplicationStore.CreateAsync().

• YesSql automatically commits pending changes when performing a query (which is a very surprising behavior, TBH).

As a consequence, when an application is about to be created, OpenIdApplicationManager.CreateAsync() is called, a new OpenIdApplication entity is instantiated and immediately added to the pending entities list of YesSql, which will persist the changes when OpenIddict will validate the application and ensure no other entity with the same client_id exists in the database.

I'll fix the bug in the YesSql app store but it might be worth revisiting YesSql's auto-commit feature to prefer a more explicit behavior.

[sebastienros]

It's called autoflush and it's a very common pattern in orms. Look at nhibernate for instance. I'd love to see a simple repro if the bug you are mentioning in yessql. There might still be an issue even with autoflush is fine.

[kevinchalet]

There's no bug in YesSql (the bug is in the YS application store).

It's just fairly surprising that changes are committed during a simple query (which is pretty much like persisting a resource during a GET request). While it's a pattern used in some other ORMs, it doesn't exist in the most popular one in the .NET ecosystem: EF 6/EF Core.

People not used to this feature might have a hard time debugging when things go wrong. Perhaps that's just me, but I prefer EF's behavior: yes, you might get stale reads, but at least things are super explicit as no change is applied before SaveChanges() is explicitly called.

Look at nhibernate for instance.

It can be disabled in NH 😄

[sebastienros]

And as I read again your explanation I understand the issue in the code now. I will assume you'll provide the fix later.

I am fine adding an option to disable autoflush in yessql, but it wouldn't be the default, and OC would not use it. Maybe I could make it in a way we can disable it temporarily, or a flag in the queries.

To explain why it is this way, if with the same YesSql session you start creating objects, then try to do queries, I assume that the objects you created and saved will probably impact the subsequent queries, so we need to commit them. This is important in OC because all modules can interact without knowing each other. So you can't define a flush operation from one module, or pre-commit from another, as these might not be necessary. The other issue is that there is a single session per request. So autoflush is necessary. There are pros and cons for both solutions. In O1 we started without autoflush, and we had to use it after some time, and it became much easier.

[kevinchalet]

I am fine adding an option to disable autoflush in yessql, but it wouldn't be the default, and OC would not use it.

We'll likely not need it. I was just curious to know why it was designed this way 😄

And as I read again your explanation I understand the issue in the code now. I will assume you'll provide the fix later.

Yep, I'll send a PR in a few minutes.

So you can't define a flush operation from one module

Why? If a component A using a shared session wants to make the "not yet committed" changes visible/available to components B and C, it seems reasonable to think that it should explicitly persist the changes before the next components are invoked. On the other hand, it seems weird that components B or C incidentally commit the changes they didn't initiate while performing a query (which also leads to "misplaced" exceptions if anything bad happens).

[kevinchalet]

The other issue is that there is a single session per request.

Is it an issue? If you think it is, then maybe you should consider having an ISessionFactory a-la-NHibernate so that every component that needs to perform DB actions does that in a constrained unit of work, isolated from the other components.

[sebastienros]

Can we close now? You referred to this issue from the PR, but it was not stating it would "fix" it.

[kevinchalet]

@domonkosgabor do you mind giving it another try to see if my recent fix addressed your issue? Thanks!

[kevinchalet]

@domonkosgabor ping.

[domonkosgabor]

@PinpointTownes tried with the latest changeset: f4eca1e
Now I got a different issue with two exceptions:

System.AggregateException: One or more errors occurred. (SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.) ---> Microsoft.Data.Sqlite.SqliteException: SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.
at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader()
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteScalar()
at System.Data.Common.DbCommand.ExecuteScalarAsync(CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
at Dapper.SqlMapper.ExecuteScalarImplAsync[T](IDbConnection cnn, CommandDefinition command) in C:\projects\dapper\Dapper\SqlMapper.Async.cs:line 1217
at YesSql.Session.SaveEntityAsync(Object entity) in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 192
at YesSql.Session.CommitAsync() in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 515
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.Wait()
at YesSql.Session.Dispose() in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 435
at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose()
at OrchardCore.Hosting.ShellBuilders.ShellContext.ServiceScopeWrapper.Dispose() in C:\Repos\Orchard Core GIT\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Builders\ShellContext.cs:line 250
at OrchardCore.Modules.ModularTenantContainerMiddleware.Invoke(HttpContext httpContext) in C:\Repos\Orchard Core GIT\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 102
at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
---> (Inner Exception #0) Microsoft.Data.Sqlite.SqliteException (0x80004005): SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.
at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader()
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteScalar()
at System.Data.Common.DbCommand.ExecuteScalarAsync(CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
at Dapper.SqlMapper.ExecuteScalarImplAsync[T](IDbConnection cnn, CommandDefinition command) in C:\projects\dapper\Dapper\SqlMapper.Async.cs:line 1217
at YesSql.Session.SaveEntityAsync(Object entity) in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 192
at YesSql.Session.CommitAsync() in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 515<---

And a SqliteException:

Microsoft.Data.Sqlite.SqliteException (0x80004005): SQLite Error 19: 'UNIQUE constraint failed: Document.Id'.
at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader()
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteScalar()
at System.Data.Common.DbCommand.ExecuteScalarAsync(CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
at Dapper.SqlMapper.ExecuteScalarImplAsync[T](IDbConnection cnn, CommandDefinition command) in C:\projects\dapper\Dapper\SqlMapper.Async.cs:line 1217
at YesSql.Session.SaveEntityAsync(Object entity) in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 192
at YesSql.Session.CommitAsync() in C:\projects\yessql-un1yf\src\YesSql.Core\Session.cs:line 515

[kevinchalet]

@sebastienros dunno if it's related to the issue mentioned by @domonkosgabor, but I finally had a chance to catch the NRE with SourceLink enabled:

https://github.com/sebastienros/yessql/blob/dev/src/YesSql.Core/Session.cs#L584

Does that ring a bell?

[sebastienros]

is it the result of descriptorGroup(x) that is null? I think I have had that issue by the past but I fixed it for map indexes... Do you know which index was called here?

[kevinchalet]

is it the result of descriptorGroup(x) that is null?

Yep, using the execution window in VS confirmed it.

Do you know which index was called here?

OrchardCore.OpenId.YesSql.Indexes.OpenIdApplicationByRoleNameIndex

[sebastienros]

I try to repro locally. I am pretty sure it's a bug in yessql, that can probably be fixed very easily.