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

Support for the composite pattern #970

Closed
alexmg opened this issue Mar 26, 2019 · 7 comments
Closed

Support for the composite pattern #970

alexmg opened this issue Mar 26, 2019 · 7 comments

Comments

@alexmg
Copy link
Member

alexmg commented Mar 26, 2019

The decorator enhancements in #880 did not manage to include support for the composite pattern. This feature was also requested in #776. It is possible to achieve this with named/keyed services and custom registration filtering, but it would be nice to support the composite pattern more directly.

Given the example types below:

public interface IService { }

public class ServiceA : IService { }

public class ServiceB : IService { }

public class ServiceC : IService { }

public class Decorator : IService
{
    public IService Decorated { get; }

    public Decorator(IService decorated)
    {
        Decorated = decorated;
    }
}

public class CompositeService : IService
{
    public IEnumerable<IService> Services { get; set; }

    public CompositeService(IEnumerable<IService> services)
    {
        Services = services;
    }
}

The following unit test should pass:

[Fact]
public void CanCreateCompositeComponent()
{
    var builder = new ContainerBuilder();
    builder.RegisterType<ServiceA>().As<IService>();
    builder.RegisterType<ServiceB>().As<IService>();
    builder.RegisterType<ServiceC>().As<IService>();
    builder.RegisterDecorator<DecoratorService, IService>();
    builder.RegisterType<CompositeService>().As<IService>();
    var container = builder.Build();

    var service = container.Resolve<IService>();

    Assert.IsType<CompositeService>(service);
    var composite = (CompositeService)service;

    var services = composite.Services.OfType<DecoratorService>().ToArray();
    Assert.Equal(3, services.Length);
    Assert.IsType<ServiceA>(services[0].Decorated);
    Assert.IsType<ServiceB>(services[1].Decorated);
    Assert.IsType<ServiceC>(services[2].Decorated);
}

When resolving IService the CompositeService should be returned. It should be provided instances of ServiceA, ServiceB and ServiceC that are decorated with DecoratorService. The CompositeService itself is not decorated (though I suppose it could be if desirable).

Instead of having the composite identified automatically, another option would be to identify the composite type explicitly at the point of registration.

builder.RegisterComposite<CompositeService, IService>();

The same level of support should be applied to open generic registrations and decorators as well.

This needs further consideration. Hopefully there is enough here to capture the general intent of the feature.

@johneking
Copy link

I've looked into the internals and have an idea for an approach for this. It boils down to two generalisations of existing code so hopefully wouldn't be too invasive:

  1. modify DecoratorService and the associated structures to accommodate data and logic for composites
  2. switch decorator (and composite) resolution to be recursive instead of iterative

I can provide more detail on the specifics but might have a go at pulling some code together first. Any initial thoughts on that approach in general?

@alexmg
Copy link
Member Author

alexmg commented Jul 23, 2019

The help would definitely be welcome and spiking out some potential changes in the code might be the best starting point. I usual find I discover a few things while doing that and end up revising the plan a few times.

I do have some fixes for a few of the decorator issues that require breaking changes. Those should be coming into the the develop branch soon. I don't think there is anything in those changes that would fundamentally change things from the composite perspective, though having access to the Service being resolved may provide additional options.

@johneking
Copy link

Cool, I might actually wait for that update to develop before going too much further into this. I can already see a blocker with the approach above whereby TryDecorateRegistration would actually be required to act on all IComponentRegistrations for a Service, but that list of registrations gets reduced to a single entry pretty high up the call stack. Those incoming changes sound like they're at least partly connected to that and would at least be useful just for the extra info while exploring that a bit more.

@johneking
Copy link

I'm still looking at this and thinking that the optimal way to approach this would actually be a significant structural change, but might be possible. To expand on the previous comment, at the moment when resolving a service the broad sequence of events looks like:

  • receive request for service
  • locate single registration
  • start resolving for the specific registration
    • check scope for existing instance, otherwise activate new
    • apply decorators

The CollectionRegistrationSource provides in-built IEnumerable access by just retrieving all registrations for a service and resolving each.

The problem with composites is that they break the 1:1 relationship between service resolution and registration; it doesn't make sense to resolve a single registration if that feeds into (and is replaced by) a composite which acts over multiple registrations. Essentially we don't even know how many components we'll finally get back when resolving a list of registrations.

If we didn't need to mask any of the original registrations which get included in the composite (do we definitely need to do that?) or worry about ordering in relation to decorators, we could probably just add a new registration source or similar - but that seems quite limited. It seems likely that we'd also want to extend the registration to include conditions on where to apply the composite.

To deal with it more thoroughly, I'd propose a flow more like:

  • receive request for service
  • check scope for existing instance, otherwise activate new
  • find all registrations
  • apply decorators and composites (using broadly the same structure)
    • enumerate decorator structure and registrations
    • resolve each root registration when requested
  • by default, return first enumerated component

The enumerable decoration part is broadly possible via the approach outlined in #967 where the decoration process is inverted (but mostly still results in the same resolution order at least). It just requires making the return types for each part of that process enumerable, which isn't too crazy. It really falls over once we involve the deferred resolution for #967 but maybe that could just be an exception.

The main problem is the part before that, specifically that connecting a service request to a single registration is quite pervasive and probably with good reason. At the moment it's unclear to me as to how expanding some of that upstream code to allow multiple registrations to be passed around would affect performance, or have other significant side-effects. It's also not clear how things like the CollectionRegistrationSource would interact with components which have composites applied to them - maybe an approach like the above could actually be used to supersede it too?

I'll keep looking at this approach for a while to see exactly what the impacts would be. Interested to hear any thoughts or alternative approaches in the meantime!

@tillig
Copy link
Member

tillig commented Sep 2, 2019

In the past for special case things (like Startable) we've created a special service type that can be used when registering a particular component. It acts like a flag to indicate special treatment. Perhaps there's some sort of AsCompositeFor<T> registration that would add the special service to the registration and that service would be an indicator to exclude it from collections and kick off the special behavior.

I feel like there's a potential perf issue here. Composites would be a rare thing but if you have to check for a composite on every resolve then there's overhead added for every component. Somehow we'd need to ensure we don't take a hit on every resolve for this.

I've often noodled about a middleware style pipeline implementation for resolutions. Something where a resolution "request" goes through the stack of middleware, hits the concrete set of registrations, and on the way back out, the "response" gets things decorated/composited/etc. There's something in the back of my head making me think that'd be helpful here, like if there was a composite middleware, that would have the short list of registered composites and it'd be a faster check. I'm sort of rambling. It's the weekend and I've not got a wholly formed thought here. But as the decorator stack starts getting resolve contexts and things added to it, I wonder if that's a good model for the larger resolution flow.

@johneking
Copy link

A middleware / pipeline approach is exactly how I could see something like this fitting together - the existing decoration step when resolving already does something a bit like this. I've actually just renamed my modifications for #967 with that in mind. Allowing that layer sequentially between the original Resolve request for a service and retrieving underlying components is where it could be powerful and I feel like going down that route in the long-run would be preferable to special-casing for composites. In a practical sense they seem very similar to decorators in terms of when they're applied, filtering, chaining etc. They just have a different input type.

This maybe points to a larger piece of work, but the way I'd see it working is that it's possible to pre-calculate a resolution pipeline per service (at container build time or lazily?), based on special registration types in the container (as they are now). For example, for service T, we can cache a structure which attempts to resolve a sequence of component instances like decorator -> composite -> decorator -> implementation, for any given component context.

Regarding enumerable and performance: conceivably with the above structure the resolution pipeline would completely exclude the need to enumerate the underlying registrations if you didn't have a related composite registration. So even if every section of the pipeline worked with IEnumerables, every section would still only iterate over the first entry for the vast majority of scenarios. That's assuming there aren't other significant performance gains in the existing code where references to single registrations speed things up.

I had a play around with a sandbox structure for this and it ended up a little more complex than I intended initially, but I think does demonstrate how this might work: https://gist.github.com/johneking/3198d6ed9b169add8bb4d53978a86bf5

@tillig
Copy link
Member

tillig commented Jun 25, 2020

Resolved via #1152

@tillig tillig closed this as completed Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants