-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Output caching middleware #41037
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
Output caching middleware #41037
Conversation
2897106
to
719df0e
Compare
src/Middleware/OutputCaching.Abstractions/src/IPoliciesMetadata.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching.Abstractions/src/OutputCacheEntry.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Memory/MemoryCachedResponse.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/OutputCachingPolicyProvider.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/OutputCachingPolicyProvider.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching.Abstractions/src/IOutputCachingContext.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Memory/MemoryCachedResponse.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching.Abstractions/src/CachedResponseBody.cs
Outdated
Show resolved
Hide resolved
Thank you for your API proposal. I'm removing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial API review notes
src/Middleware/OutputCaching/samples/OutputCachingSample/Startup.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/samples/OutputCachingSample/Startup.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/samples/OutputCachingSample/Startup.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Nullable is enabled by default
- Enable trimming on new projects. Also, need to run a command line to update projects file passed to validator. See https://github.com/dotnet/aspnetcore/blob/main/docs/Trimming.md
...leware/OutputCaching.Abstractions/src/Microsoft.AspNetCore.OutputCaching.Abstractions.csproj
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Microsoft.AspNetCore.OutputCaching.csproj
Outdated
Show resolved
Hide resolved
Alternatively, you can leave trimming to a follow up PR, just create an issue for it so it doesn't get forgotten. |
Co-authored-by: Kahbazi <A.Kahbazi@gmail.com>
Co-authored-by: Kahbazi <A.Kahbazi@gmail.com>
Co-authored-by: Kahbazi <A.Kahbazi@gmail.com>
Co-authored-by: Kahbazi <A.Kahbazi@gmail.com>
Co-authored-by: Kahbazi <A.Kahbazi@gmail.com>
# Conflicts: # src/submodules/googletest
@campersau thanks a lot, replied when I didn't follow your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also back out the src/submodules/googletest update
@@ -209,6 +210,7 @@ static TestData() | |||
{ "Microsoft.AspNetCore.Mvc.RazorPages", "7.0.0.0" }, | |||
{ "Microsoft.AspNetCore.Mvc.TagHelpers", "7.0.0.0" }, | |||
{ "Microsoft.AspNetCore.Mvc.ViewFeatures", "7.0.0.0" }, | |||
{ "Microsoft.AspNetCore.OutputCaching", "7.0.0.0" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's our approval process these days for extending the shared framework❔ Was that process followed here❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing apart from api review where it was discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether that's sufficient. @adityamandaleeka❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed w/ Aditya in triage that we want this in the SharedFx
src/Middleware/OutputCaching/samples/OutputCachingSample/OutputCachingSample.csproj
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/samples/OutputCachingSample/OutputCachingSample.csproj
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Microsoft.AspNetCore.OutputCaching.csproj
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Microsoft.AspNetCore.OutputCaching.csproj
Outdated
Show resolved
Hide resolved
@@ -48,6 +48,7 @@ Microsoft.AspNetCore.Mvc.RouteAttribute</Description> | |||
<Reference Include="Microsoft.AspNetCore.Http" /> | |||
<Reference Include="Microsoft.AspNetCore.Http.Extensions" /> | |||
<Reference Include="Microsoft.AspNetCore.ResponseCaching.Abstractions" /> | |||
<Reference Include="Microsoft.AspNetCore.OutputCaching" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought we normally only referenced {blah}.Abstractions
from Mvc.Core for middlewares. Why doesn't Microsoft.AspNetCore.OutputCaching.Abstractions exist❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstractions were removed (after api review) since it ships in Framework and an abstraction is not on nuget.
src/Middleware/OutputCaching/test/OutputCachePolicyProviderTests.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Memory/MemoryOutputCacheStore.cs
Outdated
Show resolved
Hide resolved
I assume the submodule change was unintended? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure all API changes are correct and any modifications are put in the issue for proper API review for the next meeting.
/// <summary> | ||
/// Represents vary-by rules. | ||
/// </summary> | ||
public sealed class CachedVaryByRules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few differences from the API seen in API review. For example, this class was renamed to CacheVaryByRules
and three of the properties were { get; init; }
src/Middleware/OutputCaching/src/Memory/MemoryOutputCacheStore.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Memory/MemoryOutputCacheStore.cs
Outdated
Show resolved
Hide resolved
src/Middleware/OutputCaching/src/Policies/OutputCacheConventionBuilderExtensions.cs
Show resolved
Hide resolved
CI was green on this PR: https://dev.azure.com/dnceng/public/_build/results?buildId=1840119&view=logs&j=3f6d4e0f-1b71-56b5-361e-d95b6e6da15a. Just merged an update to un-update the googletest submodule, which won't affect CI. Force-merging this now to get it in for preview6. |
@sebastienros, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
Hi @jerhon. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
The goal of this PR is to gather comments on the APIs or feature gaps while I start working on unit tests in case there would be some significant changes.
This PR doesn't contain a custom store like disk or redis, this will be added as a separate one after this is merged.
Most of the low-level code is taken from response caching. I expect comments that could also apply to response caching, but we might decide to still do the changes only here.
Closes #40228 #40227 #40226 #40224 #40223 #40222 #40221