Feat/184 set multitenant onmodelcreating

[AndrewTriesToCode]

@GordonBlahut What do you think about this?

• Added EntityModelBuilder.IsMultiTenant() extension method that can be used in OnModelCreating to set the annotation.

• Types with [MultiTenant] get the annotation in Shared.SetupModel

• All logic uses the annotations

• MultiTenantIdentityDbContext now applies IsMultiTenant() to each of the standard Identity types.

• MultiTenantIdentityUser and similar all marked obsolete.

• Added unit test for an entity using the extension method

• Updated IdentityDataIsolationSample for netcoreapp2.2 and netcoreapp3.0

• Updated DataIsolationSample for netcoreapp3.0

• Still need to update documentation.

[GordonBlahut]

One concern I have as a library consumer is forcing all of the Identity-related classes to be MultiTenant. That would actually break my usage today.

In my current use case, my User is marked as MultiTenant, but my Role is not since in my case they are standard regardless of the tenant. I also don't bother marking my UserClaim, UserLogin, etc. as MultiTenant because they are dependent on the User which is already MultiTenant so I didn't see any added benefit for the overhead of marking them so.

I suppose it could also be possible for a library user to want to have MultiTenant entities but not have the Identity classes be MultiTenant if they had a single sign-on type thing where users work across tenants.

I think it would offer greater flexibility to leave it up to the library user to call IsMultiTenant() for the Identity classes they want to enroll. That flexibility exists today so it would be a loss to remove it.

Obsoleting the MultTenantIdentity* classes is good.

I was also toying with the idea of doing the entity configuration right inside FinbuckleEntityTypeBuilderExtensions.IsMultiTenant instead of Shared.SetupModel (Shared.SetupModel would only check for the attribute and call IsMultiTenant() when found), which would give the library consumer even greater flexibility on model configuration. I have to think about how this impacts modifying the indexes on Identity entities though. I have some ideas around it.

In your documentation you recommend calling base.OnModelCreating() last. You could extend the guideline to say that if you have a call to HasQueryFilter() on an entity, you should call IsMultiTenant() for the entity after the call to HasQueryFilter().

I do all my entity configurations in IEntityTypeConfiguration<T> classes so being able to control when stuff is setup is very helpful. In my case I have to keep base.OnModelCreating() call first because I override some of the default ASP.NET Identity class configurations and calling it last would overwrite my configuration with the default. (Note: I have my own AddQueryFilter implementation to make sure I don't remove the filters added by your library).

I also wonder if there could be any benefit to moving HasMultiTenantAnnotation to a public extension class. There could be cases where that information could be useful, maybe in a SaveChanges override for example.

[AndrewTriesToCode]

@GordonBlahut
Thanks for the feedback! I had the same hesitation on making all the Identity entities MultiTenant by default and I'm glad you point out this would break your code.

I took a quick stab at moving all the global query filter logic right into the builder extension method but hit some problems and it was getting late so I left it as-is. I think had trouble passing the expression needed to get the tenant id to the extension method cleanly -- I think a default parameter on the extension method would probably do the trick.

I've given you access to the downstream repo--please submit any of the other changes you mentioned. I understand your situation regarding calling the base OnModelCreating first. I think it should be ok, but if you set the global query filter be careful. My code takes into account any previous global query filter so I recommended it go last.

As long as the unit tests all pass I'm happy to accept changes.

Once we get things working how we like it, I'll update the docs and merge.

[GordonBlahut]

Pulled down your feature branch. Do you want me to commit directly to that branch or to a new branch from there?

[AndrewTriesToCode]

Feel free to commit straight to this one.

[GordonBlahut]

Ok, will do. I figured out how to do the global query filter expression without needing to pass any expression in. I just need to do the index modifications for the identity classes from the extension method as well and then it should be good for review.

[GordonBlahut]

Running the tests raised something I didn't consider: with MultiTenantIdentityDbContext now inheriting from MultiTenantIdentityDbContext<IdentityUser> instead of MultiTenantIdentityDbContext<MultiTenantIdentityUser> and IdentityUser (and related) not automatically being multitenant, someone using MultiTenantIdentityDbContext and upgrading would no longer have a multitenant user where they previously did.

I'm conflicted because on the one hand, IdentityUser (and related) should not/cannot automatically be MultiTenant, but on the other hand it could cause confusion for anyone upgrading.

Edit: I have an idea that I think maintains backward-compatibility for upgrades without sacrificing any flexibility:

MultiTenantIdentityDbContext

• Register all Identity classes as MultiTenant.

MultiTenantIdentityDbContext<TUser>

• Register all Identity* classes except TUser as MultiTenant.
• Anyone upgrading from a previous release would have either inherited from MulitTenantIdentityUser or used [MultiTenant] if they wanted a multitenant user, in which case it will be still be multitenant due to the attribute anyway.

MultiTenantIdentityDbContext<TUser, TRole, TKey>

• Register all Identity* classes except TUser and TRole as MultiTenant.
• Anyone upgrading from a previous release would have either inherited from MulitTenantIdentityUser/MultiTenantIdentityRole or used [MultiTenant] on their user and/or role class, in which case they will still be multitenant due to the attribute.

MultiTenantIdentityDbContext<TUser, TRole, TKey, TUserClaim, TUserRole, TUserLogin, TRoleClaim, TUserToken>

• Do not register any Identity* classes as MultiTenant
• Anyone upgrading from a previous release would have decided when to inherit from MultiTenantIdentity* or used [MulitTenant] or not depending on which class(es) they wanted to be multitenant, in which case they will be multitenant.

That should mean everything continues to work exactly as it previously did and the MulitTenantIdentity* classes could still be obsolete. Maybe some Xmldocs for the DbContexts constructors could indicate that certain identity classes will be multitenant out of the box.

Thoughts?

[AndrewTriesToCode]

Hm, go ahead and push what you have so I can take a look. I've been thinking about how to support a DbContext not needing to inherit from MultiTenantDbContext or MultiTenantIdentityDbContext at all. I think the IsMultiTenant extension methods is a big step. I think I could wrap up the other customizations that MultiTenantDbContext and MultiTenantIdentityDbContext does (eg the pre and post save code) so that someone like yourself could call them in your own DbContext and not need to inherit.

With respect to the breaking change you describe above, I think your approach makes a lot of sense. Then in your case you would derive from the long form MultiTenantIdentityDbContext<TUser, TRole, TKey, TUserClaim, TUserRole, TUserLogin, TRoleClaim, TUserToken> and set the specific entities to MultiTenant as you want right?

[GordonBlahut]

I've been thinking about how to support a DbContext not needing to inherit from MultiTenantDbContext or MultiTenantIdentityDbContext at all. I think the IsMultiTenant extension methods is a big step. I think I could wrap up the other customizations that MultiTenantDbContext and MultiTenantIdentityDbContext does (eg the pre and post save code) so that someone like yourself could call them in your own DbContext and not need to inherit.

That's funny, I've been thinking about something like that too. I was imaging a case where there'd be 2 different libraries that both required an inherited DbContext and it would be impossible to inherit from both. It's too bad EFCore doesn't have the concept of a plugin concept that can hook into the DbContext lifecycle.

With respect to the breaking change you describe above, I think your approach makes a lot of sense. Then in your case you would derive from the long form MultiTenantIdentityDbContext<TUser, TRole, TKey, TUserClaim, TUserRole, TUserLogin, TRoleClaim, TUserToken> and set the specific entities to MultiTenant as you want right?

Yes, exactly.

[GordonBlahut]

I pushed my changes so far. Please take a look.

All tests pass, but I do want to do some cleanup still.

[AndrewTriesToCode]

@GordonBlahut awesome. I'll have to take a close look at how you did the expression for the filter--looks interesting.

Do you mind if I extract out some more properties like TenantNotSetMode into the interface?

[GordonBlahut]

I'll add some comments when I do cleanup. It was a lot of trial and error. I am by no means an expert at expression building. EF will rewrite an expression when it finds a type that is assignable from the context type so that it uses the actual context type. I had some trouble with that because there are 2 separate context hierarchies until I realized the obvious solution to use an interface to cover both. The nested private class is just used to create a closure over the context variable in the expression. It's never actually instantiated.

Sure, feel free to extend the interface where it makes sense. I made it internal for now but maybe it could be made public if there's any value in it?

[AndrewTriesToCode]

@GordonBlahut
I just committed a few changes. I expanded the interface and created more extensions to handle the rest of MultiTenant setup. Let me know what you think.

Now to utilize the features a DbContext need not inherit from MultiTenantDbContext or the MultiTenantIdentityDbContext -- although they are good places to start for simple cases.

All that is needed:

• Implement IMultiTenantDbContext, make sure the Context just returns this.
• call modelBuilder.SetupMultiTenant in OnCreating if using the [MultiTenant] attribute
• Call IsMultiTenant on entities as needed
• call this.EnforceMultiTenant() just before base.Save and base.SaveAsync.

Still to do: any needed test cases and documentation.

[GordonBlahut]

Seems like we're on the same page regarding moving stuff to extension methods. I'll take a look when I clean up my changes.

[AndrewTriesToCode]

Something else I have in mind -- using Roslyn to create a wrapper/decorator for an existing DbContext at runtime. That way you don't even have to derive a subclass or implementation yourself. I'll probably do it in a followup PR.

My use case for that is making Finbuckle work with IdentityServer4 easily although I could probably do so just with what we have so far.

[GordonBlahut]

Working on cleaning up my changes.

IMultiTenantDbContext.Context seems kinda awkward. Would you be ok if I change
EnforceMultiTenant(this IMultiTenantDbContext context)
into
EnforceMultiTenant<TContext>(this TContext context) where TContext : DbContext, IMultiTenantDbContext?

Then IMultiTenantDbContext.Context and self reference is no longer needed.

I've tried it locally and all tests still pass.

I noticed you removed the protected set on the contexts' TenantInfo as part of refactoring the interface. Was this intentional? Not sure if/why anyone would have been using the protected set before but it is a potentially breaking change.

[AndrewTriesToCode]

@GordonBlahut

Hi, I agree your approach to remove the Context thing is a better design. On the protected set--I guess technically it is a breaking change, but I don't think the set is needed. Maybe we should read it then mark it obsolete? I removed it without much thought.

[GordonBlahut]

@achandlerwhite
I just pushed some more refactoring changes. More stuff into extension methods and some renaming, etc. to clean up the public API a bit. Please take a look.

I did not re-add the protected set for TenantInfo.

[AndrewTriesToCode]

@GordonBlahut Thanks, I'll take a look this weekend. I have some family visiting so I might not be able to get to it until Sunday.

[AndrewTriesToCode]

@GordonBlahut looks great. I'm going to update the docs, ideally tomorrow, and then get this merged.

[GordonBlahut]

@achandlerwhite Sounds good.

Not sure if this will be released in a major or minor release, but next time you're doing a major release, consider moving MultiTenantAttribute to Finbuckle.MultiTenant.Core.

That would serve 2 purposes:

1. Anyone who prefers the attribute-based system can use it without a dependency on Finbuckle.MultiTenant.EntityFrameworkCore if their entity classes are in a separate class library.
2. If you or anyone creates non-EntityFramework-based systems for multitenant objects, that attribute could also be used for identifying multienant objects in those cases.

Just an idea.

[AndrewTriesToCode]

@GordonBlahut
I just updated the documentation and made few other changes:

• Moved the MultiTenant data attribute and the related type extension to the .Core assembly per your recommendation.
• Removed GetMultiTenantEntities from IMultiTenantDbContext. This wasn't really used, but I left it in the db context classes marked as obsolete for future removal. The extension on IModel provides the same functionality if anyone needs it.
  -Moved an interface definition for EFCoreStore from the core assembly into the entityframewwork core assemby.

If you have time do you mind taking a look at the documentation and other changes?

Thanks again for your help on this.

[GordonBlahut]

@achandlerwhite I will try to review it tonight.

[GordonBlahut]

@achandlerwhite Code looks good. I added some changes to the EFCore doc.

Maybe need a tests for each of the various MultiTenantIdentityDbContext, MultiTenantIdentityDbContext<TUser>, etc. types to ensure that only certain entities end up being multitenant as documented?

Also maybe some tests involving contexts that do not derive from MultiTenantDbContext or MultiTenantIdentityDbContext? One thing that really needs to be verified is using the fluent API with a context that derives from IdentityDbContext. I believe in that case, any calls to IsMultiTenant on identity entities need to come after base.onModelCreating, otherwise the indexes/keys that Identity adds that IsMultiTenant updates won't exist in the model yet. If that turns out to be accurate, it should probably be added to the docs as well.

It will probably be a few days before I can look at that unless you can do it sooner.

[AndrewTriesToCode]

I added test for a db context directly deriving from IdentityDbContext, confirmed the base OnModelCreating needs to be called first, and updated the docs.

I'll add more tests for the other items you mentioned hopefully tomorrow.

[AndrewTriesToCode]

I submitted test for all the MultiTenantIdentityDbContext variants to make sure our docs are correct. I started to write out tests for all the extension methods and will submit those soon. That effectively makes the existing MultiTenantDbContextShould and MultiTenantIdentityDbContextShould integration tests of all the component extension methods.

[AndrewTriesToCode]

I submitted test for all the MultiTenantIdentityDbContext variants to make sure our docs are correct. I started to write out tests for all the extension methods and will submit those soon. That effectively makes the existing MultiTenantDbContextShould and MultiTenantIdentityDbContextShould integration tests of all the component extension methods.

[AndrewTriesToCode]

@GordonBlahut I've got the last of the extension unit tests completed and everything looks good. I plan to do some more experimenting with it, but I think we have it pretty much where we want it. If you have any more changes let me know.

Thanks for your help!

[GordonBlahut]

@achandlerwhite
Everything looks really good.

The only thing I'm not crazy about is SetupMultiTenant as a name since "Setup" is a noun. "Set up" is a verb but SetUpMuliTenant just looks awkward.

Also I don't really have any better suggestions (naming stuff sucks) so it's just an overly nitpicky thing. Maybe ConfigureMultiTenant as a way to get around it, since it is part of model configuration but that's also not overly descriptive.

Or just leave it as is.

I was also thinking about an IsMuliTenant(false) implementation on the entity type builder extension that would effectively undo the multitenant configuration but am struggling to think of a likely use case. It would likely also be a pain to "undo" the query filter merge, so probably not worth it at this time.

[AndrewTriesToCode]

I like ConfigureMultiTenant lets go with that. Can't think of a use case for IsMultiTenant(false) either--I prefer to add it later if needed. I'm going to go ahead and merge this. Thanks for your help!