OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.38k stars 1.12k forks source link

Add events to the Output Cache filter in order to allow modules to extend it #5790

Open paynecrl97 opened 9 years ago

paynecrl97 commented 9 years ago

3rd party (or even 1st party) modules are currently very restricted in the way that they can interact with the Output Cache functionality.

I propose adding an event handler interface to allow developers to hook into specific events offered by the Output Cache filter.

This could look something along the lines of this:

public interface IOutputCacheEventHandler : IEventHandler
{
    void Caching(string cacheKey, CacheItem cacheItem);
    void Cached(string cacheKey, CacheItem cacheItem);
    void Retrieving(string cacheKey);
    void Retrieved(string cacheKey, CacheItem cacheItem);
}

Example use cases would be:

DaRosenberg commented 9 years ago

:+1:

sebastienros commented 9 years ago

I also prefer this suggestion to the old PR. One comment would be to have a CachedItemContext and CachingItemContext instead of two parameters, and also add a way to Cancel in the Caching one.

DaRosenberg commented 9 years ago

@sebastienros A question on that. When we introduce the IOutputCacheControlProvider interface it will be encouraged to implement that interface to "cancel" caching of a request. If we also add a cancellation mechanism to the Caching event here, there will be two ways to prevent caching. Do you see an issue with this, or do you think it's fine?

sebastienros commented 9 years ago

Well I think these event provides enough extensibility and the other interface is not necessary,

DaRosenberg commented 9 years ago

Well... the plan is for the other interface to have much more functionality for cache control such as:

All these things will be necessary for the next step which is to make output cache configuration much more composable and collaborative between whatever parts and filters participate in a given request.

If we add these things to this event handler approach, then I agree, the cache control interface is of course unnecessary. But I think that would not be a very good design, because it would become tricky to determine which events should be used for which cache control operations, and also if the same "context" object is passed around through all event handlers it becomes trickier to implement things like collaboratively agreeing on the max duration allowed by all parts involved in the rendering...

Thoughts?

sebastienros commented 9 years ago

All your points make even more sense for me with event handlers, as many parts can affect the process. As long as the context classes support these extensibility points. One handler could invalidate another's changes, or append something in the cache key, add more time to a gracetime, ...

Also, when you use an example with "all event handlers", I am afraid you are exaggerating the number of handlers one would need to manage cache settings. And if you really need that many handlers, it means your system is as complex as the number of handlers you need to implement.

DaRosenberg commented 9 years ago

I am realizing neither a provider nor an event handler are ideal.

We want the different components that participate in processing a request and rendering the output (e.g. filters, controllers, parts, fields) to collaboratively control the caching of the request.

Each such component should only contribute cache control when it actually executes. The problem with event handlers and providers is that all handlers/providers will execute all the time. We need a way for a component (e.g. a part driver) to contribute cache control only when it executes, but still not have a dependency on output cache.

How to model that?

sfmskywalker commented 9 years ago

Sounds similar to the way the IPageBuilder works. It exposes methods to contribute to the final page title. At some point this information is taken and used to render the title.

paynecrl97 commented 9 years ago

I envisioned these events to be separate to the cache control (which is why I chose to not to expose a context on the events that would allow a module to disable caching etc...).

I'm probably being selfish here, but I need the Caching(string cacheKey, CacheItem cacheItem); method (or something similar) to be able to have my Glimpse module play nice with Output Cached pages. This is one example where a module needs to alter what is being cached (by stripping out the Glimpse HUD markup).

I can't think of any useful use cases for the other three methods on this interface, but that doesn't mean that there aren't any.

In the case of this issue, the question is- do we want the Cache Control to be able to alter the CacheItem object directly? I'd say no- each Cache Control provider (or however else it is implemented) should be able to provide input as to how it thinks the item should be cached, but it should not be able to alter the CacheItem object directly.

For example, you may have three Cache Control providers that each think that page should be cached for a different period of time. These are only suggestions, and the actual cache period should be determined by taking the smallest of these three values.

And that is the difference between these events and the Cache Control providers: the event handlers have direct access to the CacheItem object, to do what they please- whether that be logging, minification, or even removing markup (my use case).

DaRosenberg commented 9 years ago

Makes sense to me to separate the purposes as @paynecrl97 is suggesting:

The question still remains for cache control on how to implement providers in such a way that they only vote if their corresponding component (filter/controller/part/field/whatever) is actually participates in the rendering of the output, but that then become a matter for the cache control discussion, outside the scope of this issue.

HermesSbicego-Laser commented 8 years ago

Any news on this feature?

I have a scenario where in one of my PartDriver I would like to partecipate in generating the cache-key; in particular my part executes these steps:

  1. Reads AnonymousUser particular informations (IP for example, header variables, cookies or something else)
  2. Verifies correctness of infos against DB informations
  3. if correct it serves a default view: here i would like to add "ok" to my cache-key
  4. else it serves another view: here i would like to add "ko" to my cache-key

With the current outputcache logics this cannot be applied (am I right?)... I would love something more robust than VaryByHeader settings

So, in Orchard 1.8.2 I made some changes to my fork in order to achieve the above result; here is my commit (never pull requested because was in Orchard 1.8.x and 1.9 made many changes on same file) [https://github.com/OrchardCMS/Orchard/commit/2a6b6287ad4af96660a77f7f42e23b5d55f577cd]

Now I would love to have this feature on 1.10.x; any chance to create a pullrequest after having re-applied the change in branch 1.10.x of Orchard.Outputcache module? is that compliant with your Cache Events proposition? if yes i will try a PR on this in a few... Thanks