-
Notifications
You must be signed in to change notification settings - Fork 39
Add BatchResult #48
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
Add BatchResult #48
Changes from 1 commit
cf38fcc
8d6abb2
e6acb53
f68ad2c
437a2e1
3950679
9cb62d9
708c447
3eb7cbc
44e8c01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client; | ||
|
||
use Http\Client\Exception; | ||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class BatchResultSpec extends ObjectBehavior | ||
{ | ||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\BatchResult'); | ||
} | ||
|
||
function it_is_immutable(RequestInterface $request, ResponseInterface $response, Exception $exception) | ||
{ | ||
$new = $this->addResponse($request, $response); | ||
$new2 = $new->addException($request, $exception); | ||
|
||
$this->getResponses()->shouldReturn([]); | ||
$this->getExceptions()->shouldReturn([]); | ||
$new->shouldHaveType('Http\Client\BatchResult'); | ||
$new->getResponses()->shouldReturn([$response]); | ||
$new->getExceptions()->shouldReturn([]); | ||
$new2->shouldHaveType('Http\Client\BatchResult'); | ||
$new2->getResponses()->shouldReturn([$response]); | ||
$new2->getExceptions()->shouldReturn([$exception]); | ||
} | ||
|
||
function it_has_a_response_for_a_request(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$new = $this->addResponse($request, $response); | ||
|
||
$this->getResponseFor($request)->shouldReturn(null); | ||
$this->hasResponses()->shouldReturn(false); | ||
$this->hasResponseFor($request)->shouldReturn(false); | ||
$this->getResponses()->shouldReturn([]); | ||
$this->isSuccessful($request)->shouldReturn(false); | ||
$this->isFailed($request)->shouldReturn(false); | ||
$new->getResponseFor($request)->shouldReturn($response); | ||
$new->hasResponses()->shouldReturn(true); | ||
$new->hasResponseFor($request)->shouldReturn(true); | ||
$new->getResponses()->shouldReturn([$response]); | ||
$new->isSuccessful($request)->shouldReturn(true); | ||
$new->isFailed($request)->shouldReturn(false); | ||
} | ||
|
||
function it_has_an_exception_for_a_request(RequestInterface $request, Exception $exception) | ||
{ | ||
$new = $this->addException($request, $exception); | ||
|
||
$this->getExceptionFor($request)->shouldReturn(null); | ||
$this->hasExceptions()->shouldReturn(false); | ||
$this->hasExceptionFor($request)->shouldReturn(false); | ||
$this->getExceptions()->shouldReturn([]); | ||
$this->isSuccessful($request)->shouldReturn(false); | ||
$this->isFailed($request)->shouldReturn(false); | ||
$new->getExceptionFor($request)->shouldReturn($exception); | ||
$new->hasExceptions()->shouldReturn(true); | ||
$new->hasExceptionFor($request)->shouldReturn(true); | ||
$new->getExceptions()->shouldReturn([$exception]); | ||
$new->isSuccessful($request)->shouldReturn(false); | ||
$new->isFailed($request)->shouldReturn(true); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,196 @@ | ||
<?php | ||
|
||
namespace Http\Client; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
|
||
/** | ||
* Successful responses and exceptions returned from parallel request execution | ||
* | ||
* @author Márk Sági-Kazár <[email protected]> | ||
*/ | ||
final class BatchResult | ||
{ | ||
/** | ||
* @var \SplObjectStorage | ||
*/ | ||
private $responses; | ||
|
||
/** | ||
* @var \SplObjectStorage | ||
*/ | ||
private $exceptions; | ||
|
||
public function __construct() | ||
{ | ||
$this->responses = new \SplObjectStorage(); | ||
$this->exceptions = new \SplObjectStorage(); | ||
} | ||
|
||
/** | ||
* Checks if a request is successful | ||
* | ||
* @param RequestInterface $request | ||
* | ||
* @return boolean | ||
*/ | ||
public function isSuccessful(RequestInterface $request) | ||
{ | ||
return $this->responses->contains($request); | ||
} | ||
|
||
/** | ||
* Checks if a request is failed | ||
* | ||
* @param RequestInterface $request | ||
* | ||
* @return boolean | ||
*/ | ||
public function isFailed(RequestInterface $request) | ||
{ | ||
return $this->exceptions->contains($request); | ||
} | ||
|
||
/** | ||
* Returns all successful responses | ||
* | ||
* @return ResponseInterface[] | ||
*/ | ||
public function getResponses() | ||
{ | ||
$responses = []; | ||
|
||
foreach ($this->responses as $request) { | ||
$responses[] = $this->responses[$request]; | ||
} | ||
|
||
return $responses; | ||
} | ||
|
||
/** | ||
* Returns a response of a request or null if not found | ||
* | ||
* @param RequestInterface $request | ||
* | ||
* @return ResponseInterface|null | ||
*/ | ||
public function getResponseFor(RequestInterface $request) | ||
{ | ||
if ($this->responses->contains($request)) { | ||
return $this->responses[$request]; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. otherwise, we should look in the exceptions to see if there is an exception carrying a response and return that. if that is not found, i would throw the ecxeption we find at $request. or an invalid argument exception if the request does not exist. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently responses are handled the following way: every response and exception is collected in the BatchResult. If there are any exceptions, The BatchException gets the result and it is thrown. There is a getExceptionFor method to get the exception. If that exception is HttpException, you can get a response. I would not mix the two methods. I also wouldn't throw the exception, because that way you cannot process the rest. The reason why I put exceptions here as well is that an exception cannot be immutable. But it would possibly be better, to collect exceptions somewhere else (eg. right in the batch exception?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have an ugly idea for that. 😜 In batch exception accept two objects: batch result, exception chain The exception chain would be an object which accepts a request, an exception and the previous/next exception. THis way you have one object in the end to be injected into the batch exception. You can always get the next exception by calling getNextException, which returns the next in the chain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm. when i think of responses, i might want to process all responses, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I dunno about throwing it... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I tried first. The problem is that an Exception cannot be cloned, so making it immutable that way doesn't work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm. i would prefer the clean and logic code structure over insisting on
the immutability. if people want to do weird stuff, they always find
ways to do it.
we can do an `@internal` annotation on the add method and note you
should not use this outside a client/adapter.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we do this in case of the BatchResult as well? I think it would be weird if the Result is immutable, the exception is not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would keep it immutable. its just a technical detail that the exception
is not forced immutable. it is to be considered immutable by the user
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
|
||
/** | ||
* Checks if there are any successful responses at all | ||
* | ||
* @return boolean | ||
*/ | ||
public function hasResponses() | ||
{ | ||
return $this->responses->count() > 0; | ||
} | ||
|
||
/** | ||
* Checks if there is a response of a request | ||
* | ||
* @param RequestInterface $request | ||
* | ||
* @return ResponseInterface | ||
*/ | ||
public function hasResponseFor(RequestInterface $request) | ||
{ | ||
return $this->responses->contains($request); | ||
} | ||
|
||
/** | ||
* Adds a response in an immutable way | ||
* | ||
* @param RequestInterface $request | ||
* @param ResponseInterface $response | ||
* | ||
* @return BatchResult | ||
*/ | ||
public function addResponse(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$new = clone $this; | ||
$new->responses->attach($request, $response); | ||
|
||
return $new; | ||
} | ||
|
||
/** | ||
* Returns all exceptions | ||
* | ||
* @return Exception[] | ||
*/ | ||
public function getExceptions() | ||
{ | ||
$exceptions = []; | ||
|
||
foreach ($this->exceptions as $request) { | ||
$exceptions[] = $this->exceptions[$request]; | ||
} | ||
|
||
return $exceptions; | ||
} | ||
|
||
/** | ||
* Returns an exception for a request or null if not found | ||
* | ||
* @param RequestInterface $request | ||
* | ||
* @return Exception|null | ||
*/ | ||
public function getExceptionFor(RequestInterface $request) | ||
{ | ||
if ($this->exceptions->contains($request)) { | ||
return $this->exceptions[$request]; | ||
} | ||
} | ||
|
||
/** | ||
* Checks if there are any exceptions at all | ||
* | ||
* @return boolean | ||
*/ | ||
public function hasExceptions() | ||
{ | ||
return $this->exceptions->count() > 0; | ||
} | ||
|
||
/** | ||
* Checks if there is an exception for a request | ||
* | ||
* @param RequestInterface $request | ||
* | ||
* @return Exception | ||
*/ | ||
public function hasExceptionFor(RequestInterface $request) | ||
{ | ||
return $this->exceptions->contains($request); | ||
} | ||
|
||
/** | ||
* Adds an exception in an immutable way | ||
* | ||
* @param RequestInterface $request | ||
* @param Exception $exception | ||
* | ||
* @return BatchResult | ||
*/ | ||
public function addException(RequestInterface $request, Exception $exception) | ||
{ | ||
$new = clone $this; | ||
$new->exceptions->attach($request, $exception); | ||
|
||
return $new; | ||
} | ||
|
||
public function __clone() | ||
{ | ||
$this->responses = clone $this->responses; | ||
$this->exceptions = clone $this->exceptions; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,61 +2,49 @@ | |
|
||
namespace Http\Client\Exception; | ||
|
||
use Http\Client\BatchResult; | ||
|
||
/** | ||
* @author Márk Sági-Kazár <[email protected]> | ||
*/ | ||
final class BatchException extends TransferException | ||
final class BatchException extends RuntimeException | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets document this: This exception is thrown when a batch of requests led to at least one failure. It holds the response/exception pairs and gives access to a BatchResult with the successful responses. |
||
{ | ||
/** | ||
* @var TransferException[] | ||
* @var BatchResult | ||
*/ | ||
private $exceptions; | ||
private $result; | ||
|
||
/** | ||
* @param TransferException[] $exceptions | ||
* @param BatchResult $result | ||
*/ | ||
public function __construct(array $exceptions = []) | ||
public function __construct(BatchResult $result) | ||
{ | ||
parent::__construct('An error occurred when sending multiple requests.'); | ||
|
||
foreach ($exceptions as $e) { | ||
if (!$e instanceof TransferException) { | ||
throw new InvalidArgumentException('Exception is not an instanceof Http\Client\Exception\TransferException'); | ||
} | ||
} | ||
|
||
$this->exceptions = $exceptions; | ||
$this->result = $result; | ||
} | ||
|
||
/** | ||
* Returns all exceptions | ||
* Returns successful responses or null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returns a BatchResult object which contains succesful responses and exceptions for failed requests. |
||
* | ||
* @return TransferException[] | ||
* @return BatchResult | ||
*/ | ||
public function getExceptions() | ||
public function getResult() | ||
{ | ||
return $this->exceptions; | ||
return $this->result; | ||
} | ||
|
||
/** | ||
* Checks if a specific exception exists | ||
* Decides whether the result has any failures | ||
* | ||
* @param TransferException $exception | ||
* @param BatchResult $result | ||
* | ||
* @return boolean TRUE if there is the exception else FALSE. | ||
* @return BatchResult|BatchException | ||
*/ | ||
public function hasException(TransferException $exception) | ||
public static function decideReturnValue(BatchResult $result) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if we need this. is the idea to avoid repeating logic in the adapters? while we are at it, we could have a second parameter to decide whether to throw the BatchException or return it. otherwise you would need an instanceof check if you want to throw, which would be weird. and it would be even more weird to return a BatchException from the client. oh, but apart from that, i think the decision of throw vs return can be moved to the adapter configuration. so we just say Client::sendRequests either returns a successful BatchResult and you can configure the client whether you want it to throw a BatchException or return a BatchResult. hm. which sounds like a huge pitfal and a pain for generic code that does not know whether the user will configure the client to throw or return... so with this i think we have to make up our mind and decide for either the exception or not. my proposition is to say the client does the check for $result->hasExceptions() and if there are exceptions instantiates the BatchException and throws it. that way there is no ambiguity. and if i do have the same logic for both situations, in my application i do:
the alternative is that if i want an exception, the client would have to be written as
both seem viable and not too much work. but i feel the throwing an exception is more consistent with the sendRequest signature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You are right, I wasn't thinking when I wrote this. I agree with the rest: if the result has exceptions, throw it in a batch exception. It is also easier, because you have the same internal logic in sendRequest: ALWAYS collect responses and exceptions in BatchResult and it's not like: if we have an exception, we create a batchexception and put it there and in the end if we have a batchexception, we throw it. |
||
{ | ||
return array_search($exception, $this->exceptions, true) !== false; | ||
} | ||
if ($result->hasExceptions()) { | ||
return new self($result); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets throw an InvalidArgumentException if you request the exception for a request that is not found. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure about this. I always tend to rather return null than throwing an exception. Is it more elegant to throw one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, we should be consistent and do the same in batch result. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. returning null is a horrible thing imho :-)
as it makes errors go unnoticed. i rather have an exception with a
stacktrace than "fatal error: called method on something that is not an
object"
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, exceptions whenever something weird is being done imho
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SplObjectStorage throws UnexpectedValueException in case an object is not found. Probably we could catch that exception and throw ours. |
||
|
||
/** | ||
* Checks if any exception exists | ||
* | ||
* @return boolean | ||
*/ | ||
public function hasExceptions() | ||
{ | ||
return !empty($this->exceptions); | ||
return $result; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/and exceptions //