search

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

Added support for .Net Core 3 #17

Closed adam8797 closed 4 years ago

adam8797 commented 4 years ago

I have a complimentary pull request here: https://github.com/JudahGabriel/RavenDB.DependencyInjection/pull/4

Added support for .Net Core 3 libraries, as well as new sample projects.

adam8797 commented 4 years ago

Also note that I added a call to SaveChanges() in the UpdateUserAsync() function of the UserStore. Core3.0's new project includes the Identity UI wrapped up entirely in its own library, so you can't derive its controllers from the RavenController class.

The UserStore had to call SaveChanges instead. This functionality was only changed for the .Net Core 3 version, as not to break any existing implementations in Core 2

JudahGabriel commented 4 years ago

Thanks for the PR.

Please remove SaveChanges() from UpdateUserAsync. For saving changes, see how we're doing it for the Razor Pages sample.

adam8797 commented 4 years ago

Sure, can do. I'll push it up this evening.

FYI, I might address that in another PR. I think saving the session after every action is a little extreme. Plus if UserStore isn't saving changes on an UpdateUser(), its not fulfilling the contract of IUserStore

However, one thing at a time. We can hash that out in a separate PR (if I ever get around to it)

JudahGabriel commented 4 years ago

Great, thanks.

The key is, leave saving into the hands of the end user. It's a deliberate decision that has paid off well over the years; it enables apps to take advantage of transactions when dealing with identity. This is an important feature.

As for saving after every action, Raven does nothing if nothing's changed. But, again, committing changes is simply left into the hands of the consumer, not forced upon them by our library.

adam8797 commented 4 years ago

Just removed those lines real quick. Don’t pull yet as the Samples will be broken.

There may be an issue as the new Razor Pages don’t seem to support Filters anymore, but that’s after a quick read.

adam8797 commented 4 years ago

@JudahGabriel I think this one is ready to go, if you're happy with the changes. I got the samples working with the filters.

adam8797 commented 4 years ago

Do note that this requires changes to RavenDB.DependencyInjection as well

JudahGabriel commented 4 years ago

We've made some updates to the source based on your pull request. We've done things a bit different though, so we won't be merging your request. Thanks so much for contributing - your PR was the inspiration for our changes and our upgrade to netstandard2.1. Thanks!