JudahGabriel / RavenDB.Identity

RavenDB Identity provider for ASP.NET Core. Let RavenDB manage your users and logins.
https://www.nuget.org/packages/RavenDB.Identity/1.0.0
MIT License
61 stars 29 forks source link

Use static indexes instead of dynamic #33

Closed lahma closed 1 year ago

lahma commented 4 years ago

Would you accept PR for adding usage of static indexes which would also allow waiting from them in certain operations. For example after adding using the API can return null because indexes aren't in sync yet. The preferred logic would be with having proper UserIndex:

We have prohibited dynamic indexes in general in development mode so that we don't get any surprising new indexes in production.

JudahGabriel commented 3 years ago

I'm skeptical. We don't use indexes to ensure consistency (e.g. unique email addresses); we use cluster-wide, always-consistent compare/exchange.

And, for creating a new user, it returns a user with an ID, and with that ID, you don't need to go to any indexes.

So, it's not clear to me where static indexes would be generally useful here. If individual apps need static indexes, by all means create them for your app, and Raven (and RavenDB.Identity) will use a static index that has the right fields.

lahma commented 3 years ago

I tried creating the index by hand and hoping that RavenDB.Identity would pick that up:

public class ApplicationUserIndex : AbstractIndexCreationTask<ApplicationUser>
{
    public ApplicationUserIndex()
    {
        Map = users => from user in users
            select new
            {
                user.UserName
            };
    }
}

When this is deployed and available I still see in admin traffic watch from 'ApplicationUsers' where UserName = $p0 limit $p1, $p2 {"p0":"USER@COMPANY.COM","p1":0,"p2":2} and exception after that

NotSupportedException: System.NotSupportedException: Dynamic queries are not supported by this database because the configuration 'DisableQueryOptimizerGeneratedIndexes' is set to true and the query optimizer needs an index for this query at Raven.Server.Documents.Queries.InvalidQueryRunner.ExecuteQuery

So it seems that this static index won't be picked even though it has the UserName indexed.

lahma commented 3 years ago

I was able to get the static predictable behavior at least for my identity server login flow with these changes (I also need RavenDB 4 support for now).

JudahGabriel commented 3 years ago

I'd accept a PR if the static indexes is opt-in, e.g.

services.AddRavenDbIdentityStores(options => o.UseStaticIndexes = true);

Something like that.

Reviewing the UserStore.cs code, I see we use indexes in a few places:

If you really want to do away with dynamic indexes, you'd need to build an index(es) that can fulfill those queries.

(FYI, that last one isn't strictly required - we could rework the code there to skip the index if it's important.)