csarrazi / guzzle-cache-middleware

A Guzzle Cache middleware
54 stars 10 forks source link

Mock filename tokenizer #3

Open Ciloe opened 6 years ago

Ciloe commented 6 years ago

https://github.com/csarrazi/CsaGuzzleBundle/issues/208

Hi, I saw a problem with the fixture file name generated by this function :

src/Cache/NamingStrategy/AbstractNamingStrategy.php

protected function getFingerprint(RequestInterface $request)
    {
        return md5(serialize([
            'method' => $request->getMethod(),
            'path' => $request->getUri()->getPath(),
            'query' => $request->getUri()->getQuery(),
            'user_info' => $request->getUri()->getUserInfo(),
            'port' => $request->getUri()->getPort(),
            'scheme' => $request->getUri()->getScheme(),
            'headers' => array_diff_key($request->getHeaders(), array_flip($this->blacklist)),
        ]));
    }

It's appear that if wee requested a graphQl API, we use the body to send the request and not the query. Is it possible to add 'body' => $request->getBody(), in this array ?

I think a new naming strategy is a good idea. But what can we use in the strategy ? I think this header is the minimum

       [
            'method' => $request->getMethod(),
            'user_info' => $request->getUri()->getUserInfo(),
            'port' => $request->getUri()->getPort(),
            'scheme' => $request->getUri()->getScheme(),
            'headers' => array_diff_key($request->getHeaders(), array_flip($this->blacklist)),
        ]

But, with my tests I can see some problems.

What is the good solution for you ?

csarrazi commented 6 years ago

Hi @Ciloe, and sorry for the late answer. It seems that when you create a new repository on GitHub, you no longer automatically watch the repository, so you miss all notifications. My bad on that one.

Can you provide some call examples, so it is more clear?

What I think is that we should use all possible inputs for generating the hash. Indeed, it seems that in your case, you need all information from the URI + the actual body. This could be passed as an option to the naming strategy, or we could create a BodyAwareNamingStrategy, which would take all parts of the URI + the body's content.

Do you think that would do the trick?

Ciloe commented 6 years ago

Hi !

For example we can call https://api.host.com/v1/ with the body

{
  userList (someoptions) {
    edges { node {
      name
      login
    }}
  }
}

The url is always the same, but the body change. I saied that

If we use the body : 'body' => $request->getBody(). If the request has generic fragments and we change the fragment signature. The naming will change, and we will loose the last name. We need then to delete and regenerate all fixtures...

So at the time, in my client, witch have defined url, I hash my url dans pass them in the query with CSA guzzle to call my api. This will create a unique naming stategy for the cache and the fixture file. But I think it's not a good approche.

What do you think ?

csarrazi commented 6 years ago

When speaking about GraphQL, a different body means a different signature, and thus should be considered a different request. I think we should take all parts of the URI + the body. This will let you remove the hashed URI from the query string. But in any case, passing parameters through the URI is still a valid. So in your naming strategy, I think you should use all parts of the request:

[
            'method' => $request->getMethod(),
            'path' => $request->getUri()->getPath(),
            'query' => $request->getUri()->getQuery(),
            'user_info' => $request->getUri()->getUserInfo(),
            'port' => $request->getUri()->getPort(),
            'scheme' => $request->getUri()->getScheme(),
            'headers' => array_diff_key($request->getHeaders(), array_flip($this->blacklist)),
            'body' => (string) $request->getBody()
        ]

Generally speaking, fixtures / mocks are highly dependent on the request's actual signature. So if you change a fragment, this means that you change at least a part of the query, meaning that you should indeed regenerate the associated mocks. This is the expected behaviour.

If you want to prevent regenerating all fixtures in case you just change a generic fragment, in this case you might want to have a more specific naming strategy, which would filter the body's contents, which would calculate a hash based on the significant part of your query.