umco / umbraco-ditto

Ditto - the friendly view-model mapper for Umbraco
http://our.umbraco.org/projects/developer-tools/ditto
MIT License
79 stars 33 forks source link

Adding caching options to processors #150

Closed mattbrailsford closed 8 years ago

mattbrailsford commented 8 years ago

Open for discussion really but thought it could be handy to allow processors to cache for a given period.

Processor attributes now all have a CacheDuration property (seconds) and a CacheBy property which can be a mixture of any of

(defaults to DittoProcessorCacheBy.ContentId | DittoProcessorCacheBy.PropertyName | DittoProcessorCacheBy.Culture)

In addition, you can also set a CacheKeyBuilderType on the attribute to specify a custo cache key builder which must implement DittoProcessorCacheKeyBuilder which has a single BuildCacheKey(DittoProcessorAttribute attribute) method (good for when using custom contexts for things like paging etc).

This would result in being able to do

[UmbracoProperty(CacheDuration = 30)]
public MyType MyProp { get; set; }

[UmbracoProperty(CacheDuration = 30, CacheBy = DittoProcessorCacheBy.ContentId | DittoProcessorCacheBy.PropertyName)]
public MyType MyProp { get; set; }

[UmbracoProperty(CacheDuration = 30, CacheKeyBuilderType = typeof(MyBuilder)]
public MyType MyProp { get; set; }

Thoughts?

mattbrailsford commented 8 years ago

Could maybe rename DittoProcessorCacheBy to just DittoCacheBy

mattbrailsford commented 8 years ago

Do we need something at class level? Do we need an ability to cache an entire processor chain? (this is kinda possible via a MultiProcessor)

jamiepollock commented 8 years ago

@mattbrailsford I'd probably keep with the longer name. I initially thought the shorter name would be fine but what if there are other caching options down the line you guys haven't thought of yet?

Either way I'm sure there is no harm with either name.

mattbrailsford commented 8 years ago

@jamiepollock yea, it's just a bit ugly when chaining a few, but I guess you could create some constants if you had a regular set you used quite a bit

mattbrailsford commented 8 years ago

I was wondering a little then if there would be an issue with things like nested content where the content item doesn't have an ID, so if you cached those using ContentID, you could hit some unexpected behavior, but then I thought, in reality, NC would be retrieved as a recursive .As and you shouldn't really be caching per item in the list, you'd really just cache the containing property and thus the whole list as one so if anyone did do that, you'd just say it was bad implementation.

leekelleher commented 8 years ago

The implementation looks good to me! All good to merge into "develop" branch.

I think DittoCacheBy is nicer than DittoProcessorCacheBy - but not too concerned either way.

mattbrailsford commented 8 years ago

Ok, it probably looks a bit drastic with the file changes now, but I've incorporated a [DittoCache] attribute which allows you to cache a property / class as a whole rather than having to cache each individual processor. I've still kept the ability to cache processors, as I think this is still handy for fine grained control, but these just allow you to say "just cache the whole processor chain".

I also did some tidying and refactoring to make this all fit in nicer.