Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TopicController response caching #89

Closed
JeremyCaney opened this issue Apr 13, 2021 · 7 comments
Closed

TopicController response caching #89

JeremyCaney opened this issue Apr 13, 2021 · 7 comments
Assignees
Labels
Area: Web Relates to the `AspNetCore` or other web-related functionality. Priority: 1 Status 5: Complete Task is considered complete, and ready for deployment. Type: Feature Introduces a major area of functionality.
Milestone

Comments

@JeremyCaney
Copy link
Member

JeremyCaney commented Apr 13, 2021

Update TopicController to conditionally set content headers that will be recognized by e.g. ResponseCaching middleware. This way, we can dynamically configure particular pages to participate in response caching based on a topic attribute (e.g., IsCached).

To accommodate this, we'll need to determine whether this will operate off of a fixed profile, centralized configuration values, or per-page values.

The best bet might be to rely on cache profiles, but to allow those to be selected via a MetadataList attribute. This would permit settings to be configured per site and to vary per page, while still allowing those settings to be centralized.

@JeremyCaney JeremyCaney added Area: Web Relates to the `AspNetCore` or other web-related functionality. Priority: 1 Type: Feature Introduces a major area of functionality. Status 2: Scheduled Planned for an upcoming release. labels Apr 13, 2021
@JeremyCaney JeremyCaney added this to the OnTopic 5.2.0 milestone Apr 13, 2021
@JeremyCaney JeremyCaney self-assigned this Apr 13, 2021
@JeremyCaney
Copy link
Member Author

JeremyCaney commented Dec 5, 2021

Does this proposed MetadataList need to contain specialized attributes for managing the cache (e.g., Expiry, IsServerSide, &c.)? Or would it simply map to cache profiles configured in ASP.NET Core itself? Need to determine how cache profiles can be configured in ASP.NET Core; e.g., can they be dynamic?

We could have an extension method for default defining cache profiles, while allowing them to be added by the customer for more specific criteria. But this might complicate e.g., making minor updates to cache profile defaults.

Regardless, can a cache profile be set programmatically? It doesn’t seem like it. If not, we’d need to store the cache profile values in OnTopic and set them via the TopicController, likely by setting the response headers. Positively, this makes the defaults easier to customize per site.

@JeremyCaney
Copy link
Member Author

The [ResponseCache()] attribute source code demonstrates how to programmatically retrieve actual ASP.NET Cache Profiles. But I think it might make more sense to store these in OnTopic.

@JeremyCaney
Copy link
Member Author

JeremyCaney commented Dec 9, 2021

Ideally, we should establish a structure similar to:

/Configuration 
  /Settings (Settings)
    /CacheProfiles (NestedList)
      /{Name} (CacheProfile)
        - Duration (NumberAttribute)
        - Location (LookupListAttribute)
        - NoStore (BooleanAttribute)
        - VaryByHeader (TextAttribute)
        - VaryByQueryKeys (TextAttribute)

@JeremyCaney
Copy link
Member Author

From the Page topic perspective, this should be configured as a TopicReference named CacheProfile pointing to e.g.

Configuration:settings:CacheProfiles

Then, on e.g. the PageTopicViewModel, we should add something like:

public CacheProfileViewModel CacheProfile { get; set; }

Where the CacheProfileViewModel is something like:

public record CacheProfileViewModel {
  public int Duration { get; set; }
  public string Location { get; set; }
  public bool NoStore { get; set; }
  public string VaryByHeader { get; set; }
  public string VaryByQueryKeys { get; set; }
}

This way, if a cache profile is configured, then it will be automatically mapped as a reference. And, if it isn’t, the application can determine what defaults (if any) it wishes to use in the meta headers.

@JeremyCaney
Copy link
Member Author

JeremyCaney commented Dec 14, 2021

Instead of relying on each and every implementation to optionally expose the cache profile via e.g. the _Layout.cshtml, consider instead adding support for a CacheProfileFilter that can be registered globally, and which will modify the response headers accordingly. This would provide a more reusable solution. It would also be similar to the out-of-the-box ResponseCacheFilter (and related ResponseCacheFilterExecutor).

Note: In fact, if the ResponseCacheFilterExecutor was public, we could probably rely on it for our own filter. Unfortunately, it's marked internal, so we may need to reproduce it.

A challenge with this is that it would live in the OnTopic.AspNetCore.Mvc project, which doesn't depend on OnTopic.ViewModels. And we don't want OnTopic.ViewModels to rely on OnTopic.AspNetCore.Mvc. That makes it difficult for them to share a CacheProfile object. There are two solutions to this:

  1. Place the CacheProfile class in the OnTopic project (even though it's web specific),
  2. Place an ICacheProfile interface in the OnTopic project, and setup the filter with a generic type argument so it can be configured.

Another challenge with this is that it will need access to the current topic. One approach to this is to make it dependent on the TopicController base, similar to what we've done with the ValidateTopicAttribute. This isn't ideal in that it limits the scope that we can use it. But the alternative might necessitate us using e.g. ASP.NET Core's dependency injection container to allow the TopicRepository to be made available to the filter, which also isn't ideal.

Of course, if this filter solves the vast majority of cases, the exceptions can implement this via their e.g. _Layout.cshtml, just as they'd otherwise have to do without it. As such, it doesn't need to address corner cases.

@JeremyCaney
Copy link
Member Author

JeremyCaney commented Dec 15, 2021

“Filters can be added by type or by instance. If an instance is added, that instance is used for every request. If a type is added, it's type-activated. A type-activated filter means: An instance is created for each request.”

I.e., we can inject a global instance with hard-coded dependencies, such as an optional default CacheProfile. E.g.,

options.Filters.Add(new ResponseCacheFilter("Default"));

@JeremyCaney JeremyCaney added Status 3: In Progress Currently being worked on. and removed Status 2: Scheduled Planned for an upcoming release. labels Dec 16, 2021
JeremyCaney added a commit that referenced this issue Dec 17, 2021
The `[TopicResponseCache]` attribute operates off of the `TopicController` (or derivative) and will identify any `CacheProfile` topics associated with the `CurrentTopic` (defaulting to the `Default` cache profile) and use its attributes to set the HTTP response headers for the current request. This is compatible with the ASP.NET Core Response Caching Middleware.

This satisfies the core of Issue #89 .
JeremyCaney added a commit that referenced this issue Dec 17, 2021
…ibute

This tests the new `/Web/CachedPage/` stub data (8cbf7da) to ensure that the `Counter.cshtml` (ea40bbb) doesn't increment after two subsequent calls, and that the cache headers are correctly set. It also checks a `/Web/UncachedPage/` stub data (8cbf7da) to ensure that the `Counter.cshtml` (ea40bbb) _is_ correctly incremented, as a control case.

The completes the primary requirements for #89.
JeremyCaney added a commit that referenced this issue Dec 17, 2021
Introduced the new `[TopicResponseCache]` attribute, which operates similar to the out-of-the-box `[ResponseCache]` attribute, except that it loads the `CacheProfile` from the `CurrentTopic` instead of from the attribute or ASP.NET Core configuration. This allows cache profiles to be centrally configured and managed via OnTopic, and associated with any page. Further, as this is applied to `TopicController` and all descendants, and works directly with the response headers, it is automatically applied without the need for implementers to e.g. customize their models or views.

This satisfies the requirements for #89.
@JeremyCaney
Copy link
Member Author

This was implemented via a new [TopicResponseCache] attribute (9bc5797), which operates similar to the out-of-the-box [ResponseCache] attribute, except that it loads the CacheProfile from the CurrentTopic instead of from the attribute or ASP.NET Core configuration. This allows cache profiles to be centrally configured and managed via OnTopic, and associated with any page. Further, as this is applied to TopicController and all descendants (1a5994d), and works directly with the response headers, it is automatically applied without the need for implementers to e.g. customize their models or views. The final commit (a30b85f) includes integration tests to confirm not only response caching, but also support for output caching via the ASP.NET Core Response Caching Middleware (08db830).

@JeremyCaney JeremyCaney added Status 5: Complete Task is considered complete, and ready for deployment. and removed Status 3: In Progress Currently being worked on. labels Dec 17, 2021
JeremyCaney added a commit that referenced this issue Jun 26, 2022
OnTopic 5.2.0 introduces support for response caching (#89), a new `ErrorController` for HTTP error codes (#91), and improvements to `ITopicMappingService` performance via optional `AttributeDictionary` constructors (#99), updates to the internal reflection libraries (#90), and caching of compatible properties (#102). In addition, it also includes a number of internal code improvements, such as adoption of new C# 10 capabilities (#96), including global using directives (#93).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Web Relates to the `AspNetCore` or other web-related functionality. Priority: 1 Status 5: Complete Task is considered complete, and ready for deployment. Type: Feature Introduces a major area of functionality.
Projects
None yet
Development

No branches or pull requests

1 participant