Add support for ResponseCache in Razor Pages #6627
Conversation
|
|
||
| namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal | ||
| { | ||
| public class ResponseCacheFilterApplicationModelProvider : IPageApplicationModelProvider |
There was a problem hiding this comment.
No tests for this because I wasn't sure if there's a specific reason controllers use the filter factory route to add the actual filter.
There was a problem hiding this comment.
I think that's fine as long as there's a functional test (which there is)
| @@ -41,17 +30,17 @@ public ResponseCacheFilter(CacheProfile cacheProfile) | |||
| /// </summary> | |||
| public int Duration | |||
There was a problem hiding this comment.
Why do we have these properties?
There was a problem hiding this comment.
Because you can still construct the filter
| /// </summary> | ||
| public interface IResponseCacheFilter : IActionFilter |
There was a problem hiding this comment.
Use a different marker type for pages? I thought we were kinda ok changing this since it's in .Internal
There was a problem hiding this comment.
Oh! it is, cool. Ok that's all fine then
| _cacheProfile = cacheProfile ?? throw new ArgumentNullException(nameof(cacheProfile)); | ||
| } | ||
|
|
||
| public int Duration |
There was a problem hiding this comment.
Rather than duplicating all of this, it would be better pass in the ResponseCacheFilter object directly - similar to how our action result executors work.
There was a problem hiding this comment.
We have two different types of ResponseCacheFilter though. I didn't want to go down the inheritance route because that seemed gross.
| // Assert | ||
| Assert.Collection( | ||
| context.PageApplicationModel.Filters, | ||
| f => { }, |
There was a problem hiding this comment.
This is kinda annoying. ResponseCacheAttribute is a IFilterFactory and ends up adding ResponseCacheFilter : IActionFilter to the model. It ultimately does nothing, but it would be nice if we could avoid having it in the list at all.
|
Added test 🆙 📅 |
Fixes #6437