From 6a98c6862880060468a9534392dc39ce96615e74 Mon Sep 17 00:00:00 2001 From: Bart Wolff Date: Tue, 8 Jan 2019 23:49:38 +0100 Subject: [PATCH] Code cleanup response caching middleware (#6124) * minor code cleanups * comment public interface * simplified some things using language features --- .../Internal/CacheEntry/CacheEntryHelpers .cs | 3 +- .../ResponseCaching/src/Internal/FastGuid.cs | 3 +- .../src/Internal/Interfaces/IResponseCache.cs | 27 +++++++++ .../src/Internal/LoggerExtensions.cs | 58 +++++++++---------- .../src/Internal/MemoryResponseCache.cs | 15 ++--- .../Internal/ResponseCachingKeyProvider.cs | 4 +- .../Internal/ResponseCachingPolicyProvider.cs | 1 - .../src/Internal/SendFileFeatureWrapper.cs | 6 +- .../src/Internal/StringBuilderExtensions.cs | 2 +- .../src/Internal/SystemClock.cs | 8 +-- .../src/ResponseCachingExtensions.cs | 1 - .../src/ResponseCachingMiddleware.cs | 6 +- .../src/Streams/ResponseCachingStream.cs | 6 +- .../src/Streams/SegmentReadStream.cs | 7 +-- 14 files changed, 76 insertions(+), 71 deletions(-) diff --git a/src/Middleware/ResponseCaching/src/Internal/CacheEntry/CacheEntryHelpers .cs b/src/Middleware/ResponseCaching/src/Internal/CacheEntry/CacheEntryHelpers .cs index f23286a77e..919e52b3d7 100644 --- a/src/Middleware/ResponseCaching/src/Internal/CacheEntry/CacheEntryHelpers .cs +++ b/src/Middleware/ResponseCaching/src/Internal/CacheEntry/CacheEntryHelpers .cs @@ -7,7 +7,6 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal { internal static class CacheEntryHelpers { - internal static long EstimateCachedResponseSize(CachedResponse cachedResponse) { if (cachedResponse == null) @@ -25,7 +24,7 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal { foreach (var item in cachedResponse.Headers) { - size += item.Key.Length * sizeof(char) + EstimateStringValuesSize(item.Value); + size += (item.Key.Length * sizeof(char)) + EstimateStringValuesSize(item.Value); } } diff --git a/src/Middleware/ResponseCaching/src/Internal/FastGuid.cs b/src/Middleware/ResponseCaching/src/Internal/FastGuid.cs index 76cac184ae..b62ba22f83 100644 --- a/src/Middleware/ResponseCaching/src/Internal/FastGuid.cs +++ b/src/Middleware/ResponseCaching/src/Internal/FastGuid.cs @@ -15,7 +15,8 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal // Instance components private string _idString; - internal long IdValue { get; private set; } + + internal long IdValue { get; } internal string IdString { diff --git a/src/Middleware/ResponseCaching/src/Internal/Interfaces/IResponseCache.cs b/src/Middleware/ResponseCaching/src/Internal/Interfaces/IResponseCache.cs index 41c85b277a..f7b6ac82fd 100644 --- a/src/Middleware/ResponseCaching/src/Internal/Interfaces/IResponseCache.cs +++ b/src/Middleware/ResponseCaching/src/Internal/Interfaces/IResponseCache.cs @@ -8,10 +8,37 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal { public interface IResponseCache { + /// + /// Gets the cached response for the given key, if it exists. + /// If no cached response exists for the given key, null is returned. + /// + /// The cache key to look up. + /// The response cache entry if it exists; otherwise null. IResponseCacheEntry Get(string key); + + /// + /// Gets the cached response for the given key, if it exists. + /// If no cached response exists for the given key, null is returned. + /// + /// The cache key to look up. + /// The response cache entry if it exists; otherwise null. Task GetAsync(string key); + /// + /// Stores the given response in the response cache. + /// + /// The cache key to store the response under. + /// The response cache entry to store. + /// The amount of time the entry will be kept in the cache before expiring, relative to now. void Set(string key, IResponseCacheEntry entry, TimeSpan validFor); + + /// + /// Stores the given response in the response cache. + /// + /// The cache key to store the response under. + /// The response cache entry to store. + /// The amount of time the entry will be kept in the cache before expiring, relative to now. + /// No result is returned. Task SetAsync(string key, IResponseCacheEntry entry, TimeSpan validFor); } } diff --git a/src/Middleware/ResponseCaching/src/Internal/LoggerExtensions.cs b/src/Middleware/ResponseCaching/src/Internal/LoggerExtensions.cs index f8a0bf3151..5039925e93 100644 --- a/src/Middleware/ResponseCaching/src/Internal/LoggerExtensions.cs +++ b/src/Middleware/ResponseCaching/src/Internal/LoggerExtensions.cs @@ -12,35 +12,35 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal /// internal static class LoggerExtensions { - private static Action _logRequestMethodNotCacheable; - private static Action _logRequestWithAuthorizationNotCacheable; - private static Action _logRequestWithNoCacheNotCacheable; - private static Action _logRequestWithPragmaNoCacheNotCacheable; - private static Action _logExpirationMinFreshAdded; - private static Action _logExpirationSharedMaxAgeExceeded; - private static Action _logExpirationMustRevalidate; - private static Action _logExpirationMaxStaleSatisfied; - private static Action _logExpirationMaxAgeExceeded; - private static Action _logExpirationExpiresExceeded; - private static Action _logResponseWithoutPublicNotCacheable; - private static Action _logResponseWithNoStoreNotCacheable; - private static Action _logResponseWithNoCacheNotCacheable; - private static Action _logResponseWithSetCookieNotCacheable; - private static Action _logResponseWithVaryStarNotCacheable; - private static Action _logResponseWithPrivateNotCacheable; - private static Action _logResponseWithUnsuccessfulStatusCodeNotCacheable; - private static Action _logNotModifiedIfNoneMatchStar; - private static Action _logNotModifiedIfNoneMatchMatched; - private static Action _logNotModifiedIfModifiedSinceSatisfied; - private static Action _logNotModifiedServed; - private static Action _logCachedResponseServed; - private static Action _logGatewayTimeoutServed; - private static Action _logNoResponseServed; - private static Action _logVaryByRulesUpdated; - private static Action _logResponseCached; - private static Action _logResponseNotCached; - private static Action _logResponseContentLengthMismatchNotCached; - private static Action _logExpirationInfiniteMaxStaleSatisfied; + private static readonly Action _logRequestMethodNotCacheable; + private static readonly Action _logRequestWithAuthorizationNotCacheable; + private static readonly Action _logRequestWithNoCacheNotCacheable; + private static readonly Action _logRequestWithPragmaNoCacheNotCacheable; + private static readonly Action _logExpirationMinFreshAdded; + private static readonly Action _logExpirationSharedMaxAgeExceeded; + private static readonly Action _logExpirationMustRevalidate; + private static readonly Action _logExpirationMaxStaleSatisfied; + private static readonly Action _logExpirationMaxAgeExceeded; + private static readonly Action _logExpirationExpiresExceeded; + private static readonly Action _logResponseWithoutPublicNotCacheable; + private static readonly Action _logResponseWithNoStoreNotCacheable; + private static readonly Action _logResponseWithNoCacheNotCacheable; + private static readonly Action _logResponseWithSetCookieNotCacheable; + private static readonly Action _logResponseWithVaryStarNotCacheable; + private static readonly Action _logResponseWithPrivateNotCacheable; + private static readonly Action _logResponseWithUnsuccessfulStatusCodeNotCacheable; + private static readonly Action _logNotModifiedIfNoneMatchStar; + private static readonly Action _logNotModifiedIfNoneMatchMatched; + private static readonly Action _logNotModifiedIfModifiedSinceSatisfied; + private static readonly Action _logNotModifiedServed; + private static readonly Action _logCachedResponseServed; + private static readonly Action _logGatewayTimeoutServed; + private static readonly Action _logNoResponseServed; + private static readonly Action _logVaryByRulesUpdated; + private static readonly Action _logResponseCached; + private static readonly Action _logResponseNotCached; + private static readonly Action _logResponseContentLengthMismatchNotCached; + private static readonly Action _logExpirationInfiniteMaxStaleSatisfied; static LoggerExtensions() { diff --git a/src/Middleware/ResponseCaching/src/Internal/MemoryResponseCache.cs b/src/Middleware/ResponseCaching/src/Internal/MemoryResponseCache.cs index d69d48ebbd..0d3235cf5d 100644 --- a/src/Middleware/ResponseCaching/src/Internal/MemoryResponseCache.cs +++ b/src/Middleware/ResponseCaching/src/Internal/MemoryResponseCache.cs @@ -13,20 +13,14 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal public MemoryResponseCache(IMemoryCache cache) { - if (cache == null) - { - throw new ArgumentNullException(nameof(cache)); - } - - _cache = cache; + _cache = cache ?? throw new ArgumentNullException(nameof(cache)); } public IResponseCacheEntry Get(string key) { var entry = _cache.Get(key); - var memoryCachedResponse = entry as MemoryCachedResponse; - if (memoryCachedResponse != null) + if (entry is MemoryCachedResponse memoryCachedResponse) { return new CachedResponse { @@ -49,8 +43,7 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal public void Set(string key, IResponseCacheEntry entry, TimeSpan validFor) { - var cachedResponse = entry as CachedResponse; - if (cachedResponse != null) + if (entry is CachedResponse cachedResponse) { var segmentStream = new SegmentWriteStream(StreamUtilities.BodySegmentSize); cachedResponse.Body.CopyTo(segmentStream); @@ -90,4 +83,4 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal return Task.CompletedTask; } } -} \ No newline at end of file +} diff --git a/src/Middleware/ResponseCaching/src/Internal/ResponseCachingKeyProvider.cs b/src/Middleware/ResponseCaching/src/Internal/ResponseCachingKeyProvider.cs index d69d9008eb..78f329600f 100644 --- a/src/Middleware/ResponseCaching/src/Internal/ResponseCachingKeyProvider.cs +++ b/src/Middleware/ResponseCaching/src/Internal/ResponseCachingKeyProvider.cs @@ -96,7 +96,7 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal throw new InvalidOperationException($"{nameof(CachedVaryByRules)} must not be null on the {nameof(ResponseCachingContext)}"); } - if ((StringValues.IsNullOrEmpty(varyByRules.Headers) && StringValues.IsNullOrEmpty(varyByRules.QueryKeys))) + if (StringValues.IsNullOrEmpty(varyByRules.Headers) && StringValues.IsNullOrEmpty(varyByRules.QueryKeys)) { return varyByRules.VaryByKeyPrefix; } @@ -204,7 +204,7 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal private class QueryKeyComparer : IComparer> { - private StringComparer _stringComparer; + private readonly StringComparer _stringComparer; public static QueryKeyComparer OrdinalIgnoreCase { get; } = new QueryKeyComparer(StringComparer.OrdinalIgnoreCase); diff --git a/src/Middleware/ResponseCaching/src/Internal/ResponseCachingPolicyProvider.cs b/src/Middleware/ResponseCaching/src/Internal/ResponseCachingPolicyProvider.cs index 8ffc59612e..e6f46c9e0f 100644 --- a/src/Middleware/ResponseCaching/src/Internal/ResponseCachingPolicyProvider.cs +++ b/src/Middleware/ResponseCaching/src/Internal/ResponseCachingPolicyProvider.cs @@ -47,7 +47,6 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal else { // Support for legacy HTTP 1.0 cache directive - var pragmaHeaderValues = request.Headers[HeaderNames.Pragma]; if (HeaderUtilities.ContainsCacheDirective(request.Headers[HeaderNames.Pragma], CacheControlHeaderValue.NoCacheString)) { context.Logger.LogRequestWithPragmaNoCacheNotCacheable(); diff --git a/src/Middleware/ResponseCaching/src/Internal/SendFileFeatureWrapper.cs b/src/Middleware/ResponseCaching/src/Internal/SendFileFeatureWrapper.cs index 2716e4cd37..65ac5850f8 100644 --- a/src/Middleware/ResponseCaching/src/Internal/SendFileFeatureWrapper.cs +++ b/src/Middleware/ResponseCaching/src/Internal/SendFileFeatureWrapper.cs @@ -19,10 +19,10 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal } // Flush and disable the buffer if anyone tries to call the SendFile feature. - public Task SendFileAsync(string path, long offset, long? length, CancellationToken cancellation) + public Task SendFileAsync(string path, long offset, long? count, CancellationToken cancellation) { _responseCachingStream.DisableBuffering(); - return _originalSendFileFeature.SendFileAsync(path, offset, length, cancellation); + return _originalSendFileFeature.SendFileAsync(path, offset, count, cancellation); } } -} \ No newline at end of file +} diff --git a/src/Middleware/ResponseCaching/src/Internal/StringBuilderExtensions.cs b/src/Middleware/ResponseCaching/src/Internal/StringBuilderExtensions.cs index 98cfa7e172..40169d66bc 100644 --- a/src/Middleware/ResponseCaching/src/Internal/StringBuilderExtensions.cs +++ b/src/Middleware/ResponseCaching/src/Internal/StringBuilderExtensions.cs @@ -21,4 +21,4 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal return builder; } } -} \ No newline at end of file +} diff --git a/src/Middleware/ResponseCaching/src/Internal/SystemClock.cs b/src/Middleware/ResponseCaching/src/Internal/SystemClock.cs index 39b6e4735a..e4462d803f 100644 --- a/src/Middleware/ResponseCaching/src/Internal/SystemClock.cs +++ b/src/Middleware/ResponseCaching/src/Internal/SystemClock.cs @@ -13,12 +13,6 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal /// /// Retrieves the current system time in UTC. /// - public DateTimeOffset UtcNow - { - get - { - return DateTimeOffset.UtcNow; - } - } + public DateTimeOffset UtcNow => DateTimeOffset.UtcNow; } } diff --git a/src/Middleware/ResponseCaching/src/ResponseCachingExtensions.cs b/src/Middleware/ResponseCaching/src/ResponseCachingExtensions.cs index 76b81dbccb..45d905cea6 100644 --- a/src/Middleware/ResponseCaching/src/ResponseCachingExtensions.cs +++ b/src/Middleware/ResponseCaching/src/ResponseCachingExtensions.cs @@ -3,7 +3,6 @@ using System; using Microsoft.AspNetCore.ResponseCaching; -using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Builder { diff --git a/src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs b/src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs index d2eee86ad7..208d6e998a 100644 --- a/src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs +++ b/src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs @@ -138,8 +138,7 @@ namespace Microsoft.AspNetCore.ResponseCaching internal async Task TryServeCachedResponseAsync(ResponseCachingContext context, IResponseCacheEntry cacheEntry) { - var cachedResponse = cacheEntry as CachedResponse; - if (cachedResponse == null) + if (!(cacheEntry is CachedResponse cachedResponse)) { return false; } @@ -199,8 +198,7 @@ namespace Microsoft.AspNetCore.ResponseCaching context.BaseKey = _keyProvider.CreateBaseKey(context); var cacheEntry = await _cache.GetAsync(context.BaseKey); - var cachedVaryByRules = cacheEntry as CachedVaryByRules; - if (cachedVaryByRules != null) + if (cacheEntry is CachedVaryByRules cachedVaryByRules) { // Request contains vary rules, recompute key(s) and try again context.CachedVaryByRules = cachedVaryByRules; diff --git a/src/Middleware/ResponseCaching/src/Streams/ResponseCachingStream.cs b/src/Middleware/ResponseCaching/src/Streams/ResponseCachingStream.cs index c9d476c97d..6a0c162be4 100644 --- a/src/Middleware/ResponseCaching/src/Streams/ResponseCachingStream.cs +++ b/src/Middleware/ResponseCaching/src/Streams/ResponseCachingStream.cs @@ -13,9 +13,9 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal private readonly Stream _innerStream; private readonly long _maxBufferSize; private readonly int _segmentSize; - private SegmentWriteStream _segmentWriteStream; - private Action _startResponseCallback; - private Func _startResponseCallbackAsync; + private readonly SegmentWriteStream _segmentWriteStream; + private readonly Action _startResponseCallback; + private readonly Func _startResponseCallbackAsync; internal ResponseCachingStream(Stream innerStream, long maxBufferSize, int segmentSize, Action startResponseCallback, Func startResponseCallbackAsync) { diff --git a/src/Middleware/ResponseCaching/src/Streams/SegmentReadStream.cs b/src/Middleware/ResponseCaching/src/Streams/SegmentReadStream.cs index 83c60dd0c5..8234d135a9 100644 --- a/src/Middleware/ResponseCaching/src/Streams/SegmentReadStream.cs +++ b/src/Middleware/ResponseCaching/src/Streams/SegmentReadStream.cs @@ -19,12 +19,7 @@ namespace Microsoft.AspNetCore.ResponseCaching.Internal internal SegmentReadStream(List segments, long length) { - if (segments == null) - { - throw new ArgumentNullException(nameof(segments)); - } - - _segments = segments; + _segments = segments ?? throw new ArgumentNullException(nameof(segments)); _length = length; }