Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeProvider is not registered properly when using RegisterType #312

Closed
Enngage opened this issue Jan 28, 2022 · 13 comments · Fixed by #347
Closed

TypeProvider is not registered properly when using RegisterType #312

Enngage opened this issue Jan 28, 2022 · 13 comments · Fixed by #347
Assignees

Comments

@Enngage
Copy link
Member

Enngage commented Jan 28, 2022

Brief bug description

When registering type provider with builder.RegisterType<TProvider>().Named<ITypeProvider>('x'); the ITypeProvider is not actually registered and GetType method within provider is never called when client is fetched using IDeliveryClientFactory

Expected behavior

TypeProvider should be invoked when delivery client is initialized through IDeliveryClientFactory

Test environment

.NET 6, VS 2022, Kentico.Kontent.Delivery@16.0.0-beta5

@Simply007
Copy link
Contributor

One note according context - builder.RegisterType<TProvider>().Named<ITypeProvider>('x'); is being used when using Autofac configuration for DI container - as described here: https://github.com/Kentico/kontent-delivery-sdk-net/wiki/Accessing-Data-From-Multiple-Projects#registering-multiple-type-providers

I have identified the issue itself. It is caused by behavior:

  • AutofacServiceProvider.GetService is using IComponentContext.ResolveNamed which properly initialized ITypeProvider on the IDeliveryClient level, but not on ModelProvider level.

I have tried multiple ways to configure services for chaining the dependency, but it seems possible to use AutoFac configuration from the outside:

builder.RegisterType<TProvider>().Named<ITypeProvider>("x");
builder.RegisterType<ModelProvider>().Named<IModelProvider>("x") // default implementation of ModelProvider
    .WithParameter(Autofac.Core.ResolvedParameter.ForNamed<TProvider>("x"));

This seems to work as showcased in #315


I have also tried to rewrite the multiple dependencies on the TypeProvider for just one from Delivery Client and it works as well - see #314


I am not sure about the next step here. We definitely need a more robust solution for all possible transient dependencies we might have for DI setup. This is just one dependency.

@Simply007
Copy link
Contributor

Maybe @petrsvihlik or @tomasjurasek might point us to some best practices?

@Nickm615
Copy link

Is it possible to show an example of the workaround code you've implemented?

@Simply007
Copy link
Contributor

Simply007 commented May 23, 2022

Is it possible to show an example of the workaround code you've implemented?

Yeap - I can see the previous answer that was supposed to showcase a workaround and link a draft to a possible fix might be confusing.

Currently in the released version, if you want to register a named client with named TypeProvider (and thus a named ModelProvider). you need to specifically define you want the specific ModelProvider to be used to create the name TypeProvider like that:

builder.RegisterType<TProvider>().Named<ITypeProvider>("x");
builder.RegisterType<ModelProvider>().Named<IModelProvider>("x") // default implementation of ModelProvider
    .WithParameter(Autofac.Core.ResolvedParameter.ForNamed<TProvider>("x"));

So that code would be

public class Startup
{
        public void ConfigureContainer(ContainerBuilder builder)
        {
            builder.RegisterType<ProjectACustomTypeProvider>().Named<ITypeProvider>("projectA");
            builder.RegisterType<ModelProvider>().Named<IModelProvider>("projectA") 
                .WithParameter(Autofac.Core.ResolvedParameter.ForNamed<TProvider>("projectA"));
            builder.RegisterType<ProjectBCustomTypeProvider>().Named<ITypeProvider>("projectB");
            builder.RegisterType<ModelProvider>().Named<IModelProvider>("projectB") 
                .WithParameter(Autofac.Core.ResolvedParameter.ForNamed<TProvider>("projectB"));
        }
}

And then use it in i.e. you controller

public class HomeController : Controller
{
    private IDeliveryClient _deliveryClient;

    public HomeController(IDeliveryClientFactory deliveryClientFactory)
    {
        _deliveryClient = deliveryClientFactory.Get("projectA"); // or "projectB"
    }
}

The root cause of the issue is based on the "Multilevel" dependency

DeliveryClient -> TypeProvider -> ModelProvider

but the context with the service "name" is not propagated to the ModelProvider correctly

The draft of the fix (#315) is basically narrowing down the multilevel dependencies, but it is pretty big and currently I don't see this being merged any type soon.


There is also another workaround for the current released version and it is to create 2 separate ASP.NET core services with the differently configured delivery client and then use them in dependency injection configuration.

This might be probably the cleanest way to use multiple delivery clients and remove the named client feature from the SDK.

@happywisepaul
Copy link

@Simply007 I cannot get your sample code for registering ModelProvider to work since it's an internal class in Kentico.Kontent.Delivery.ContentItems.ModelProvider.cs. Did you actually get that line to compile?

@Simply007
Copy link
Contributor

@Simply007 I cannot get your sample code for registering ModelProvider to work since it's an internal class in Kentico.Kontent.Delivery.ContentItems.ModelProvider.cs. Did you actually get that line to compile?

Thx @happywisepaul - I did. But I was testing the code in the Test project to be able to flexibly adjust the code if something come up. What I didn't realize, was that the test project/assembly has InternalsVisibleTo being turned on in my dev development.


In that case, I need to dig into the problem ad try to find mode suitable solution.

The easiest workaround right now is to create 2 services where you register delivery client singletons and register these two services in your DI. This might also be the way we might go as a recommended solution, because there are more dependencies, between the services and the named service functionality is not fully covered in the solution from what I have just skimmed - but this is my initial thought, I need to investigate it a bit more,

Could you briefly summarize our use case - what services do you need to have a different setup between delivery clients?

@Simply007
Copy link
Contributor

Simply007 commented May 26, 2022

Hello @happywisepaul,

I have created a separate sample project with a showcase of the problem, the nonworking solution, and then the workaround, that should be sufficient for you.

Everything is being in this branch: https://github.com/Kentico/kontent-delivery-sdk-net/tree/preparation-issue-312


This issue is not solved. I will consult a more robust approach because the fix in #314 is rather a hack. Currently, it looks like we will extend the codebase by a couple of factories, that will allow registering delivery clients. And the named services might be deprecated/removed, but this is just a brief idea.

Could you briefly summarize our use case - what services do you need to have a different setup between delivery clients? It might help us understand and draft more suitable resolution.

@happywisepaul
Copy link

Hello @Simply007,

Thank you for showing a sample code tfor registering 2 delivery clients with different TypeProvider using 2 Singletons rather than Autofac's named services. I think this will work for me.

Could you briefly summarize our use case - what services do you need to have a different setup between delivery clients? It might help us understand and draft more suitable resolution.

The use case is one Kentico Kontent project containing models for 2 ASP.NET Core websites with namespace A and B; some models are specific to namespace A, some specific to namespace B, and some are shared. The ones that are shared rely on a class library (compiled into a Nuget package) with namespace C. We want the class library to be developed independently, should rarely change, and used only a stable and only a handful number of Kontent models. Most new models are specific to either A or B, and therefore the TypeProvider needs only be updated in the 2 websites, and don't necessitate change to C.

We use the strongly typed models generated by Kontent Generator for the respective A, B, and C. The MVC Solution for A would create another delivery client for the class library's use with TypeProvider generated for C.

@Simply007
Copy link
Contributor

We did some initial discussion about the Named services and it seems like it might be because Named services might be an antipatern or at least this might be one of the reasons why it is not implemented right in the dotnet (dotnet/runtime#64427).

As the result, we have drafted the skeleton for possible substitution: f9fc1f6.

I will discuss the further approach with the maintainers.

@Simply007
Copy link
Contributor

I have drafted the first draft of the replacement for NamedService - it is completely independent from Autofac - it uses default container.

This is how it could work: https://github.com/Kentico/kontent-delivery-sdk-net/blob/preparation-issue-312/DeliverySDKWithAutofac/Program.cs#L49-L71

The skeleton is DeliveryClientDictionaryFactoryBuilder and DeliveryClientDictionaryFactory (naming is hard - we might come out with something better).

@Simply007
Copy link
Contributor

Hello @happywisepaul and @Nickm615,

could you take a look at #333 - especially on Program.cs. There should be a draft of the new way of registering multiple clients - you will just use the Factory - and the Autofac Named Service provider would be deprecated.

I would be glad to hear any feedback you might have on this step.

@Simply007
Copy link
Contributor

Simply007 commented Jun 29, 2022

RFP - replace named clients with delivery client factory

Please comment on this issue - feedback is more than welcome!

Current solution

Currently, it is possible to register services (like Delivery Client) with names. Unfortunately, the dependencies among the services are a bit more complicated and there is an issue (this issue) if you want to register sum sub-services, they are not being registered properly.

image

Suggested solution

Replace Named services to register multiple client by DeliveryClient Factory

The alpha is implemented in #333

Showcase is here: https://github.com/Kentico/kontent-delivery-sdk-net/blob/preparation-issue-312/DeliverySDKWithAutofac/Program.cs

services.AddDeliveryClientFactory(
    factoryBuilder => factoryBuilder
        // Simple client with custom type provider
        .AddDeliveryClient(
            ClientA,
            deliveryOptionBuilder => deliveryOptionBuilder
                .WithProjectId(ClientAProjectId)
                .UseProductionApi()
                .Build(),
            optionalClientSetup =>
                optionalClientSetup.WithTypeProvider(new ProjectAProvider())
        )
        // Another simple client with another custom type provider
        .AddDeliveryClient(
            ClientB,
            deliveryOptionBuilder => deliveryOptionBuilder
                .WithProjectId(ClientBProjectId)
                .UseProductionApi()
                .Build(),
            optionalClientSetup =>
                optionalClientSetup.WithTypeProvider(new ProjectBProvider())
        )
        // Client using appsettings.json to load delivery options
        .AddDeliveryClient(
            "C",
            _ =>
            {
                var options = new DeliveryOptions();
                config.Configuration.GetSection("MultipleDeliveryOptions:C").Bind(options);
                return options;
            }
        )
        // Another delivery client using appsettings.json to load delivery options
        .AddDeliveryClient(
            "D",
            _ =>
            {
                var options = new DeliveryOptions();
                config.Configuration.GetSection("MultipleDeliveryOptions:D").Bind(options);
                return options;
            }
        )
        // Cache client using Memory Cache
        .AddDeliveryClientCache(
            "MemoryCache",
            deliveryOptionBuilder => deliveryOptionBuilder
                .WithProjectId(ClientAProjectId)
                .UseProductionApi()
                .Build(),
            CacheManagerFactory.Create(
                new MemoryCache(new MemoryCacheOptions()),
                Options.Create(new DeliveryCacheOptions
                {
                    CacheType = CacheTypeEnum.Memory
                })),
            optionalClientSetup =>
                optionalClientSetup.WithTypeProvider(new ProjectAProvider())
        )
         // Cache client using Distributed Cache
        .AddDeliveryClientCache(
            "MemoryDistributedCache",
            deliveryOptionBuilder => deliveryOptionBuilder
                .WithProjectId(ClientAProjectId)
                .UseProductionApi()
                .Build(),
            CacheManagerFactory.Create(
                new MemoryDistributedCache(Options.Create(new MemoryDistributedCacheOptions())),
                Options.Create(new DeliveryCacheOptions
                {
                    CacheType = CacheTypeEnum.Distributed
                })),
            optionalClientSetup =>
                optionalClientSetup.WithTypeProvider(new ProjectAProvider())
        )
        .Build()
    );
}).Build();

And once you have everything registered, you can use the factory:

// Load the factory, or get it from container
var deliveryClientFactory = host.Services.GetRequiredService<IDeliveryClientFactory>();

var firstClient = deliveryClientFactory.Get(ClientA);
var secondClient= deliveryClientFactory.Get(ClientB);
// ...

Please comment on this issue - feedback is more than welcome!

@Simply007
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants