search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.34k stars 1.63k forks source link

output caching issue #741

Open nurhat opened 5 years ago

nurhat commented 5 years ago

Hi, An endpoint with caching enabled caches output even if downstream request returns anything other than httpstatus code with 200. Is it by design, or a bug?

philproctor commented 5 years ago

@nurhat is this happening with 4xx and 5xx requests? Success isn't 200 only.

If so, it sounds like a bug to me -- @TomPallister is there something I'm missing as to why non-success would still be cached?

nurhat commented 5 years ago

@philproctor yes, if you check OutputCacheMiddleware.cs line 55, it does not check http status code. Caching only disabled when http request fails with exception.

philproctor commented 5 years ago

Got it -- I'll keep this open as a bug.

margaale commented 5 years ago

I think this was intentional and i wouldn't call it a bug. Why, for example, wouldn't you cache a 404 response if the api behind is telling you that the resource doesn't exist?

philproctor commented 5 years ago

404 is a response type that might make sense to cache, but there's also a lot of other 4xx and 5xx responses that don't make sense to cache. Pretty much anything in the 5xx series shouldn't be cached, and some 4xx responses may be made due to current state of the services or similar.

Perhaps if response is not .IsSuccessStatusCode(), we should check against a configured whitelist of cacheable responses.

margaale commented 5 years ago

Yep, it was just an example. You mean configured by the user?

philproctor commented 5 years ago

That's my thought, as part of the cache config.

margaale commented 5 years ago

mmmm, i don't like that. What if someone wants to disable one code for a downstream but not for another?

philproctor commented 5 years ago

I was thinking about the ReRoute cache config, not the global. Maybe something like:

          "FileCacheOptions": {
              "TtlSeconds": 0,
              "Region": "",
              "StatusCodes": [400, 404, 405, 410]
          },
margaale commented 5 years ago

Ohhhhhh that's better, but I think it's going to be kinda painful to declare all the routes explicitly. Maybe some global whitelist configuration with exceptions per route would be easier

philproctor commented 5 years ago

I agree on the global + per route

nurhat commented 5 years ago

@philproctor , any progress for the issue? thx

apoteet commented 3 years ago

We're still having this problem with 500 responses being cached. Any chance a config was added and this issue just got buried?