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

Ah, I see. So basically a caller will still see the same thing, just printed from somewhere else?

Yes, caller will still see the same thing, but with the difference that now error messages will not be duplicated. Previously, if we deserialized a modeled message then we duplicate the error we output.
For example, before we were getting:

PHP Fatal error: Uncaught exception 'Aws\S3\Exception\S3Exception' with message 'Error executing "GetObject" on "https://bucket.s3.us-east-2.amazonaws.com/no-exists"; AWS HTTP error: Client error: `GET https://bucket.s3.us-east-2.amazonaws.com/no-exists` resulted in a `404 Not Found` response: <?xml version="1.0" encoding="UTF-8"?> <Error><Code>NoSuchKey</Code><Message>The specified key does not exist.</Message> (truncated...) NoSuchKey (client): The specified key does not exist.' GuzzleHttp\Exception\ClientException: Client error: `GET https://bucket.s3.us-east-2.amazonaws.com/no-exists` resulted in a `404 Not Found` response: <?xml version="1.0" encoding="UTF-8"?> <Error><Code>NoSuchKey</Code><Message>The specified key does not exist.</Message> (truncated...)

But now, after this fix:

PHP Fatal error: Uncaught exception 'Aws\S3\Exception\S3Exception' with message 'Error executing "GetObject" on "https://bucket.s3.us-east-2.amazonaws.com/no-exists"; AWS HTTP error: NoSuchKey (client): The specified key does not exist.' GuzzleHttp\Exception\ClientException: Client error: `GET https://bucket.s3.us-east-2.amazonaws.com/no-exists` resulted in a `404 Not Found` response: <?xml version="1.0" encoding="UTF-8"?> <Error><Code>NoSuchKey</Code><Message>The specified key does not exist.</Message> (truncated...)

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

src/Api/ErrorParser/JsonParserTrait.php

src/Api/Parser/AbstractRestParser.php

stobrien89

A few more comments- I just responded to that one question without doing a proper review, but thanks for the quick turnaround

src/Api/ErrorParser/JsonRpcErrorParser.php

src/Api/Parser/AbstractRestParser.php

src/Api/Parser/QueryParser.php

stobrien89 · 2026-02-23T21:19:30Z

src/WrappedHttpHandler.php

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

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


Ah, I see. So basically a caller will still see the same thing, just printed from somewhere else?

stobrien89 · 2026-02-24T23:17:10Z

src/Api/Parser/AbstractRestParser.php

+        } else {
+            $response = AbstractParser::getResponseWithCachingStream($response);
+
+            $rawBody = AbstractParser::getBodyContents($response);


Could we prevent the double read here by using getSize() in the seekable case? At least for RestJsonParser, this method is also called in payload()

stobrien89 · 2026-02-24T23:30:34Z

src/Api/Parser/AbstractRestParser.php

            //Unions must have at least one member set to a non-null value
            // If the body is empty, we can assume it is unset
-            if (!empty($member['union']) && ($body->isSeekable() && !$body->getSize())) {
+            $rawBody = AbstractParser::getBodyContents($response);


I think this would be a third read if we're using rest-json

stobrien89 · 2026-02-24T23:31:48Z

src/CloudWatchLogs/CloudWatchLogsClient.php

-        'StartLiveTail' => true
-    ];

    public function __construct(array $args)


We can probably do away with the constructor altogether, but not required

stobrien89 · 2026-02-24T23:36:51Z

tests/Api/Parser/RestJsonParserTest.php

+        // Not error occurred. Test successfully.
+        // Previously, on non-seekable streams it would have failed.
+        // Because, it would have tried to parse an empty string.
+        $this->assertTrue(true);


Could we strengthen the test here with some assertions on the parsed result?

stobrien89 · 2026-02-24T23:37:51Z

src/Api/Parser/AbstractParser.php

+    public static function getBodyContents(ResponseInterface $response): string
+    {
+        $body = $response->getBody();
+        if ($body->isSeekable()) {
+            $body->rewind();
+        }
+
+        return $body->getContents();
+    }
+
+    public static function getResponseWithCachingStream(
+        ResponseInterface $response
+    ): ResponseInterface
+    {
+        if (!$response->getBody()->isSeekable()) {
+            return $response->withBody(
+                new CachingStream($response->getBody())
+            );
+        }
+
+        return $response;
+    }


These two methods should have test coverage

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

src/Api/ErrorParser/JsonRpcErrorParser.php Outdated Show resolved Hide resolved

src/Api/ErrorParser/JsonParserTrait.php Outdated Show resolved Hide resolved

src/Api/Parser/AbstractRestParser.php Outdated Show resolved Hide resolved

chore: simplify logic

2f37032

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

Merge branch 'master' into feat_add_streaming_flag_middleware

f78557a

yenfryherrerafeliz added 4 commits February 19, 2026 14:03

chore: fix missing braket

6a8bc47

chore: use helper to get raw response content

c5c0b50

chore: remove unnecessary condition from tests

17ed9cd

chore: remove invalid import

19193a7

stobrien89 requested changes Feb 23, 2026

View reviewed changes

yenfryherrerafeliz added 2 commits February 23, 2026 16:47

chore: add caching body in other error parsers

b223153

chore: use existent helper for response caching stream

2922628

yenfryherrerafeliz requested a review from stobrien89 February 24, 2026 01:03

yenfryherrerafeliz mentioned this pull request Feb 24, 2026

S3, check if file exists => Error parsing XML: String could not be parsed as XML #3181

Open

1 task

stobrien89 requested changes Feb 24, 2026

View reviewed changes

Comments

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yenfryherrerafeliz Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Uh oh!

stobrien89 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yenfryherrerafeliz commented Nov 20, 2025 •

edited

Loading

yenfryherrerafeliz Feb 24, 2026 •

edited

Loading