Create common interface for aio response classes #1328

stj · 2025-04-21T14:57:42Z

Having the parse method async on all parsers means the need for checking that parse is a coroutine is not needed anymore. The check for coroutine or not moved into the parser where a private method can be async or sync. This cleans up the public interface of the aio response classes.

Description of Change

Change is extracted out of pull #1323 after a review comment ask for it.

Assumptions

Replace this text with any assumptions made (if any)

Checklist for All Submissions

Checklist when updating botocore and/or aiohttp versions

I have read and followed CONTRIBUTING.rst
I have updated test_patches.py where/if appropriate (also check if no changes necessary)
I have ensured that the awscli/boto3 versions match the updated botocore version
I have added URL to diff: boto/botocore@[CURRENT_BOTO_VERSION_TAG]...[NEW_BOTO_VERSION_TAG]

Having the `parse` method async on all parsers means the need for checking that `parse` is a coroutine is not needed anymore. The check for coroutine or not moved into the parser where a private method can be async or sync. This cleans up the public interface of the aio response classes.

aiobotocore/parsers.py

codecov · 2025-04-21T15:14:17Z

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.79%. Comparing base (d7c1092) to head (b490745).

Files with missing lines	Patch %	Lines
aiobotocore/parsers.py	90.90%	2 Missing ⚠️
aiobotocore/response.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1328      +/-   ##
==========================================
- Coverage   90.79%   90.79%   -0.01%     
==========================================
  Files          68       68              
  Lines        6553     6548       -5     
==========================================
- Hits         5950     5945       -5     
  Misses        603      603

Flag	Coverage Δ
unittests	`90.79% <88.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aiobotocore/parsers.py

jakob-keller · 2025-04-21T17:47:14Z

Looks good to me! Would you mind adding an entry to CHANGES.rst?

stj · 2025-04-21T18:08:25Z

How does

refactor AioResponseParser to have a consistent async implementation of the parse method.

sound for a changelog entry? It communicates a change and where it is, though doesn't say what the implications are for aiobotocore users. Is parse a public interface?

thehesiod · 2025-04-21T18:48:43Z

aiobotocore/endpoint.py

+        parsed_response = await parser.parse(
+            response_dict, operation_model.output_shape
+        )


have we validated across all the parsers this is now true? Are there parsers that don't inherit from our baseclass?

i see everything parents to ResponseParser 👍🏼

Good call!

Upstream has a number of parsers that we have not patched:

BaseXMLResponseParser

BaseJSONParser

BaseEventStreamParser

EventStreamJSONParser

EventStreamXMLParser

BaseRestParser

The Base* parsers should be fine, since we patch any relevant child parsers. But I believe the EventStream* parsers might be problematic. They are used indirectly and might need to be patched to work correctly.

I could prepare a follow-up of #1312 to fully patch the parser class hierarchy. That would be a prerequisite for this PR, IMO. Any thoughts?

#1329 makes sure that EVENT_STREAM_PARSER_CLS class attributes ("indirect" uses) point to patched parsers. Once that is merged, we can continue with this one.

My belief atm is that not having an async version for EventStream* is fine.

I guess it would be, as long as we continue to allow for both sync and async variants of parse(). However, this PR takes away that flexibility and we need to be careful to handle all parser classes, as @thehesiod rightfully pointed out. As far as I understand, that includes parser classes that we have not patched previously, hence #1329. I could well be on the wrong track here, though.

An alternative would be to simply keep the current implementation of parse() and move it up the AioResponseParser, but leave everything else as it is.

ya that's why I was waiting to do a review of this PR when I had some time, we need to thoroughly look at how these classes are used

lets report back when one of us does that

How about allowing sync parse methods but emit a deprecation warning?

sounds like a plan, would be safer

webknjaz · 2025-04-25T12:22:09Z

CHANGES.rst

@@ -5,6 +5,7 @@ Changes
 ^^^^^^^^^^^^^^^^^^^
 * patch `ResponseParser`
 * use SPDX license identifier for project metadata
+* refactor `AioResponseParser` to have a consistent async implementation of the `parse` method.


It's RST

Suggested change

* refactor `AioResponseParser` to have a consistent async implementation of the `parse` method.

* refactor ``AioResponseParser`` to have a consistent async implementation of the ``parse`` method.

To avoid a breaking change the callers of a ResponseParser.parse method yield a deprecation warning when a non-async method is found.

jakob-keller · 2025-04-26T06:52:06Z

aiobotocore/endpoint.py

-            parsed_response = parser.parse(
-                response_dict, operation_model.output_shape
+            warnings.deprecated(
+                f"The use of non-async `parse` function on {parser.__class__.__name__} is deprecated."


Why not use type(parser).__name__?

jakob-keller · 2025-04-26T06:52:57Z

aiobotocore/response.py

-        )
-    else:
-        parsed = parser.parse(response_dict, operation_model.output_shape)
+    parsed = await parser.parse(response_dict, operation_model.output_shape)


Apply deprecation pattern from endpoints.py

jakob-keller · 2025-04-26T06:55:12Z

tests/test_patches.py

That test was slightly refactored in the default branch. Please do a rebase or merge in new commits.

jakob-keller · 2025-04-26T23:04:46Z

@stj: I discovered a few more quirks with regards to event streams and decided to created #1347 which is meant to supersede this PR. I hope you don't mind and thanks again for the work!

jakob-keller · 2025-04-29T16:38:42Z

Superseded by #1347

stj commented Apr 21, 2025

View reviewed changes

aiobotocore/parsers.py Outdated Show resolved Hide resolved

stj mentioned this pull request Apr 21, 2025

Update botocore to 1.37.3 #1323

Merged

13 tasks

This comment was marked as resolved.

Sign in to view

jakob-keller self-assigned this Apr 21, 2025

jakob-keller added the enhancement New feature or request label Apr 21, 2025

jakob-keller reviewed Apr 21, 2025

View reviewed changes

aiobotocore/parsers.py Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

Address code review comments

5893c16

stj requested review from thehesiod and jakob-keller April 21, 2025 16:41

This comment was marked as resolved.

Sign in to view

Changelog entry

b490745

jakob-keller previously approved these changes Apr 21, 2025

View reviewed changes

jakob-keller enabled auto-merge April 21, 2025 18:46

thehesiod reviewed Apr 21, 2025

View reviewed changes

jakob-keller disabled auto-merge April 21, 2025 18:54

jakob-keller self-requested a review April 21, 2025 19:19

jakob-keller mentioned this pull request Apr 21, 2025

Patch ResponseParser and remaining subclasses #1329

Merged

14 tasks

webknjaz reviewed Apr 25, 2025

View reviewed changes

Deprecate non-async ResponseParser.parse methods

37b027c

To avoid a breaking change the callers of a ResponseParser.parse method yield a deprecation warning when a non-async method is found.

stj dismissed jakob-keller’s stale review via 37b027c April 25, 2025 23:41

jakob-keller requested changes Apr 26, 2025

View reviewed changes

jakob-keller mentioned this pull request Apr 26, 2025

Patch ResponseParser.parse() and others to prepare for upstream bump #1347

Merged

14 tasks

jakob-keller closed this Apr 29, 2025

stj deleted the override-AioResponseParser-parse branch April 29, 2025 18:52

	* refactor `AioResponseParser` to have a consistent async implementation of the `parse` method.
	* refactor ``AioResponseParser`` to have a consistent async implementation of the ``parse`` method.

Uh oh!

Create common interface for aio response classes #1328

Create common interface for aio response classes #1328

Uh oh!

Conversation

stj commented Apr 21, 2025

Description of Change

Assumptions

Checklist for All Submissions

Checklist when updating botocore and/or aiohttp versions

Uh oh!

Uh oh!

codecov bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

jakob-keller commented Apr 21, 2025

Uh oh!

stj commented Apr 21, 2025

Uh oh!

This comment was marked as resolved.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakob-keller commented Apr 26, 2025

Uh oh!

jakob-keller commented Apr 29, 2025

Uh oh!

Uh oh!

codecov bot commented Apr 21, 2025 •

edited

Loading