Skip to content

[HttpClient] Add a Record & Replay callback to the MockHttpClient. #35677

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[HttpClient] Add a Record & Replay callback to the MockHttpClient. #35677

wants to merge 4 commits into from

Conversation

GaryPEGEOT
Copy link
Contributor

@GaryPEGEOT GaryPEGEOT commented Feb 11, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets partially #30502
License MIT
Doc PR TODO

Allow to record & replay responses for test / dev purpose:

<?php

/* @var Psr\Cache\CacheItemPoolInterface $cache */
$cache = new ArrayAdapter();
$callback = new RecordReplayCallback(RecordReplayCallback::MODE_REPLAY_OR_RECORD, $cache)

$mockClient = new MockHttpClient($callback);

// Will make an actual HTTP request
$client->request('POST', 'https://example.org/whatever');

// Will replay the previous response
$client->request('POST', 'https://example.org/whatever');

TODO:

  • Create the PR for the documentation
  • Integrate with Framework bundle for easy config

@GaryPEGEOT GaryPEGEOT mentioned this pull request Feb 11, 2020
Closed
3 tasks
@nicolas-grekas nicolas-grekas added this to the next milestone Feb 11, 2020
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 29, 2020
Merged
@fabpot
Copy link
Member

fabpot commented Aug 11, 2020

@GaryPEGEOT Do you have time to finish this PR?

@GaryPEGEOT
Copy link
Contributor Author

Hi @fabpot! I would need some feedback on the callback before creating the Framework integration. PR #35893 gives the ability to integrate the callback easily.

nicolas-grekas
nicolas-grekas reviewed Aug 26, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about making this way simpler, by just recording + replaying the req/resp in order?

Right now, the hashing logic makes me think of this as a special cache layer, not a record-replay tape.

I feel like we might prefer implementing a real HTTP cache and make it able to force-store+serve requests, and have this one specialized on in-order serving.

WDYT?

src/Symfony/Component/HttpClient/RecordReplayCallback.php Outdated
*/
private $client;

public function __construct(string $mode, CacheItemPoolInterface $cache, HttpClientInterface $client = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we make the mode optional, defaulting to MODE_REPLAY_OR_RECORD?
That would ease integration with tests I believe: one wants to re-record? => bin/console cache:pool:clear some_pool_for_some_reqs then run the tests again.
WDYT?

src/Symfony/Component/HttpClient/RecordReplayCallback.php Outdated
$response = null;

if (isset($options['body'])) {
$parts[] = md5(static::getBodyAsString($options['body']));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better not use md5 for this.
we could use hash_init() instead of the $parts array

@nicolas-grekas
Copy link
Member

For inspiration btw: https://github.com/ikwattro/guzzle-stereo/

@fabpot
Copy link
Member

fabpot commented Aug 27, 2020

#35893 has been merged now.

@GaryPEGEOT GaryPEGEOT force-pushed the feature/http-rnr branch 2 times, most recently from 92aff83 to 2c57469 Compare September 6, 2020 09:29
nicolas-grekas
nicolas-grekas reviewed Sep 7, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this moves forward nicely.
One thing that doesn't work right now is async: the way this is currently implemented break async and streaming when recording.
It should be possible to record without breaking async by leveraging AsyncDecorator.
Up to continue this way?

src/Symfony/Component/HttpClient/Tests/RecordReplayCallbackTest.php Outdated

protected function setUp(): void
{
$recorder = new ResponseRecorder(sys_get_temp_dir(), new ResponseSerializer());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should clean up the temporary files created when running the tests

src/Symfony/Component/HttpClient/Tests/RecordReplayCallbackTest.php Outdated
$this->client = new MockHttpClient($this->callback);
}

public function testReplayOrRecord(): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove all : void from all test cases

src/Symfony/Component/HttpClient/RecordReplayCallback.php Outdated
private $client;

/**
* @var ResponseRecorder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove all @var that duplicate the constructor, they're useless

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 24, 2020 •
edited
Loading

Closing as we discussed on Slack: we should try another approach based on AsyncDecoratorTrait, that would share some logic/classes between this use case and the "client-side http-cache" use case.

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@rrajkomar
Copy link

@nicolas-grekas Has there been any progress or new leads on this feature ?
It would be awesome if we could get a working record/replay system for the HttpClient as currently there does not seem to be any viable alternative (aside from configuring a custom callback which would have to be duplicated accross projets, not ideal).
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
Feature HttpClient Status: Needs Review
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
5 participants
@GaryPEGEOT @fabpot @nicolas-grekas @rrajkomar @carsonbot