Umbraco-cms: IPublishedSnapshotService initialization & event binding changes

[Shazwazza]

Based on this discussion https://github.com/umbraco/Umbraco-CMS/pull/9415#discussion_r529130445

There are issues with IPublishedSnapshotService and the way it is initialized. It's constructor currently binds to all of the Umbraco service events that it uses to listen for changes and react accordingly. This is problematic because in some cases like integration tests we don't want this to automatically bind to all of these events just because the service has been created.

The discussion on that thread hints at having an "Initialize" method but that has a temporal coupling issue which we don't want. It was further discussed that we should look into having a IPublishedSnapshotServiceFactory which would be used to create the service so it's created lazily when needed. The theory would be so that the service is not constructed until it is actually needed and therefore the events are not subscribed to.

This doesn't actually work though. The IPublishedSnapshotService is injected into a number of places and it is a singleton. Changing this to be a factory would mean that the service is no longer a singleton but is transient because anywhere the factory is injected, it will create a new instance of the service. That could be change by having the factory only create a single instance (Lazy) but that isn't right either.

The main issue here is that the ctor of PublishedSnapshotService binds to service events. Should it be the responsibility of this service to bind to events? Or should it be the responsibility of the application (i.e. another class) to bind events to the service methods? Another issue is that PublishedSnapshotService binds to the IUmbracoApplicationLifetime.ApplicationInit event which it uses in order to initialize itself and during this process will rebuild the caches if they are empty.

• Do we want PublishedSnapshotService and the front-end caches to initialize/load/etc... during all integration tests? In my opinion we should only have the front-end caches initialized when we want to use them for integration tests, not for all of them. This is also going to slow down the testing phase probably by quite a lot.
• Is it the responsibility of PublishedSnapshotService to bind to the service and ApplicationInit events? Or should there be another class that manages these bindings to call the appropriate methods on the service?

Possible solutions

These are just simple examples of what we could do or start with. I'm sure you have some great ideas on this so let's discuss on this issue :)

• For integration tests don't register the real PublishedSnapshotService and instead register a fake/testing one that doesn't really do anything
• Create new IPublishedSnapshotEvents class which does all of the event binding and calls into the public methods of IPublishedSnapshotService. For some integration tests this could be registered as a fake/test/noop service so no binding occurs. Also means that the PublishedSnapshotService class is reduced in size.

Other related notes

These are outside the scope of solving the event issues but it's worth noting here

• Should the IUmbracoApplicationLifetime.ApplicationInit exist at all? There's very few usages of this, it's only for nucache and for models builder. This could/should probably be handled during the umbraco startup phase? Similarly the IUmbracoApplicationLifetimeManager might not need to exist at all. I guess this is all related to event refactoring too.
• PublishedSnapshotService is a gigantic class that should be split up

[rustybox]

As hinted above, presumably now that #9478 has been merged for #9397, one could wire up the events via service collection and register a noop/stub event aggregator for test scenarios where the side effects are not required.

Should the IUmbracoApplicationLifetime.ApplicationInit exist at all?

Probably not anymore as we can fire starting/started events via the event aggregator.

In my opinion we should only have the front-end caches initialized when we want to use them for integration tests

Absolutely, if they aren't actively under test then it's just adding confusion and slowing the tests down.

https://martinfowler.com/articles/practical-test-pyramid.html#IntegrationTests

---

If this is causing immediate issues and requires a short term hack fix, the PublishedSnapshotService.InitializeRepositoryEvents call could move to first line of PublishedSnapshotService.OnApplicationInit before the RuntimeLevel check.

[Shazwazza]

I've begun work on the front-end rendering tasks and as it turns out the middlewares that execute during startup along with these old event and event handlers like ApplicationInit do so on different threads in an async manner. This throws things all out of whack and it's super confusing due to these app init events and how they are raised. So I'll need to fix this all anyways and then as an added bonus it will fix the tests too.

I will start by removing these:

• IUmbracoApplicationLifetimeManager
• IUmbracoApplicationLifetime.ApplicationInit. The other members of IUmbracoApplicationLifetime will need to be reviewed
• AspNetCoreComponent

As hinted above, presumably now that #9478 has been merged for #9397, one could wire up the events via service collection and register a noop/stub event aggregator for test scenarios where the side effects are not required.

Yep so the end-goal will be the event aggregator solution but this requires a large amount of initial work (i.e. #9397). In the meantime I'm going to look into making these changes which will make it easier for that to happen:

• Split PublishedSnapshotService so that event handing is in a separate class
• PublishedSnapshotService will then just have the public methods to perform the actions
• Events will be wired up in a UseUmbracoCache method for now - we can change this later

I would love to know best practices of how you would do the event aggregator for this example and to be able to "register a noop/stub event aggregator for test scenarios"? Would it be something like this?

// handles all notifications for events for the published snapshot service
public interface IPublishedSnapshotServiceNotifyHandler : INotificationHandler<NotificationA>, INotificationHandler<NotificationB>, INotificationHandler<NotificationC>
{
}

And then have a noop implementation of this for tests and an real one for the app. Or would you do that a different way?

[rustybox]

I'm not sure if the IPublishedSnapshotServiceNotifyHandler interface provides any utility, you would still have to make all the registrations

builder.AddNotificationHandler<NotificationA, ConcretePublishedSnapshotServiceNotifyHandler>()
builder.AddNotificationHandler<NotificationB, ConcretePublishedSnapshotServiceNotifyHandler>()
builder.AddNotificationHandler<NotificationC, ConcretePublishedSnapshotServiceNotifyHandler>()

By stub and noop EventAggregator I meant something like this, picking between stub and noop as appropriate for the test.

Essentially I'm swapping out the EventAggregator, not the handlers.

https://gist.github.com/rustybox/2eb3815b767edd9b5dc7bc751572cf17

Removed noop implementation from example, it isn't really useful as you can just use the stub with 0 whitelisted items, or just a Mock.Of

TODO: EventAggregator should be registered unique in UmbracoBuilder.cs

[lars-erik]

I'm not sure I'm in on a whitelist either. 🤓

Why does the stapshotservice need to attach to events itself? Shouldn't that be handled during composition?

In that case, it's just a matter of _not_ registering things that shouldn't run. Thought that was the point of this whole process. 😜

(Guess I'm asking for a TL;DR.)

[rustybox]

We're discussing selectively disabling notification handlers during integration testing.

PublishedSnapshotService should run in production but not during some tests, assume it's wired up in the test base classes deep down inside one of the umbraco builder extensions, here is a tool in your arsenal for preventing it running in a given test.

Not registering an notification handler in the first place is equally valid, we don't have to use the stub everywhere, you register it to replace the default event aggregator when required.

Why does the stapshotservice need to attach to events itself? Shouldn't that be handled during composition?

It doesn't attach events to itself that would happen during composition, but it does need to implement the INotificationHandler interface to enable it to be registered against the event aggregator,

[lars-erik]

assume it's wired up in the test base classes deep down inside one of the umbraco builder extensions

Rip it out! Or make the test base overridable. 🤷‍♂️
Again - peeps are gonna use things if they're available. Whitelists _will_ be abused instead of proper use of composition.
Don't put a patch over a rust hole. Cut it out and weld properly. Or preferably screws and a gasket. ;)

[rustybox]

peeps are gonna use things if they're available. Whitelists will be abused instead of proper use of composition.

I don't have a problem with people using a stub during a test :)

It's OK to have a choice between tools.

This is nice and clean and obvious perhaps more-so than overriding UmbracoIntegrationTest.ConfigureServices with a very explicit list of what is required for a given test, what if you require the event handler to fire in one test but not the next, that means you need two classes with different ConfigureServices.

Although thinking about It now a blacklist is probably more appropriate than a whitelist.

[rustybox]

Just to expand on

perhaps more-so than overriding UmbracoIntegrationTest.ConfigureServices

Explicitly opting out of NuCache.OnSomeThingHappened within a given test case clearly documents the difference between the test case and production setup.

Whereas overriding ConfigureServices with an omission for NuCache.OnSomeThingHappened is much less clear when looking at a given test case, you need to inspect both the test itself and the classes ConfigureServices to understand what's happening and even then without a comment documenting the omission it wouldn't be obvious.

[lars-erik]

I don't have a problem with people using a stub during a test :)

I might have misunderstood. If this is just visible on a stub it's a passable workaround for living with closed test base-classes.
I just don't want a "whitelist" concept in the core.

Although thinking about It now a blacklist is probably more appropriate than a whitelist.

Get with the times and call it "exception-list" or something. ;)

[rustybox]

If this is just visible on a stub

Absolutely this is an alternate implementation of IEventAggregator that should live inside Umbraco.Tests.Integration to use as a test double.

---

Switched sample stub over to ignore vs whitelist.

---

• Want everything to run, use default event aggregator
• Want no handlers to run, services.AddUnique(Mock.Of
• Want some handlers omitted
  
  
  
  • omit the notification handler registration
  
  
  • or use the stub and add given handler to ignore list.
  
  

[Shazwazza]

Essentially I'm swapping out the EventAggregator, not the handlers.

Thanks @rustybox for the code and insight, makes sense to me.

Why does the stapshotservice need to attach to events itself?

It doesn't, this was why I posted my comments above https://github.com/umbraco/Umbraco-CMS/issues/9510#issuecomment-741412172 and splitting up this gigantic class. This class was doing all sorts of DB interaction, etc... It's now split into a repository/service, PublishedSnapshotService and PublishedSnapshotServiceEventHandler (which is now run on a UseUmbracoContentCache startup method). It will require more cleanup but is better. With the event aggregator will probably be much simpler still which is why I was asking these questions :)

Shouldn't that be handled during composition?

Yes with event aggregator that would be the way. For now it's done with PublishedSnapshotServiceEventHandler which is a means to an end for now. When this PR is ready for review would love a discussion on there about