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

Changed ID creation to use TransformTypeCollectionNameToDocumentIdPrefix for collection name. #15

Closed JoshClose closed 5 years ago

JoshClose commented 5 years ago

I noticed that in most cases the collection names were lowercase, but if the collection name had multiple uppercase, it would stay the same. I then found this post. https://groups.google.com/forum/#!searchin/ravendb/id$20lowercase|sort:date/ravendb/vX4D6ysfS4c/Nc1QF2xABAAJ

This commit changes the ID creation to use Conventions.TransformTypeCollectionNameToDocumentIdPrefix for the prefix to match how the RavenDB client creates them.

I saw you've created a migration script for changes in the past. This does not include a migration upgrade method.

JoshClose commented 5 years ago

Here is an example using LINQPad.

void Main()
{
    var store = new Raven.Client.Documents.DocumentStore();

    var name1 = store.Conventions.GetCollectionName(typeof(Test));
    var name2 = store.Conventions.GetCollectionName(typeof(FirstSecond));

    name1.Dump();
    name2.Dump();

    store.Conventions.TransformTypeCollectionNameToDocumentIdPrefix(name1).Dump();
    store.Conventions.TransformTypeCollectionNameToDocumentIdPrefix(name2).Dump();
}

public class Test
{   
}

public class FirstSecond
{   
}
JoshClose commented 5 years ago

I noticed this will change the role ID from IdentityRoles/RoleName to roles/RoleName. This is probably correct in my case since I created my own Role : IdentityRole class. I may have created the roles before I made my own role class. Either way, just letting you know.

JoshClose commented 5 years ago

I added another commit that adds TRole to UserStore so that AddToRoleAsync and RemoveFromRoleAsync work properly.

JudahGabriel commented 5 years ago

Interesting. Thanks for the PR. Does this break any data backward compatibility? (Can Raven databases created using previous versions of RavenDB.Identity still work without changes if they upgraded to this?)

josh-penrad commented 5 years ago

If they use a custom TRole type, no. Their role collection would change from IdentityRoles to roles (if their class was name Role).

Other than that, I did add a new user and it was added as users/email@domain.com instead of Users/email@comain.com, but it showed up in the same collection in the Studio. The only place where that would be a concern is if the RavenDB.Identity code is checking somewhere for a string and not running it through the same process.

I tested a few scenarios in my project, but I'm sure not all of them.

JudahGabriel commented 5 years ago

Ok, sounds like there's a potential for breaking change, so we'd need to bump this a major version. I'll merge this and deploy a new major version.

Thanks Josh! p.s. I was just doing some CSV work at my day job, and looking at the docs for it. I see you're the author! Cool library, that CSV Helper.

JoshClose commented 5 years ago

Oh cool! I have a bunch of work I need to do on it that I haven't had any time for.

I just realized that I'm posting with 2 different accounts. Oops.

JudahGabriel commented 5 years ago

Josh, I noticed there's one area that still needs updating: in UserStore.AddtoRoleAsync(user, roleName), we find the role based on the role name. If it doesn't exist, we create it. Only problem is, we're creating it using new IdentityRole(roleName): this is fine if you're using plain old IdentityRole, but if you have a type derived from IdentityRole, we have a problem.

To fix this, we could change it to something like:

existingRole = new TRole();
existingRole.Name = roleName;

That would require that TRole have a new() generic type constraint. Is that acceptable?

JoshClose commented 5 years ago

I think that should be fine. I know sometimes people have an issue with requiring a parameterless constructor, but I never do. I think the likelihood of someone not wanting UserStore to have new() on it is pretty slim.

JudahGabriel commented 5 years ago

Cool. Making that change now. Should have a new version pushed out today.

JoshClose commented 5 years ago

I mean for TRole to have new() on it.

JoshClose commented 5 years ago

Cool, thanks!

JudahGabriel commented 5 years ago

OK, I rolled out RavenDB.Identity 6.1 NuGet package, which includes your changes and my TRole : new() constraint.