fix: add streaming flag and error handling #3214
Conversation
- Evaluates if a client's operation requires http streaming flag to be set to true, and if so then it pass the option along. - Make error parsers to reused a parsed body for scenario where the body is non seekable and can't be consumed again. Also change the condition: (!$body->isSeekable() || $body->getSize()) To: (!$body->isSeekable() || !$body->getSize()) The reason is that a stream non seekable will most likely not have a size, which means this condition will always be true.
Reverted the condition to what it was: (!$body->isSeekable() || $body->getSize()) Why?, I need to understand better what is the usage of this condition before changing. It was breaking some integ tests.
- When a response body is empty then we dont try to parse it. - When the response body is non seekable then, we read the full body before parsing.
|
- Remove lowercasing parsed in rest json error parser.
Error is parsed when the body of the response is not seekable.
|
|
||
| abstract protected function payload( | ||
| ResponseInterface $response, | ||
| ResponseInterface|SimpleXMLElement|array $responseOrParsedBody, |
There was a problem hiding this comment.
this would be better as mixed so we're not pulling in codec-specific dependencies, but mixing responses and parsed data here doesn't feel right— is it possible to delegate this call back to extractPayload?
| throw new \RuntimeException('The HTTP handler was rejected without an "exception" key value pair.'); | ||
| } | ||
|
|
||
| $serviceError = "AWS HTTP error: " . $err['exception']->getMessage(); |
There was a problem hiding this comment.
The change in format might affect log parsing. I'd leave this unless it's accounted for later
| return $this->parser->parse($member, $errorBody[0]); | ||
| } | ||
|
|
||
| throw new ParserException( |
There was a problem hiding this comment.
This could be a breaking change if callers don't expect this
There was a problem hiding this comment.
Still has the static $streamingCommands property defined (not shown in diff), but it's no longer used. Should be removed
| $tempBody = Utils::streamFor(); | ||
| Utils::copyToStream($body, $tempBody); | ||
| $body = $tempBody; |
There was a problem hiding this comment.
this could be (lazy rather than eager):
if (!$body->isSeekable()) {
$body = new CachingStream($body);
}There was a problem hiding this comment.
I don't think it is necessary to have a lazy stream there. The whole content is consumed right after this block, at line 52. Unless you are thinking in any other use case?
There was a problem hiding this comment.
I have replaced that logic for $body->getContents() instead, which avoids having to do a copy operation.
When the response body is a non seekable stream, which happens when the stream flag is set in the request, the parser condition to do a parsing is evaluated to true, but in empty payloads, such as when doing head requests, it fails. To remedy this we read the full body content and evaluate whether the returned value is empty before parsing.
2 participants
Issues #3124, #3018, #3181:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.