Skip to content
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

Make cache available per method and per status code #99

Open
Neirda24 opened this issue Nov 15, 2017 · 4 comments
Open

Make cache available per method and per status code #99

Neirda24 opened this issue Nov 15, 2017 · 4 comments

Comments

@Neirda24
Copy link
Contributor

As of now we can only exclude http methods. It would be great also to explicitly tell to not cache 404 (for example). A good idea might be to update this property: https://github.com/Kevinrob/guzzle-cache-middleware/blob/master/src/CacheMiddleware.php#L49 to make it like this:

protected $httpMethods = [
    'GET' => [
        200,
        201,
        // etc etc
    ]
];

It would be something with a default configuration. It could be overriden through strategy configuration and requests specifics.

@afoeder
Copy link
Contributor

afoeder commented Mar 7, 2018

I'd maybe consider working on this because we need it in our project. We're using it in conjunction with the GreedyCacheStrategy; and I think it would make sense to even have different TTLs depending on the type of the response status (this thinking is inspired by AWS CloudFront's behavior)

Any ideas how to logically implement this, i.e. putting this on (each) Caching Strategy's level or one layer above like the DelegateCache'ing?
Should this already be done just by userland with the DelegateCache or should the middleware provide some convenient presets?

@jeromegamez @bpolaszek allow me to "invite" you to the discussion since you were working on the GreedyCache already and I think you share some love :)

// addendum: oh and what regarding timeouts? We're sometimes experiencing timeouts on the remote service which is not business crucial, so it'd be okay to consider this a 504?

@bpolaszek
Copy link
Contributor

Hello @afoeder,

Thanks for inviting me to join this discussion.

There's is indeed a lack in GreedyCacheStrategy that forced me to implement my own CacheStrategy in one of my projects because my use case was not covered.

I think than instead of relying on http status codes, a cache strategy should also rely on the whole ResponseInterface object (which gives also access to the response status code).

For example, I work with a SAAS API that is pretty slow and doesn't send proper Cache headers nor status codes. The response code is always 200 and wether I should cache it or not (and how long) depends on the response body. Since the current GreedyCacheStrategy implementation does not allow to interact with the response, I had to override it:

use Kevinrob\GuzzleCache\Strategy\GreedyCacheStrategy;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;

/**
 * HasOffers API does not return any Cache header, and returns HTTP 200 even in case of failure.
 * We override the GreedyCacheStrategy to prevent caching failures by checking the API internal status.
 */
class HasOffersCacheStrategy extends GreedyCacheStrategy
{
    /**
     * @param RequestInterface  $request
     * @param ResponseInterface $response
     * @return bool
     * @throws \InvalidArgumentException
     */
    public function cache(RequestInterface $request, ResponseInterface $response)
    {

        if (false !== strpos((string) $response->getBody(), '"response":{"status":-1')) {
            return false;
        }
        return parent::cache($request, $response);
    }
}

This is a bit tricky and I think that kind of use case should be considered in GreedyCacheStrategy to avoid making things even dirtier.


What I suggest to improve GreedyCacheStrategy:

  • No longer rely on a KeyValueHttpHeader
  • Rely on a request matcher and / or a response matcher. So one would implement the GreedyCacheStrategy depending on their own use case: request method, request URI, response headers, response body, ... and everything could be combined.

@afoeder
Copy link
Contributor

afoeder commented Mar 7, 2018

thanks for your elaborate reply @bpolaszek. I have like a very similar case: our API we consume is totally unpredictable and yes, can confirm: everything is a 200 OK and the body is always some list'ish thing but maybe empty, even if one requests intentionally rubbish.

So, following your suggestions at the end and since naming is important, wouldn't the naming of the GreedyCacheStrategy be more appropriate if it was like ReflectiveCachingStrategy or IntrospectiveCachingStrategy, bespeaking the fact that it is some kind of examining inner things in order to decide what to do?

Having said this we might even come up with an additional Caching Strategy in order to stay backward compat and keep things simple and single-responsible.

Do you have an example (method) signature of how you'd imagine the Request and/or Response matcher?

Bottom line I wonder if this won't result in being too magic and maybe instead we should encourage people to not use a shipped Strategy and implement mathers; but just implement the whole strategy tailored to their use-case?

@bpolaszek
Copy link
Contributor

bpolaszek commented Mar 7, 2018

I work with many external APIs in many projects (like, dozens), and NONE of them follow standard caching directives. After years of implementing cache on my own in dirty ways, I discovered your middleware and the GreedyCacheStrategy helped me a lot (this is the only one I use, and the only time I needed to override it was to connect to an API that doesn't know any other status code than 200 and no other method than GET).

Even if I'd like to encourage the developer to specify its own request / response cache specifications, I sill think we need to make things easy for them.

IntrospectiveCachingStrategy sounds great indeed.

When I submitted #85 the library needed a Request Matcher. Afterwards, I needed other request matchers in my projects (for throttling requests, for instance), so in the same project, I had several request matchers, extending different RequestMatcherInterface. And I needed that in several projects.

Since the PSR-7 specification doesn't provide that kind of interfaces, I opened up the bentools/psr7-request-matcher repository. At the beginning it was just composed of a RequestMatcher interface, but I also added a ResponseMatcherInterface and a TransferMatcherInterface (to match a couple request/response).

The idea behind that is that these interfaces are too generic to my eyes to be coupled to a specific library, project or app, that's why I put them apart.

They could be used in guzzle-cache-middleware with our new IntrospectiveCachingStrategy, but we'll have to deal with the existing built-in RequestMatcher.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants