-
Notifications
You must be signed in to change notification settings - Fork 1.7k
#741 Add filtering options and logic to caching #2337
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
base: develop
Are you sure you want to change the base?
Changes from 7 commits
bf2a44b
159da67
8885baa
1245208
14a63b1
e8e4515
cdcf273
849d449
e7b9981
9013937
327fec0
ad91f96
5f6a812
62d0824
78b6936
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -60,8 +60,16 @@ public async Task Invoke(HttpContext httpContext) | |||||||||
| cached = await CreateCachedResponse(downstreamResponse); | ||||||||||
|
|
||||||||||
| var ttl = TimeSpan.FromSeconds(options.TtlSeconds); | ||||||||||
| _outputCache.Add(downStreamRequestCacheKey, cached, options.Region, ttl); | ||||||||||
| Logger.LogDebug(() => $"Finished response added to cache for the '{downstreamUrlKey}' key."); | ||||||||||
| if (options.StatusCodeFilter == null || options.StatusCodeFilter.PassesFilter(cached.StatusCode)) | ||||||||||
| { | ||||||||||
| _outputCache.Add(downStreamRequestCacheKey, cached, options.Region, ttl); | ||||||||||
| Logger.LogDebug(() => $"Finished response added to cache for the '{downstreamUrlKey}' key."); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please wrap the logging for the Debug scenario with a preprocessor directive, as another team member will likely request this 👇
Suggested change
|
||||||||||
| } | ||||||||||
| else | ||||||||||
| { | ||||||||||
| Logger.LogDebug(() => $"Finished response filtered out from being added to cache due to response status for the '{downstreamUrlKey}' key."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| private static void SetHttpResponseMessageThisRequest(HttpContext context, DownstreamResponse response) | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,7 @@ namespace Ocelot.Configuration.Creator; | |||||
| public class CacheOptionsCreator : ICacheOptionsCreator | ||||||
| { | ||||||
| public CacheOptions Create(FileCacheOptions options) | ||||||
| => new(options?.TtlSeconds, options?.Region, options?.Header, options?.EnableContentHashing); | ||||||
| => new(options?.TtlSeconds, options?.Region, options?.Header, options?.EnableContentHashing, options?.StatusCodeFilter); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems I missed this issue in the previous PR. Extending the argument list in a single constructor is not the right approach. It’s better to have a few overloaded constructors that cover most user scenarios.
Suggested change
The null-coalescing operator (
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can do. I was just following the existing pattern. Not my project, so did not figure it was my place to make that change.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, never mind. I will handle it myself. |
||||||
|
|
||||||
| public CacheOptions Create(FileRoute route, FileGlobalConfiguration globalConfiguration, string loadBalancingKey) | ||||||
| { | ||||||
|
|
@@ -58,6 +58,7 @@ protected virtual CacheOptions Merge(FileCacheOptions options, FileCacheOptions | |||||
| var header = options.Header.IfEmpty(globalOptions.Header).IfEmpty(CacheOptions.Oc_Cache_Control); | ||||||
| var ttlSeconds = options.TtlSeconds ?? globalOptions.TtlSeconds; | ||||||
| var enableHashing = options.EnableContentHashing ?? globalOptions.EnableContentHashing; | ||||||
| return new CacheOptions(ttlSeconds, region, header, enableHashing); | ||||||
| var statusCodes = options.StatusCodeFilter ?? globalOptions.StatusCodeFilter; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a lack of defaulting! Refer to my Idea 2 in the previous comment. We could implement defaulting closer to the Dev Complete stage, and I'll help with that.
Suggested change
|
||||||
| return new CacheOptions(ttlSeconds, region, header, enableHashing, statusCodes); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,6 @@ | ||||||
| namespace Ocelot.Configuration.File; | ||||||
| using Ocelot.Filter; | ||||||
|
|
||||||
| namespace Ocelot.Configuration.File; | ||||||
|
|
||||||
| public class FileCacheOptions | ||||||
| { | ||||||
|
|
@@ -10,6 +12,7 @@ public FileCacheOptions(FileCacheOptions from) | |||||
| TtlSeconds = from.TtlSeconds; | ||||||
| Header = from.Header; | ||||||
| EnableContentHashing = from.EnableContentHashing; | ||||||
| StatusCodeFilter = from.StatusCodeFilter; | ||||||
| } | ||||||
|
|
||||||
| /// <summary>Using <see cref="Nullable{T}"/> where T is <see cref="int"/> to have <see langword="null"/> as default value and allowing global configuration usage.</summary> | ||||||
|
|
@@ -23,4 +26,6 @@ public FileCacheOptions(FileCacheOptions from) | |||||
| /// <remarks>If <see langword="null"/> then use global configuration with <see langword="false"/> by default.</remarks> | ||||||
| /// <value><see langword="true"/> if content hashing is enabled; otherwise, <see langword="false"/>.</value> | ||||||
| public bool? EnableContentHashing { get; set; } | ||||||
|
|
||||||
| public HttpStatusCodeFilter StatusCodeFilter { get; set; } | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| public HttpStatusCodeFilter StatusCodeFilter { get; set; } | |
| public int[] StatusCodes { get; set; } |
Note that the type is not the old HttpStatusCode[] but rather int[], an array of integers. If an Ocelot user is not a C# developer, they might not be familiar with the HttpStatusCode enumeration and its values!
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've just pushed a new commit 9013937 with the fix, but I assume the HttpStatusCode enumeration is parsed without issues by the JSON deserializer from the System.Text.Json namespace. I am not so sure about the Newtonsoft.Json deserializer, which is currently the default parser in Ocelot, though we plan to replace Newtonsoft.Json with System.Text.Json soon. If Newtonsoft handles C# enumerations well, hopefully, it might be worth rethinking the proposed property. Either way, the int[] StatusCodes property will remain the main one. I just commented out the line so we can go over this again.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| using Ocelot.Configuration.File; | ||
|
|
||
| namespace Ocelot.Filter | ||
| { | ||
| public abstract class Filter<T> : IFilter<T> | ||
| { | ||
| public FilterType FilterType { get; set; } | ||
| public T[] Values { get; set; } | ||
|
|
||
| public Filter() { } | ||
| public Filter(FilterType filterType, T[] values) | ||
| { | ||
| FilterType = filterType; | ||
| Values = values; | ||
| } | ||
|
|
||
| public virtual bool PassesFilter(T value) | ||
| { | ||
| // there's probably a clever way to clean this up but this is legible. | ||
| if (FilterType == FilterType.Blacklist) | ||
| { | ||
| foreach (var item in Values) | ||
| { | ||
| if (value.Equals(item)) | ||
| { | ||
| return false; // the value is in the blacklist, so it can't pass the filter | ||
| } | ||
| } | ||
|
|
||
| return true; // the value wasn't found in the blacklist, so it passes the filter | ||
| } | ||
| else // filter is whitelist | ||
| { | ||
| foreach (var item in Values) | ||
| { | ||
| if (value.Equals(item)) | ||
| { | ||
| return true; // the value is in the whitelist, so it passes the filter | ||
| } | ||
| } | ||
|
|
||
| return false; // the value is not in the whitelist, so it doesn't pass the filter | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| namespace Ocelot.Filter | ||
| { | ||
| public enum FilterType | ||
| { | ||
| Whitelist, | ||
| Blacklist, | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| using System; | ||
|
|
||
| namespace Ocelot.Filter | ||
| { | ||
| public class HttpStatusCodeFilter : Filter<HttpStatusCode> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This filter will likely become useless because defining the values of the C#
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I can change all this to be an integer based filter and not an HttpStatusCode enumeration based filter. |
||
| { | ||
| public HttpStatusCodeFilter() { } | ||
|
|
||
| public HttpStatusCodeFilter(FilterType filterType, HttpStatusCode[] values) : base(filterType, values ?? Enum.GetValues(typeof(HttpStatusCode)).Cast<HttpStatusCode>().ToArray()) | ||
| { | ||
| } | ||
|
|
||
| public HttpStatusCodeFilter(FilterType filterType, string[] values) :base(filterType, ParseInput(values)) | ||
| { | ||
| } | ||
|
|
||
| static HttpStatusCode[] ParseInput(string[] values) | ||
| { | ||
| List<HttpStatusCode> valuesList = new List<HttpStatusCode>(); | ||
| if (values == null) | ||
| { | ||
| valuesList.AddRange(Enum.GetValues(typeof(HttpStatusCode)).Cast<HttpStatusCode>()); | ||
| } | ||
| else | ||
| { | ||
| foreach (var value in values) | ||
| { | ||
| int i; | ||
| if (int.TryParse(value, out i)) | ||
| { | ||
| if (Enum.IsDefined(typeof(HttpStatusCode), i)) | ||
| { | ||
| valuesList.Add((HttpStatusCode)i); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| foreach (var code in Enum.GetValues<HttpStatusCode>()) | ||
| { | ||
| string codeClass = ((int)code / 100) + "xx"; | ||
| bool code_is_in_range = string.Equals(value, codeClass, StringComparison.OrdinalIgnoreCase); | ||
| if (code_is_in_range) | ||
| { | ||
| valuesList.Add(code); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return valuesList.Distinct().ToArray(); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| namespace Ocelot.Filter | ||
| { | ||
| public interface IFilter<T> | ||
| { | ||
| bool PassesFilter(T value); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you suggesting using multiple filters for different options?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought perhaps having a generic Filter types could assist in the future. In particular, I saw #1587 and figured it could be used there, too. The type is unnecessary for the current feature though, so it can be removed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could assist in future?... Sorry? Are you a member of our team?
No, it is better to focus on the feature you're currently working on rather than trying to enhance or refactor other features and code areas. I've requested community help for #1587 since the author has no intention of contributing, so they have been unassigned. I think this filter might be useful for |
||
| } | ||
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.
PassesFilter(cached.StatusCode)method could serve as a small helper in theCacheOptionsbusiness object, though it depends on the final design.options.StatusCodeFilter.PassesFiltergives the impression that an additional filter has been injected into the logic, but that doesn’t seem to be the case. Since we’re not varying filters, we’ll just have a single filter.