#741 Add filtering options and logic to caching

[twinetek]

Closes #741

• Caching based on response status #741

Proposed Changes

• Added an array of HttpStatusCode to FileCacheOptions and CacheOptions to act as a whitelist of status codes.
• Before adding the response to the cache, check to see if the response status code is in the whitelist.
• If the whitelist is null, assume all status codes are to be cached
• Tests. to be added (Would like some guidance on where to put these. I suspect OutputCacheMiddlewareTests. I've also never used XUnit so there's going to be a bit of a learning curve.)
• Looking for guidance. I believe this is where and how this cache filtering would be added, but I am new to the project and getting back into coding after several years. I've also never submitted a pull request to someone else's project before so I'm hoping I'm doing this correctly.

[raman-m]

Hello, Cliff!
Welcome to the world of Ocelot, and thank you for opening this PR and making an effort to follow our development process.

• Tests. to be added (Would like some guidance on where to put these. I suspect OutputCacheMiddlewareTests. I've also never used XUnit so there's going to be a bit of a learning curve.)

Well... correct, OutputCacheMiddlewareTests contains our unit tests for the OutputCacheMiddleware class. You can use MSTest classes and engine, and I hope both frameworks (xUnit and MSTest) can be combined in a single testing class.

commit 159da67

Please avoid using CacheManager in our tests, as this library will be deprecated in the next .NET 10 release due to issue #2334. Since release 24.0, we've removed the integration testing project and moved all integration tests to the Ocelot.AcceptanceTests project, where we combine both integration, acceptance and API testing. All caching acceptance tests should be kept in the Caching folder, so please move the "real cache" acceptance tests there.

• Looking for guidance. I believe this is where and how this cache filtering would be added, but I am new to the project and getting back into coding after several years. I've also never submitted a pull request to someone else's project before so I'm hoping I'm doing this correctly.

Sure, our team will provide guidance, but please be patient as I'm currently swamped with the hot v24.1 release we're aiming to finish this week. Also, please include acceptance tests as they are an important part of the development process alongside integration/unit tests, and I'll arrange for the team to carry out code reviews. Good luck with the delivery! 😉

[coveralls]

coverage: 93.506% (+0.003%) from 93.503%
when pulling 78b6936 on twinetek:allow-cache-filtering-based-on-reseponse-statuscode
into 9fc4e78 on ThreeMammals:develop.

[twinetek]

Raman,
Thanks. I'll try to find some time tonight to make those changes. I'm not in a rush, so any guidance as you or your team can provide at your convenience is just fine. Thank you for your patience!

[raman-m]

We overlooked an important detail: the global configuration, which was improved in the latest PR #2331. The current design adds a StatusCodes option, listing allowed or included codes. For users, listing all codes for a route is challenging, and doing the same in the global StatusCodes config is also cumbersome. What if we also took an exclusion scenario into account? When a user wants to exclude just a few statuses, the configuration becomes much simpler.
In the original #741 feature, the following configuration was discussed or proposed 👇

"CacheOptions": {
  "TtlSeconds": 0,
  "Region": "",
  "StatusCodes": [400, 404, 405, 410] // excluded or included?
}

I think this currently only covers the excluding scenario. We should reconsider your initial implementation to handle the including scenario as well. We also need to implement the excluding scenario explicitly. I suggest introducing groups of codes like 2xx, 3xx, 4xx, and 5xx, where having a single 200 code in the array would represent all 2xx statuses. Finally, we should add a management Boolean option to enable or disable the status codes feature on the fly, so that global status checks can be turned off for specific routes, and vice versa, re-enabled for certain routes even when globally disabled.

More technical details will follow.
Should I ask the team for a code review? Our review process isn't quick and could take a few weeks.

[twinetek]

Raman,
I will work on implementing the include/exclude logic and the 2xx/3xx/etc. range option. Might take me a few days.
Cliff

[twinetek]

Raman,
I've added some filtering logic. You can now specify the filtering and specifying whether to either use the list as a whitelist or a blacklist.

    public enum FilterType
    {
        Whitelist,
        Blacklist,
    }

The filter is added to CacheOptions using the new HttpStatusCodeFilter class:

CacheOptions = {
    TtlSeconds = 100,
    StatusCodeFilter = new HttpStatusCodeFilter(FilterType.Whitelist, new HttpStatusCode[] { HttpStatusCode.OK, HttpStatusCode.Forbidden }),
};

You can specify the codes in the filter with strings, including the 2xx, 3xx, etc. ranges

CacheOptions = {
    TtlSeconds = 100,
    StatusCodeFilter = new HttpStatusCodeFilter(FilterType.Whitelist, new string[] { "200", "4xx" }),
};

I have added some pretty exhaustive tests.

Would you like it extended to allow the items in the list as int? Or a mixed array of HttpStatusCode, string, or int?

[raman-m]

I've added some filtering logic. You can now specify the filtering and specifying whether to either use the list as a whitelist or a blacklist.

    public enum FilterType
    {
        Whitelist,
        Blacklist,
    }

I'm not in favor of using whitelisting and blacklisting, as it feels a bit awkward. I'm not in favor of using custom enumerations, as they involve writing extensive documentation, and parsing string values can hurt the gateway's performance when route artifacts are not cached. Please revert both commits!

Is this code and design the result of vibe coding with AI help? It feels overcomplicated and impractical.

It is fine to use AI to help write code, but only for an approved feature design❗

As contributors, professional C# developers are prohibited from using AI for coding, and a few "contributors" have been banned for doing so. I usually don't allow vibe coding in our repository. Since you are returning to the software development industry without extensive .NET experience, you're allowed to use AI under these restrictions. Also, could you share a link to your LinkedIn profile please?

In the Software Development Life Cycle, the first stages are requirements, planning, BI, design, and creating the specification (three phases), followed by the 4th stage, coding or implementation. However, the design was implemented without prior discussion. The issue with this design is that all filtering is handled through C# code, while the main purpose of Ocelot configuration is to use the JSON file (ocelot.json) with the appropriate ASP.NET JSON configuration provider. If an Ocelot gateway user isn't a .NET/C# developer, they would only be able to write JSON configurations. Therefore, we need to first discuss options for the FileCacheOptions JSON model❗

The old StatusCodes array option worked fine, but we also need to account for an exclusion scenario. The simplest JSON config, in my view, would introduce a new StatusCodesExcluded array, with the final name to be approved by the team. This would give us a clean setup like:

"CacheOptions": {
  "TtlSeconds": 888,
  "StatusCodes": [200, 301, 302, 303], // included codes, caching enabled
  "StatusCodesExcluded": [500, 404, 405, 410] // excluded codes, no caching
}

Possible names for the exclusion option could be StatusCodesExcluded, NoCachingStatusCodes, and similar.

Idea 1

I have an idea to combine both inclusion and exclusion scenarios by using negative values in an array to represent the exclusion (no caching) cases. For example:

"CacheOptions": {
  "TtlSeconds": 888,
  "StatusCodes": [200, 301, 302, 303, -500, -404, -405, -410] // mix of positive and negative codes
}

Here is the breakdown:

• 200 is a single code in the 2xx family, so all 2xx codes are included and will enable caching.
• The 3xx family has multiple codes, so only the listed ones (301, 302, 303) will enable caching.
• -500 is a single code in the 5xx family, so all 5xx codes are excluded and will disable caching.
• The 4xx family has multiple codes (-404, -405, -410), so only these will disable caching.
• Any other status codes not defined in the array won't be managed by the middleware and will be passed along to other middlewares. If the Caching middleware detects a downstream response status not in the StatusCodes array, it immediately returns control.

Idea 2

Another sub-feature is the addition of default StatusCodes by Ocelot, based on common best practices for reverse proxies when global configuration is absent. This should be discussed as well.

---

Cliff,
I've also decided to request a code review from the team. It's time to get their input.

P.S.
Don't forget the management option to enable or disable route caching exclusively. Currently, we control caching via the TtlSeconds value: positive enables caching, zero disables it. I'm proposing a new EnableCaching setting, either inside or outside the FileCacheOptions model, and possibly at the route level as well. We already have a few Enable options in File models, like EnableContentHashing, so this EnableCaching option would be a nice extra enhancement to include in this PR. We could pair program on this feature later, after implementing the main user scenarios.

[raman-m]

Would you like it extended to allow the items in the list as int? Or a mixed array of HttpStatusCode, string, or int?

Oh no, my dear Copilot! 🤣
Please revert the last two commits, such as cdcf273 and e8e4515.

[raman-m]

Some great tests have been added, but we are moving forward with redesigning the feature.

Consider this comment as the updated specification, serving as a revision of the #741 requirements. This isn't the final version, as I'm open to new ideas and discussions.

[raman-m]

$\color{red}{Critical\ issue\ with\ the\ property\ type!}$

Suggested change

	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!

[raman-m]

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.

[raman-m]

• The PassesFilter(cached.StatusCode) method could serve as a small helper in the CacheOptions business object, though it depends on the final design.
• The expression options.StatusCodeFilter.PassesFilter gives 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.

[raman-m]

Please wrap the logging for the Debug scenario with a preprocessor directive, as another team member will likely request this 👇

Suggested change

	Logger.LogDebug(() => $"Finished response added to cache for the '{downstreamUrlKey}' key.");
	#if DEBUG
	Logger.LogDebug(() => $"Finished response added to cache for the '{downstreamUrlKey}' key.");
	#endif

[raman-m]

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.
Here’s what I propose:

Suggested change

	=> new(options?.TtlSeconds, options?.Region, options?.Header, options?.EnableContentHashing, options?.StatusCodeFilter);
	=> new(options ?? new());

The null-coalescing operator (??) is used to handle argument null checks within the new constructor which will be designed by you

[twinetek]

Can do. I was just following the existing pattern. Not my project, so did not figure it was my place to make that change.

[raman-m]

Alright, never mind. I will handle it myself.

[raman-m]

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.
Finally, I expect the following improvement:

Suggested change

	var statusCodes = options.StatusCodeFilter ?? globalOptions.StatusCodeFilter;
	var statusCodes = options.StatusCodes ?? globalOptions.StatusCodes ?? Ocelot.RecommendedStatusCodes;

[raman-m]

Are you suggesting using multiple filters for different options?

[twinetek]

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.

[raman-m]

I thought perhaps having a generic Filter types could assist in the future.

Could assist in future?... Sorry? Are you a member of our team?

In particular, I saw #1587 and figured it could be used there, too.

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 SecurityOptions, but I am not entirely sure.

[raman-m]

This filter will likely become useless because defining the values of the C# HttpStatusCode enumeration is challenging for Ocelot users who aren’t .NET developers. I’ve already raised this issue here.

[twinetek]

Yes, I can change all this to be an integer based filter and not an HttpStatusCode enumeration based filter.

[twinetek]

So, no, I didn't use Copilot or any AI. The only assistance I had was when I copied all the HttpStatusCode enum values to VS Code and did some find/replace to create the InlineData test instances. The rest is hand-written. I honestly didn't think this was particularly complex code.

Regarding the question about whether to allow a user to add filters by int, string, or HttpStatusCode - I didn't know if you wanted to allow a user to specify both specific codes AND a code range like 200, 201, "4xx"

I went with explicit whitelist/blacklist because I wanted to avoid handling a conflict. If you're including specific status codes, then you're already implicitly excluding all the status codes not in the specified list. There's no need to specify other status codes to exclude, since they aren't included. The opposite is true - if you're specifying a blacklist, then that implies that everything not in your blacklist is included. If you would still like me to pursue allowing the user to specify codes to include AND codes to exclude, I am happy to iterate with you on what that logic looks like.

As I'm writing this, I'm seeing some of your code comments come through, but they are on the commits you asked to be reverted. Do you still want me to revert them, or just make changes going forward?

[twinetek]

I have added a link to my LinkedIn profile to my GitHub profile.
I will come back to this in a couple of days.

[raman-m]

I went with explicit whitelist/blacklist because I wanted to avoid handling a conflict.

Neither I nor the community are interested in what you wanted. The design should be straightforward, but I noticed a lot of code in this project that violates the KISS principle, serving no real purpose and adding unnecessary clutter.

If you're including specific status codes, then you're already implicitly excluding all the status codes not in the specified list. There's no need to specify other status codes to exclude, since they aren't included. The opposite is true - if you're specifying a blacklist, then that implies that everything not in your blacklist is included. If you would still like me to pursue allowing the user to specify codes to include AND codes to exclude, I am happy to iterate with you on what that logic looks like.

Fair enough! Look at these use cases and you will see there is no conflict. And you are absolutely right; positive codes exclude any unwanted values. No opposite codes here:

"CacheOptions": {
  "TtlSeconds": 888,
  "StatusCodes": [200, 301, 302, 303] // only positive codes to enable caching
}

Use case #741 covers the situation where users discussed the option of excluding a scenario. And no opposite codes here:

"CacheOptions": {
  "TtlSeconds": 888,
  "StatusCodes": [-500, -404, -405, -410] // only negative codes to disable caching
}

Looks good? The goal isn't to propose another option, like a blacklisting array. Both scenarios can be covered by a single StatusCodes array. The rest of the design is handled by a validator, which ensures that mixing positive and negative codes is not allowed, making it impossible to define both including and excluding scenarios.

---

I will come back to this in a couple of days.

No need to rush, as this PR will be part of the Winter'26 milestone. The upcoming .NET 10 release will be free of new features, focusing instead on project upgrades, code reorganization, DevOps and possible bug fixes.

Please enjoy your weekend.

[twinetek]

Hi Raman,

To defend my design choice - in #2337 (comment), you mentioned you wanted to explicitly add an exclusion scenario, which I take to mean that everything will be cached except for the codes in the list. This sounds like a black list to me, hence the FilterType enum. Using negative values to indicate an exclusion seems like specifying an exclusion list by convention instead of explicitly calling a list an exclusion. And, by explicitly specifying a list as a inclusion (white) or exclusion (black) list instead of the individual values in the list, the handling of a mixture of included and excluded values can be avoided. Again, I'm only trying to provide some context to the choices I made.

That said, I am only trying to make a contribution so I am happy to pivot and use the positive/negative convention. I have a follow-up question on the validation logic: if there is a mix, then what is the expected behavior? A couple of ideas

1. Take the first value and use it's sign to treat the list as an include vs exclude list, and ignore any values that are not the same sign as the first value.
2. Ignore the entire list as a misconfiguration and use some default/best practice list.

If a list is not specified, should the caching logic default to the existing behavior or do you want to implement a default include list? The latter could be considered a breaking change.

If you want a default or best practice list, what list do you want used?

An additional follow-up regarding the 2xx/3xx/etc . In #2337 (comment), your Idea 1 reads to me that you want 200 to be interpreted to mean any status code in the 2xx class. However, 200 is a valid status code in itself. Am I interpreting this correctly? If someone only wants to cache 200s but not 201, how should they indicate that in the list?

As I thought about this some more, I thought about non-standard status codes outside the range of 100-599. Should the user be allowed to specify 000-099 and 600-999 status codes? These might be used in custom systems. If not, how should the validation logic handle them?

I will likely be out of reach until mid-next week. If I can contribute when I return and you are willing to keep dealing with me, I will be happy to do so. Alternatively, if you would prefer to close the PR pending further requirements gathering (which admittedly I failed to do prior to submitting the PR), I understand.

Thanks, Cliff

[ggnaegi]

Ok, let me check that, @raman-m

[raman-m]

@ggnaegi commented on December 3

Would you like to mentor this PR?
OK, good luck!

[ggnaegi]

For me, the code is fine, but I have some concerns about its maintainability, because CachedResponse uses HttpStatusCode while here we’re using an array of int. I think it would be more appropriate to convert the array in CacheOptions and then use HttpStatusCode consistently throughout.

[twinetek]

@ggnaegi - any other changes you'd like to see? Happy to make them.

[raman-m]

@ggnaegi requested changes on December 6

For me, the code is fine, but I have some concerns about its maintainability, because CachedResponse uses HttpStatusCode while here we’re using an array of int.

This was my idea, and I requested changes after my code review. I think using integers is the simplest approach for users who are not familiar with C#.NET or are not C# developers. I was also concerned that parsing Enum types from JSON with the Newtonsoft.Json library could be challenging. To be fair, we need some parsing tests for ocelot.json to confirm or disprove this assumption. I'm fairly certain that the System.Text.Json library works perfectly with enums, but the current parsing library in use is Newtonsoft.Json (see upcoming PR #2125). Anyway, it seems I was mistaken, and we could have an array of HttpStatusCode values as proposed without issues, though more testing is still needed.
Given this conclusion, we could suggest that users choose between two definitions, either defining int[] and/or HttpStatusCode[], and the author's design was indeed correct. There is no issue with cherry-picking his initial two commits to bring back the second option. I'm totally fine with having both options available.

I think it would be more appropriate to convert the array in CacheOptions and then use HttpStatusCode consistently throughout.

It seems like an acceptable solution if offer both options, which can then be merged into a single collection within the CacheOptions model. However, the actual conversion method should be part of either the CacheOptions or CacheOptionsCreator classes. The latter is a more flexible approach, as it could be implemented as a protected virtual method to allow overriding.

Your opinion, Guil?