fix: add streaming flag and error handling by yenfryherrerafeliz · Pull Request #3214 · aws/aws-sdk-php

yenfryherrerafeliz · 2025-11-20T20:12:20Z

Description of changes:

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.
Make sure empty payloads are not parsed.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

- 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.

yenfryherrerafeliz · 2025-11-20T21:50:57Z

Running integ tets
vendor/bin/behat --format=progress --tags=integ
...................................................................... 70
...................................................................... 140
...................................................................... 210
......................................

66 scenarios (66 passed)
248 steps (248 passed)
5m9.29s (45.13Mb)
Running smoke tests
vendor/bin/behat --format=progress --suite=smoke --tags='~@noassumerole'
...................................................................... 70
...................................................................... 140
...................................................................... 210
............

111 scenarios (111 passed)
222 steps (222 passed)
0m45.99s (95.47Mb)

- Remove lowercasing parsed in rest json error parser.

Error is parsed when the body of the response is not seekable.

src/Api/ErrorParser/AbstractErrorParser.php

stobrien89 · 2025-12-23T23:28:53Z

src/WrappedHttpHandler.php

            throw new \RuntimeException('The HTTP handler was rejected without an "exception" key value pair.');
        }

-        $serviceError = "AWS HTTP error: " . $err['exception']->getMessage();


The change in format might affect log parsing. I'd leave this unless it's accounted for later

I basically changed this because it was duplicating the error message. What I did in specific was to append the message from the exception when a response is not available, but, if we have a response then we just use the error message from the deserialization process.

if (!isset($err['response'])) { $parts = ['response' => null]; $serviceError .= $err['exception']->getMessage(); // MOVED HERE. }

src/Api/ErrorParser/XmlErrorParser.php

src/CloudWatchLogs/CloudWatchLogsClient.php

src/Api/ErrorParser/XmlErrorParser.php

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.

To make a body content to be fully available always in implementations a response wrapper class has been created that takes in a response and evaluates if the body is non-seekable then it wrappes it into a CachingStream that will allow the body to rewind.

src/Api/ResponseWrapper.php

Overwrites responses on non-seekable streams by wrapping the non-seekable body on a CachingStream to allow rewind.

Avoid consuming original body before it is cached.

yenfryherrerafeliz · 2026-01-12T04:38:56Z

Running integ tets
vendor/bin/behat --format=progress --tags=integ
...................................................................... 70
...................................................................... 140
...................................................................... 210
......................................

66 scenarios (66 passed)
248 steps (248 passed)
10m14.82s (43.42Mb)

Running smoke tests
vendor/bin/behat --format=progress --suite=smoke --tags='~@noassumerole'
...................................................................... 70
...................................................................... 140
...................................................................... 210
............

111 scenarios (111 passed)
222 steps (222 passed)
0m35.47s (92.58Mb)

stobrien89

A few more comments and an old one (log parsing) that needs to be addressed

src/Api/ErrorParser/JsonRpcErrorParser.php

stobrien89 · 2026-02-16T22:17:21Z

src/Api/ErrorParser/JsonParserTrait.php

+        if ($body->isSeekable()) {
+            $body->rewind();
+        }
+
+        $rawBody = $body->getContents();
+        if (!empty($rawBody)) {
+            $parsedBody = $this->parseJson($rawBody, $response);
        }


Could this logic and the logic in payload() for JSON and XML parsers be moved into the abstract parsers?

This logic is like a generic method for parsing JSON payload and is used by JsonRpcErrorParser and RestJsonErrorParser, and actually it was already there; I just changed a few things such as the empty content evaluation, etc.

Regarding moving payload into the abstract class, I think we could, but I would like to understand more why we want to do it?

src/Api/Parser/AbstractRestParser.php

yenfryherrerafeliz requested a review from stobrien89 November 20, 2025 20:12

yenfryherrerafeliz added 2 commits November 20, 2025 12:41

revert: revert condition change

ee873c7

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.

fix: handle empty body responses

a78557b

- 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.

yenfryherrerafeliz added 5 commits November 20, 2025 13:57

fix: no populate data with body fields

93daaab

chore: make parsed to be all lowercase when returnd

e831463

chore: remove lowercasing parsed

0ca2ca6

- Remove lowercasing parsed in rest json error parser.

test: test error is parsed

db5dd6a

Error is parsed when the body of the response is not seekable.

chore: add code to expected error

8d1cdcd

yenfryherrerafeliz marked this pull request as ready for review November 26, 2025 15:04

yenfryherrerafeliz mentioned this pull request Dec 8, 2025

feat: cbor protocol support #3223

Open

stobrien89 requested changes Dec 23, 2025

View reviewed changes

yenfryherrerafeliz added 5 commits December 24, 2025 08:58

chore: remove unused variable

bd73efd

chore: use a response wrapper

dfa9e41

To make a body content to be fully available always in implementations a response wrapper class has been created that takes in a response and evaluates if the body is non-seekable then it wrappes it into a CachingStream that will allow the body to rewind.

chore: add response wrapper in abstract parser

51140be

chore: add nullable mark to request

e7167fa

yenfryherrerafeliz requested a review from stobrien89 January 2, 2026 16:37

chore: remove unused imports

461e2e0

stobrien89 requested changes Jan 2, 2026

View reviewed changes

src/Api/ResponseWrapper.php Outdated Show resolved Hide resolved

yenfryherrerafeliz added 3 commits January 11, 2026 19:45

temp: parser fix

b24c3b3

chore: overwrite response on non-seekable streams

edca9b6

Overwrites responses on non-seekable streams by wrapping the non-seekable body on a CachingStream to allow rewind.

chore: avoid consuming original body

adc7143

Avoid consuming original body before it is cached.

yenfryherrerafeliz requested a review from stobrien89 January 13, 2026 16:13

stobrien89 requested changes Feb 16, 2026

View reviewed changes

chore: simplify logic

2f37032

yenfryherrerafeliz requested a review from stobrien89 February 17, 2026 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add streaming flag and error handling#3214

fix: add streaming flag and error handling#3214
yenfryherrerafeliz wants to merge 18 commits intoaws:masterfrom
yenfryherrerafeliz:feat_add_streaming_flag_middleware

yenfryherrerafeliz commented Nov 20, 2025 •

edited

Loading

Uh oh!

yenfryherrerafeliz commented Nov 20, 2025

Uh oh!

Uh oh!

stobrien89 Dec 23, 2025

Uh oh!

yenfryherrerafeliz Feb 17, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yenfryherrerafeliz commented Jan 12, 2026

Uh oh!

stobrien89 left a comment

Uh oh!

Uh oh!

stobrien89 Feb 16, 2026

Uh oh!

yenfryherrerafeliz Feb 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

yenfryherrerafeliz commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yenfryherrerafeliz commented Nov 20, 2025

Uh oh!

Uh oh!

stobrien89 Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

yenfryherrerafeliz Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yenfryherrerafeliz commented Jan 12, 2026

Uh oh!

stobrien89 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stobrien89 Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

yenfryherrerafeliz Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yenfryherrerafeliz commented Nov 20, 2025 •

edited

Loading