Add WithPerTenantOptions overload that exposes IServiceProvider

[goldsam]

My team has ran into several cases where having access to IServiceProvider in the context of resolving options is necessary. We have achieved this by reimplementing WithPerTenantNamedOptions, but having an out-of-the box override with the following signature would be preferred:

public FinbuckleMultiTenantBuilder<TTenantInfo> WithPerTenantNamedOptions<TOptions>(string? name,
    Action<IServiceProvider, TOptions, TTenantInfo> tenantConfigureNamedOptions) where TOptions : class, new()

[AndrewTriesToCode]

This is a good idea. Do you want to submit a PR or if not I can add this in before too long?

[goldsam]

I would be happy to put together a PR 😊

[AndrewTriesToCode]

Thanks for the PR. It has occurred to me that if I follow the Options Builder approach described here you'll be able to use DI to build Options. I think what I am going to do is implement services.ConfigurePerTenant (equivalent to WithPerTenantOptions today) and services.AddOptions...Configure<T1, T2...> to support the need for DI.

I'd probably deprecate WithPerTenantOptions in this case too and steer people toward the service collection extensions described above since they closer match the .NET design. What do you think about that as an idea?

[goldsam]

I really like that solution. It's clean and familiar😀.

Do you have time to address this now? If not, I could work on an alternative PR.

[AndrewTriesToCode]

I probably can I the next two weeks -- day job requires me to travel next week so either I will get no Finbuckle coding time or plenty of time--hard to say. If you have time I'd welcome a PR on that or otherwise use your own extension method for the time being.

[goldsam]

@AndrewTriesToCode I've got some time this week to work on this, and wanted to get your feedback on a proposed API which aligns with your previous comment above:

1. Instead of a method ConfigurePerTenant on IServiceCollection, how about the following extension methods:
   • static OptionsBuilder<TOptions> AddPerTenantOptions<TOptions>(this IServiceCollection services)
   • static OptionsBuilder<TOptions> AddPerTenantOptions<TOptions>(this IServiceCollection services, string? name)
   • static IServiceCollection ConfigurePerTenant<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions)
   • static IServiceCollection ConfigurePerTenant<TOptions>(this IServiceCollection services, string? name, Action<TOptions> configureOptions)
   • static IServiceCollection ConfigurePerTenant<TOptions, TTenantInfo>(this IServiceCollection services, Action<TOptions, TTenantInfo> configureOptions)
   • static IServiceCollection ConfigurePerTenant<TOptions, TTenantInfo>(this IServiceCollection services, string? name, Action<TOptions, TTenantInfo> configureOptions)
2. Add public classes ConfigurePerTenantOptions<TOptions, TTenantInfo> and ConfigurePerTenantNamedOptions<TOptions, TTenantInfo> to mirror ConfigureOptions<TOptions> and ConfigureNamedOptions<TOptions> respectively.
3. Strictly speaking, ITenantConfigureNamedOptions<TOptions, TTenantInfo> is unnecessary since the same functionality can be achieved using the classes from (2) . This class could be marked as obsolete.
4. An implementation of MultiTenantOptionsFactory<TOptions> (with no dependency on TTenantInfo) can provided provided which relies simply on IMultiTenantContextAccessor and collections of IConfigureOptions<TOptions>, IPostConfigureOptions<TOptions> and IValidateOptions<TOptions>. This class will support the APIs in (1).
5. Update WithPerTenantOptions(...) and WithPerTenantNamedOptions on FinbuckleMultiTenantBuilder<T> to use the above APIs.

My goal with the above proposal is to add support for OptionsBuilder<TOptions> and mirror the original Options API as closely as possible. The only thing I don't like is that it spreads the API between IServiceCollection and FinbuckleMultiTenantBuilder<T> which hinders discoverability. This is probably not a big deal as long as the options methods on FinbuckleMultiTenantBuilder<T> remain available with perhaps an updated comment hinting at the new IServiceCollection API.

[AndrewTriesToCode]

Thanks that is very generous of you and this sounds like a great approach. I agree with all of it. I think we should mark WithPerTenantOptions obsolete since I see this as being closer to the normal way options work.

Also if you are able to do unit tests and keep documentation updated it is helpful, otherwise I’ll create a separate branch from main to merge this into so I can complete those before merging to main.

[goldsam]

@AndrewTriesToCode I already have something working for you to review. I think it might be a good idea to create a separate branch for me to create a PR against. I have already pushed to my fork in the meantime.

I introduced a new MultiTenantOptionsFactory<TOptions> class, but left the original MultiTenantOptionsFactory<TOptions, TTenantInfo> implementation. Some other funky things I introduced are ITenantConfigureNamedOptionsWrapper<TOptions> and its implementation TenantConfigureNamedOptionsWrapper<TOptions> which are both non-public implementation details.

I look forward to your feedback! 😊

[AndrewTriesToCode]

Ok, I just created a branch, feat/better-per-tenant-optionsOn Jul 16, 2023, at 10:25 PM, Sam G @.***> wrote: @AndrewTriesToCode I already have something working for you to review. I think it might be a good idea to create a separate branch for me to create a PR against. I have already pushed to my fork in the meantime. I introduced a new MultiTenantOptionsFactory class, but left the original MultiTenantOptionsFactory<TOptions, TTenantInfo> implementation. Some other funky things I introduced are ITenantConfigureNamedOptionsWrapper and its implementation TenantConfigureNamedOptionsWrapper which are both non-public implementation details.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[goldsam]

Thanks for accepting my changes! I look forward to the next release featuring these changes 😀

[goldsam]

Just checking in. Any plans to merge my branch any time soon?

[AndrewTriesToCode]

Hi, I do plan to merge relatively soon. It'll be the next release by mid November at the latest. I've been too busy with day job stuff lately. I also am adding to what you provided to get a few more things in.

[LDP-Dries]

Hi Andrew,

Is anything stopping the branch from being merged?

[AndrewTriesToCode]

There another breaking change I’m close to finished testing on. This has taken me longer overall and I apologize.

[AndrewTriesToCode]

Actually the branch was merged, I use haven’t done a release yet.