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

Support HttpApplication vary-by #326

Merged
merged 2 commits into from
Jun 2, 2023
Merged

Support HttpApplication vary-by #326

merged 2 commits into from
Jun 2, 2023

Conversation

twsouthwick
Copy link
Member

@twsouthwick twsouthwick commented Apr 17, 2023

This change adds HttpApplication.GetVaryByCustomString(context, string custom). This API allowed for custom vary by strings given a custom key.

In order to connect that into ASP.NET Core 7+, we can use output caching. To enable this, a selector is required to allow the end user to identify what keys are to be used. For example, on ASP.NET Framework, attributes were used, so on ASP.NET Core a user can grab those attributes from the endpoint metadata collection and collect the keys that way.

For example, they can use the built-in browser custom string:

builder.Services.AddOutputCache(options =>
{
    options.AddHttpApplicationBasePolicy(_ => new[] { "browser" });
});

Fixes #316

This was used for caching purposes. This change adds the API to
HttpApplication, as well as provides a helper method to hook it up to
the output caching introduced in ASP.NET Core 7 (which also adds that as
a TFM for the library).
@twsouthwick twsouthwick requested a review from Tratcher April 17, 2023 19:23
@twsouthwick
Copy link
Member Author

@Tratcher This is an API Sharepoint is using (see linked issue) and this looks to be the best way to hook it up. However, I hit an issue with the OutputCacheOptions APIs:

  • There is no way to add a list of "vary-by" key/value pairs; only single items is supported
  • There is no way to add a IOutputCachePolicy to a OutputCachePolicyBuilder - only by Type is supported. The API to add an instance method is internal for some reason

I worked around this by providing an API to add to the base policy or create a named policy. If there's a way to support the above that would probably make this a bit clearner.

@Tratcher Tratcher requested a review from sebastienros April 17, 2023 20:09
@twsouthwick
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@twsouthwick
Copy link
Member Author

@sebastienros The output caching related code is here. Any thoughts on better ways to connect this up would be appreciated.

@sebastienros
Copy link
Member

Ack

@sebastienros
Copy link
Member

LGTM

There is a VaryByValue() on the OutputCachePolicyBuilder in case that helps. You can invoke it as many times as you need. I assume it could take a list too (needs an issue to go through api-review).

The fact that you can't pass an instance is probably due to good reasons after talking about it in api-review because I am pretty sure it was possible until some point. Maybe it didn't seem useful to have this.

@twsouthwick
Copy link
Member Author

Thanks @sebastienros. Looking at the history of the reviews, it looks like the overload to pass an instance to the builder was due to worry about recursion and not knowing if there was a use case.

There is a VaryByValue() on the OutputCachePolicyBuilder in case that helps. You can invoke it as many times as you need. I assume it could take a list too (needs an issue to go through api-review).

I've added an overload that now that uses that as well. I don't have a feel for what is the better way to set this up - hard coded keys up front, or querying the endpoint for metadata and extracting keys from the current endpoint. Being able to add a policy enables the second point (which is why I went that route initially), but maybe it's sufficient to define a policy with a hard coded list up front.

@joperezr joperezr added this to the 1.2 milestone May 10, 2023
@twsouthwick
Copy link
Member Author

@Tratcher any additional thoughts here?

@twsouthwick twsouthwick merged commit ca4efe1 into main Jun 2, 2023
@twsouthwick twsouthwick deleted the tasou/httpapp-varyby branch June 2, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue of HttpApplication.GetVaryByCustomString
4 participants