Finbuckle.MultiTenant

Finbuckle.MultiTenant copied to clipboard

Currently, the IMultiTenantStrategy and the ITenantResolver have methods requiring passing the HttpContext as an object, which doesn't seems needed, since it is accessible from the middlewares.

The change doesn't seems to be breaking for real world usages, but can be for some tests that passes an object to the method.

Two packages needs to be added for this change (both targeting .netstandard2.0) :

• Microsoft.AspNetCore.Http.Abstractions (for Finbuckle.MultiTenant and Finbuckle.MultiTenant.Test)
• Microsoft.AspNetCore.Http (for Finbuckle.MultiTenant.Test)

This changes would simplify the API and bring a small improve memory improvement (since the context is not boxed anymore). (PR can be ready if needed)

Help again. There are situations where these are used outside of asp.net core so it can't assume it will be a certain type. Also boxing occurs when going from a value to an object and I'm pretty sure HttpContext is a class so not a performance concern there. Although it could be in some other situations.

Can you provide a specific example of what you had in mind?

Also boxing occurs when going from a value to an object and I'm pretty sure HttpContext is a class so not a performance concern there.

My bad, used bad phrasing.

Can you provide a specific example of what you had in mind?

Currently, when you implement the IMultiTenantStrategy interface, you need to cast the context as a HttpContext :

public class MyTenantStrategy : IMultiTenantStrategy
{
    public Task<string?> GetIdentifierAsync(object context)
    {
        if (context is not HttpContext httpContext)
            throw new ArgumentException($"{0} is null or is not of {nameof(HttpContext)} type", nameof(context));

        return Task.FromResult<string?>("identifier");
    }
}

This is done in most (if not all) of the implementations included with the library (e.g BasePathStrategy).

The change would be :

public interface IMultiTenantStrategy
{
    Task<string?> GetIdentifierAsync(HttpContext context); // change from object to HttpContext 
}

public class MyTenantStrategy : IMultiTenantStrategy
{
    public Task<string?> GetIdentifierAsync(HttpContext context) // no need to cast
    {
        return Task.FromResult<string?>("identifier");
    }
}

This takes HttpContext from Microsoft.AspNetCore.Http.Abstractions. Maybe I'm missing something, but I don't see how this package should be used outside of AspNetCore, and the docs are focused on AspNetCore.

I need to update the docs or start a blog or something. It is used in some other areas like workflows, console applications, queue triggers in Azure where the queue message contains a tenant identifier... really anything. Dotnet host model allows any app to use dependency injection and other features -- in fact ASP.NET Core is build on top of the host model.

I do see what you mean and if it were always HttpContext I'd definitely change it! I could maybe make it a template class but that just adds more complexity--not sure if worth it.

This makes sense. It's just that i wasn't expecting the library to be used is such scenarios. Another possible alternative would be to specify the type in ITenantResolver/IMultiTenantStrategy with a TContext (e.g ITenantResolver<TTenant, TContext>), and then the type would be changeable, but this is still some big refacto to do.

Docs on these usages would be welcome 😁