Skip to content
This repository has been archived by the owner on Dec 13, 2021. It is now read-only.

Adding caching options to processors #150

Merged
merged 6 commits into from
Jan 8, 2016

Conversation

mattbrailsford
Copy link
Collaborator

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

  • DittoProcessorCacheBy.ContentId
  • DittoProcessorCacheBy.PropertyName
  • DittoProcessorCacheBy.TargetType
  • DittoProcessorCacheBy.Culture

(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
Copy link
Collaborator Author

Could maybe rename DittoProcessorCacheBy to just DittoCacheBy

@mattbrailsford
Copy link
Collaborator Author

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
Copy link

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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
Copy link
Collaborator

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.

@leekelleher leekelleher added this to the 0.9.0 milestone Jan 8, 2016
@mattbrailsford
Copy link
Collaborator Author

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.

mattbrailsford added a commit that referenced this pull request Jan 8, 2016
@mattbrailsford mattbrailsford merged commit a3458c7 into develop Jan 8, 2016
@mattbrailsford mattbrailsford deleted the feature/processor-caching branch January 8, 2016 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion feature rfc Request for Comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants